Re: [PATCH] Revert "scsi: sd: Do not repeat the starting disk message"

[Damien Le Moal <dlemoal@kernel.org>]

On 7/17/24 01:11, Johan Hovold wrote:
> This reverts commit 7a6bbc2829d4ab592c7e440a6f6f5deb3cd95db4.
> 
> The offending commit tried to suppress a double "Starting disk" message
> for some drivers, but instead started spamming the log with bogus
> messages every five seconds:
> 
> 	[  311.798956] sd 0:0:0:0: [sda] Starting disk
> 	[  316.919103] sd 0:0:0:0: [sda] Starting disk
> 	[  322.040775] sd 0:0:0:0: [sda] Starting disk
> 	[  327.161140] sd 0:0:0:0: [sda] Starting disk
> 	[  332.281352] sd 0:0:0:0: [sda] Starting disk
> 	[  337.401878] sd 0:0:0:0: [sda] Starting disk
> 	[  342.521527] sd 0:0:0:0: [sda] Starting disk
> 	[  345.850401] sd 0:0:0:0: [sda] Starting disk
> 	[  350.967132] sd 0:0:0:0: [sda] Starting disk
> 	[  356.090454] sd 0:0:0:0: [sda] Starting disk
> 	...
> 
> on machines that do not actually stop the disk on runtime suspend (e.g.
> the Qualcomm sc8280xp CRD with UFS).

This is odd. If the disk is not being being suspended, why does the platform
even enable runtime PM for it ? Are you sure about this ? Or is it simply that
the runtime pm timer is set to a very low interval ?

It almost sound like what we need to do here is suppress this message for the
runtime resume case, so something like:

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 2e933fd1de70..4261128bf1f3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -4220,7 +4220,8 @@ static int sd_resume_common(struct device *dev, bool runtime)
        if (!sdkp)      /* E.g.: runtime resume at the start of sd_probe() */
                return 0;

-       sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
+       if (!runtime)
+               sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");

        if (!sd_do_start_stop(sdkp->device, runtime)) {
                sdkp->suspended = false;

However, I would like to make sure that this platform is not calling
sd_resume_runtime() for nothing every 5s. If that is the case, then there is a
more fundamental problem here and reverting this patch is only hiding that.

> 
> Let's just revert for now to address the regression.
> 
> Fixes: 7a6bbc2829d4 ("scsi: sd: Do not repeat the starting disk message")
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/scsi/sd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 
> Hi,
> 
> I just noticed this regression that snuck into 6.10-final and tracked it
> down to 7a6bbc2829d4 ("scsi: sd: Do not repeat the starting disk
> message").
> 
> I wanted to get this out ASAP to address the immediate regression while
> someone who cares enough can work out a proper fix for the double start
> message (which seems less annoying).
> 
> Note that the offending commit is marked for stable.
> 
> Johan
> 
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 1b7561abe05d..6b64af7d4927 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -4119,6 +4119,8 @@ static int sd_resume(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>  
> +	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> +
>  	if (opal_unlock_from_suspend(sdkp->opal_dev)) {
>  		sd_printk(KERN_NOTICE, sdkp, "OPAL unlock failed\n");
>  		return -EIO;
> @@ -4135,13 +4137,12 @@ static int sd_resume_common(struct device *dev, bool runtime)
>  	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
>  		return 0;
>  
> -	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> -
>  	if (!sd_do_start_stop(sdkp->device, runtime)) {
>  		sdkp->suspended = false;
>  		return 0;
>  	}
>  
> +	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
>  	ret = sd_start_stop_device(sdkp, 1);
>  	if (!ret) {
>  		sd_resume(dev);

-- 
Damien Le Moal
Western Digital Research