Re: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks

[Bart Van Assche]

On 1/20/23 03:33, Anjana Hari wrote:
> +		/* Make sure that UTRL and UTMRL base address registers
> +		 * are updated with the latest queue addresses. Only after
> +		 * updating these addresses, we can queue the new commands.
> +		 */

Please follow the kernel coding style and start comment blocks with "/*" 
on a line by its own.

> +	/* Resuming from hibernate, assume that link was OFF */
> +	if (hba->restore)
> +		ufshcd_set_link_off(hba);

Why two successive 'if (hba->restore)' statements? Can these two 
if-statements be combined into a single if-statement?

> +int ufshcd_system_freeze(struct device *dev)
> +{
> +
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	/*
> +	 * Run time resume the controller to make sure
> +	 * the PM work queue threads do not try to resume
> +	 * the child (scsi host), which leads to errors as
> +	 * the controller is not yet resumed.
> +	 */
> +	pm_runtime_get_sync(hba->dev);
> +	ret = ufshcd_system_suspend(dev);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_system_freeze);

Why does the above function use 'hba->dev' instead of 'dev'?

I do not understand the comment in the above function. My understanding 
is that the power management core serializes hibernation and runtime 
power management. How could runtime resume be in progress when 
ufshcd_system_freeze() is called? Additionally, shouldn't 
ufshcd_system_freeze() skip the UFS controller if it is already runtime 
suspended instead of runtime resuming it? Some time ago I added the 
following code in sd_suspend_system() (commit 9131bff6a9f1 ("scsi: core: 
pm: Only runtime resume if necessary"):

	if (pm_runtime_suspended(dev))
		return 0;

Thanks,

Bart.