Re: [PATCH 5/5] scsi: Enable async shutdown support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Runtime PM usage counter and PM state leak on device_add error path.
- [High] Concurrent lockless bitfield mutations cause Data Race and PM state corruption.
--

commit 10ebc4309581b7d74ed1d3564b5f1af2a5cf5785
Author: David Jeffery <djeffery@redhat.com>

scsi: Enable async shutdown support

Like scsi's async suspend support, allow scsi devices to be shut down
asynchronously to reduce system shutdown time.

> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index dfc3559e7e04f..8fd317aef37b4 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1386,6 +1386,7 @@ static int scsi_target_add(struct scsi_target *starget)
>  	pm_runtime_set_active(&starget->dev);
>  	pm_runtime_enable(&starget->dev);
>  	device_enable_async_suspend(&starget->dev);
> +	dev_set_async_shutdown(&starget->dev);

[Severity: High]
This is a pre-existing issue, but does adding dev_set_async_shutdown() here
exacerbate a data race with system suspend?

In scsi_target_add(), device_add() is called earlier, exposing the device
to the PM core. If a system-wide suspend happens concurrently,
device_prepare() may lock the device and set the is_prepared bitfield.

Since device_enable_async_suspend() and dev_set_async_shutdown() locklessly
mutate bitfields in dev->power in the same memory word, couldn't the
resulting read-modify-write instructions overwrite and clear the is_prepared
flag set by the PM core?

>  	return 0;
>  }

[ ... ]

> @@ -1412,6 +1413,7 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>  	transport_configure_device(&starget->dev);
>  
>  	device_enable_async_suspend(&sdev->sdev_gendev);
> +	dev_set_async_shutdown(&sdev->sdev_gendev);

[Severity: High]
This isn't a bug introduced by this patch, but could the error path for
device_add(&sdev->sdev_gendev) leak the PM usage counter and runtime state?

When scsi_sysfs_add_sdev() is called, it increments the PM usage count via
scsi_autopm_get_device(sdev) and enables runtime PM. If the subsequent
device_add(&sdev->sdev_gendev) fails, the function returns the error directly
without dropping the usage counter or disabling runtime PM:

scsi_sysfs_add_sdev() {
    ...
    scsi_autopm_get_device(sdev);
    ...
    error = device_add(&sdev->sdev_gendev);
    if (error) {
        sdev_printk(...);
        return error;
    }
}

Would this prevent the device and its parent target from ever entering
runtime suspend, causing unbounded power drain?

>  	scsi_autopm_get_target(starget);
>  	pm_runtime_set_active(&sdev->sdev_gendev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701135015.81937-1-djeffery@redhat.com?part=5