linux-kernel - Re: [PATCH v12 1/2] scsi: ufs: Enable power management for wlun

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, 18 Mar 2021 20:12:21 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Asutosh Das <asutoshd@...eaurora.org>, cang@...eaurora.org,
        martin.petersen@...cle.com, adrian.hunter@...el.com,
        linux-scsi@...r.kernel.org
Cc:     linux-arm-msm@...r.kernel.org,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        Pedro Sousa <pedrom.sousa@...opsys.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Stanley Chu <stanley.chu@...iatek.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Bean Huo <beanhuo@...ron.com>,
        Kiwoong Kim <kwmad.kim@...sung.com>,
        Wei Yongjun <weiyongjun1@...wei.com>,
        Lee Jones <lee.jones@...aro.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Dinghao Liu <dinghao.liu@....edu.cn>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        Satya Tangirala <satyat@...gle.com>,
        open list <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES" 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM ARCHITECTURES" 
        <linux-samsung-soc@...r.kernel.org>,
        "moderated list:UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER..." 
        <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH v12 1/2] scsi: ufs: Enable power management for wlun

On 3/18/21 5:35 PM, Asutosh Das wrote:
> During runtime-suspend of ufs host, the scsi devices are
> already suspended and so are the queues associated with them.
> But the ufs host sends SSU to wlun during its runtime-suspend.
> During the process blk_queue_enter checks if the queue is not in
> suspended state. If so, it waits for the queue to resume, and never
> comes out of it.
> The commit
> (d55d15a33: scsi: block: Do not accept any requests while suspended)
> adds the check if the queue is in suspended state in blk_queue_enter().

What is the role of the WLUN during runtime suspend and why does a
command need to be sent to the WLUN during runtime suspend? Although it
is possible to derive this from the source code, please explain this in
the patch description.

What does the acronym SSU stand for? This doesn't seem like a commonly
used kernel acronym to me so please expand that acronym.

> Fix this by registering ufs device wlun as a scsi driver and
> registering it for block runtime-pm. Also make this as a
> supplier for all other luns. That way, this device wlun
> suspends after all the consumers and resumes after
> hba resumes.

That's an interesting solution.

> -void __exit ufs_debugfs_exit(void)
> +void ufs_debugfs_exit(void)

Is the above change related to the rest of this patch?

>  static struct platform_driver ufs_qcom_pltform = {
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> index 5b2bc1a..cbb5a90 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -97,7 +97,7 @@ static int ufs_bsg_request(struct bsg_job *job)
>  
>  	bsg_reply->reply_payload_rcv_len = 0;
>  
> -	pm_runtime_get_sync(hba->dev);
> +	scsi_autopm_get_device(hba->sdev_ufs_device);

Can the pm_runtime_get_sync() to scsi_autopm_get_device() changes be
moved into a separate patch?

> +static inline bool is_rpmb_wlun(struct scsi_device *sdev)
> +{
> +	return (sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN));
> +}

Has this patch been verified with checkpatch? Checkpatch should have
reported the following for the above code:

	return is not a function, parentheses are not required

> +static inline bool is_device_wlun(struct scsi_device *sdev)
> +{
> +	return (sdev->lun ==
> +		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN));
> +}

Same comment here.

>  		/*
> -		 * Don't assume anything of pm_runtime_get_sync(), if
> +		 * Don't assume anything of resume, if
>  		 * resume fails, irq and clocks can be OFF, and powers
>  		 * can be OFF or in LPM.
>  		 */

Please make better use of the horizontal space in the above comment by
making comment lines longer.

Thanks,

Bart.