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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Jan 2023 13:27:05 -0800
From:   Bart Van Assche <bvanassche@....org>
To:     Anjana Hari <quic_ahari@...cinc.com>, agross@...nel.org,
        andersson@...nel.org, jejb@...ux.ibm.com,
        martin.petersen@...cle.com
Cc:     alim.akhtar@...sung.com, avri.altman@....com,
        konrad.dybcio@...aro.org, linux-scsi@...r.kernel.org,
        linux-kernel@...r.kernel.org, quic_narepall@...cinc.com,
        quic_nitirawa@...cinc.com, quic_rampraka@...cinc.com
Subject: Re: [PATCH v3 1/1] scsi: ufs: Add hibernation callbacks

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.