Re: [PATCH v7 09/23] scsi: sd: Do not issue commands to suspended disks on shutdown

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

On 2023/09/27 16:55, Bart Van Assche wrote:
> On 9/26/23 01:14, Damien Le Moal wrote:
>> @@ -3891,21 +3895,26 @@ static int sd_suspend_runtime(struct device *dev)
>>   static int sd_resume(struct device *dev, bool runtime)
>>   {
>>   	struct scsi_disk *sdkp = dev_get_drvdata(dev);
>> -	int ret;
>> +	int ret = 0;
>>   
>>   	if (!sdkp)	/* E.g.: runtime resume at the start of sd_probe() */
>>   		return 0;
> 
> As far as I can tell there is nothing that prevents system wide
> suspend or resume after a SCSI disk has been discovered and before
> sd_probe() is called(). So I think that "sdkp->suspended = false;"
> has to be added in the above if-statement. This is the how SCSI
> disks are registered (synchronous case only):

The above if statement being "if (!sdkp)", if I add "sdkp->suspended = false;",
I will get a NULL pointer dereference. So I do not understand your point here...

> 
> scsi_probe_and_add_lun(target, bflagsp, sdevp, rescan, hostdata)
>    scsi_alloc_sdev(starget, lun, hostdata)
>      __scsi_init_queue(&sdev->host)
>      scsi_sysfs_device_initialize(sdev)
>      shost->hostt->slave_alloc(sdev)
>    scsi_probe_lun(sdev, ...)
>      scsi_execute_req(sdev, INQUIRY)
>    scsi_add_lun(sdev, ...)
>      scsi_device_set_state(sdev, SDEV_RUNNING)
>      sdev->host->hostt->slave_configure(sdev) /* may do I/O */
>      scsi_sysfs_add_sdev(sdev) /* enables runtime PM */
>        scsi_target_add(starget)
>        device_add(&sdev->sdev_gendev)
>          kobject_add(&dev->kobj, ...)
>          bus_add_device(dev)
>          bus_probe_device(dev)
>            device_initial_probe(dev)
>              __device_attach(dev, /*allow_async=*/true)
>                __device_attach_driver(drv, dev, ...)
>                  driver_probe_device(drv, dev)
>                    really_probe(dev, drv)
>                      dev->bus->probe(dev) = sd_probe(dev)
>                        gd = blk_mq_alloc_disk_for_queue()
>                        device_add(&sdkp->dev)
>                        sd_revalidate_disk(gd)
>                        device_add_disk(dev, gd, NULL)
>        device_add(&sdev->sdev_dev)
>        bsg_scsi_register_queue(rq, &sdev->sdev_gendev)
> 
> Thanks,
> 
> Bart.
> 

-- 
Damien Le Moal
Western Digital Research