Re: [PATCH] scsi: hisi_sas: work around build failure in suspend function

["chenxiang (M)" <chenxiang66@hisilicon.com>]

Hi Arnd,


在 2023/4/5 16:36, Arnd Bergmann 写道:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The suspend/resume functions in this driver seem to have multiple
> problems, the latest one just got introduced by a bugfix:
>
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: In function '_suspend_v3_hw':
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:5142:39: error: 'struct dev_pm_info' has no member named 'usage_count'
>   5142 |         if (atomic_read(&device->power.usage_count)) {
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c: In function '_suspend_v3_hw':
> drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:5142:39: error: 'struct dev_pm_info' has no member named 'usage_count'
>   5142 |         if (atomic_read(&device->power.usage_count)) {
>
> As far as I can tell, the 'usage_count' is not meant to be accessed
> by device drivers at all, though I don't know what the driver is
> supposed to do instead.

Thank you for reporting the issue.
There is a extreme situation that hisi_sas driver tries to resume 
controller when it is in the process of suspend, which will cause a 
deadlock.
So we check usage_count of controller, and if usage_count > 0, failed to 
suspend to avoid the issue. But there is no common function defined
in pm_runtime.h which check the usage_count of device, so use it 
directly (i saw the check also be used in other drivers).
But i didn't realize that member usage_count is defined only under 
CONFIG_PM=y.


>
> Another problem is the use of the deprecated UNIVERSAL_DEV_PM_OPS(),
> and marking functions as __maybe_unused to avoid warnings about
> unused functions.  This should probably be changed to using
> DEFINE_RUNTIME_DEV_PM_OPS().

We use UNIVERSAL_DEV_PM_OPS() just because runtime callbacks 
runtime_{suspend|resume} and system callbacks {suspend|resume} use the same
operations in the driver, otherwise, we need to use 
DEFINE_RUNTIME_DEV_PM_OPS() to define runtime callbacks and use 
DEFINE_SIMPLE_DEV_PM_OPS()
to define system callbacks.

>
> Both changes require actually understanding what the driver needs to
> do, and being able to test this, so instead here is the simplest
> patch to make it pass the randconfig builds instead.
>
> Fixes: e368d38cb952 ("scsi: hisi_sas: Exit suspend state when usage count is greater than 0")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Maintainers: If possible, please revisit this to do a proper fix.
> If that takes too much time, this patch can be applied as a
> workaround in the meantime, and might also help in case the
> e368d38cb952 patch gets backported to stable kernels.

It is ok to apply the patch as a workaround as soon as possible.

Reviewed-by: Xiang Chen <chenxiang66@hisilicon.com>

Thanks!

> ---
>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index d160b9b7479b..12d588454f5d 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -5139,11 +5139,13 @@ static int _suspend_v3_hw(struct device *device)
>   	flush_workqueue(hisi_hba->wq);
>   	interrupt_disable_v3_hw(hisi_hba);
>   
> +#ifdef CONFIG_PM
>   	if (atomic_read(&device->power.usage_count)) {
>   		dev_err(dev, "PM suspend: host status cannot be suspended\n");
>   		rc = -EBUSY;
>   		goto err_out;
>   	}
> +#endif
>   
>   	rc = disable_host_v3_hw(hisi_hba);
>   	if (rc) {
> @@ -5162,7 +5164,9 @@ static int _suspend_v3_hw(struct device *device)
>   
>   err_out_recover_host:
>   	enable_host_v3_hw(hisi_hba);
> +#ifdef CONFIG_PM
>   err_out:
> +#endif
>   	interrupt_enable_v3_hw(hisi_hba);
>   	clear_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags);
>   	clear_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags);