linux-kernel - Re: [PATCH 1/2] scsi: ufs: core: 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:   Thu, 29 Dec 2022 07:06:38 -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 1/2] scsi: ufs: core: Add hibernation callbacks

On 12/21/22 09:12, Anjana Hari wrote:
> Add hibernation call backs - freeze, restore and thaw.
> Add the respective prototypes.
> 
> Signed-off-by: Anjana Hari <quic_ahari@...cinc.com>
> ---
>   drivers/ufs/core/ufshcd.c | 80 +++++++++++++++++++++++++++++++++++++++
>   include/ufs/ufshcd.h      |  6 +++
>   2 files changed, 86 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index bda61be5f035..c216a97dc1dd 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -9453,6 +9453,25 @@ static int ufshcd_resume(struct ufs_hba *hba)
>   
>   	/* enable the host irq as host controller would be active soon */
>   	ufshcd_enable_irq(hba);
> +
> +	if (hba->restore) {
> +		/* Configure UTRL and UTMRL base address registers */
> +		ufshcd_writel(hba, lower_32_bits(hba->utrdl_dma_addr),
> +			      REG_UTP_TRANSFER_REQ_LIST_BASE_L);
> +		ufshcd_writel(hba, upper_32_bits(hba->utrdl_dma_addr),
> +			      REG_UTP_TRANSFER_REQ_LIST_BASE_H);
> +		ufshcd_writel(hba, lower_32_bits(hba->utmrdl_dma_addr),
> +			      REG_UTP_TASK_REQ_LIST_BASE_L);
> +		ufshcd_writel(hba, upper_32_bits(hba->utmrdl_dma_addr),
> +			      REG_UTP_TASK_REQ_LIST_BASE_H);
> +		/* Commit the registers */
> +		mb();
> +	}

The comment above the mb() call is wrong. No matter the type of a 
barrier, a barrier controls the order in which load, store and/or MMIO 
operations happen. A barrier does not "commit" register writes.

The comment above mb() should explain which load, store and/or MMIO 
accesses are ordered and also why the ordering is necessary.

> +	/*
> +	 * Ensure no runtime PM operations take
> +	 * place in the hibernation and restore sequence
> +	 * on successful freeze operation.
> +	 */
> +	if (!ret)
> +		pm_runtime_disable(hba->dev);

I think the power management core already serializes system-wide power 
management state transitions and runtime power management and hence that 
the above code can be left out.

> +	/*
> +	 * Now any runtime PM operations can be
> +	 * allowed on successful restore operation
> +	 */
> +	if (!ret)
> +		pm_runtime_enable(hba->dev);

Same comment for the above code.

> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_system_restore);
> +
> +int ufshcd_system_thaw(struct device *dev)
> +{
> +
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	ret = ufshcd_system_resume(dev);
> +	if (!ret)
> +		pm_runtime_enable(hba->dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_system_thaw);

Also here, please remove the code related to runtime power management.

> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 5cf81dff60aa..dadb3c732be4 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -978,6 +978,9 @@ struct ufs_hba {
>   #endif
>   	u32 luns_avail;
>   	bool complete_put;
> +
> +	/* Distinguish between resume and restore */
> +	bool restore;
>   };

Please convert this member variable into a function argument. This 
probably will require adding an argument to existing functions.

Thanks,

Bart.