On 2023/09/22 13:08, Bart Van Assche wrote: > On 9/22/23 12:10, Damien Le Moal wrote: >> Looking at the code, scsi_remove_host() calls scsi_forget_host() which calls >> __scsi_remove_device() for any device that is not in the SDEV_DEL state. >> __scsi_remove_device() then sets the state to SDEV_CANCEL. So it appears that >> the state should always be CANCEL and not running. However, my tests showed it >> to be running. I am not fully understanding how sd_remove() end up being called... > > I think this is how sd_sync_cache() gets called from inside > scsi_remove_host(): > > scsi_remove_host() > -> scsi_forget_host() > -> __scsi_remove_device() > -> device_del(&sdev->sdev_gendev) > -> bus_remove_device() > -> device_release_driver() > -> __device_release_driver() > -> sd_remove() > -> sd_shutdown() > -> sd_sync_cache() > > In other words, it is guaranteed that scsi_device_set_state(sdev, > SDEV_CANCEL) has been called before sd_remove() if it is called by > scsi_remove_host(). > >> I think we should investigate this further though, to make sure that we can >> always safely call sd_shutdown(). __scsi_remove_device() has this comment: >> >> /* >> * If blocked, we go straight to DEL and restart the queue so >> * any commands issued during driver shutdown (like sync >> * cache) are errored immediately. >> */ >> >> Which kind of give a hint that we should probably not blindy always try to call >> sd_shutdown(). > > Does that comment perhaps refer to the SDEV_BLOCK / SDEV_CREATED_BLOCK > states? Anyway, I'm wondering whether there are better ways to prevent > that it is attempted to queue SCSI commands if a SCSI device is > suspended, e.g. by checking the suspended state from inside > scsi_device_state_check() or scsi_dispatch_cmd(). Using information in the device ->power structure is not reliable without holding the device lock(), so we should not do that. But we can add a "suspended" scsi_device flag that we maintain on execution of sd_suspend_system() and sd_resume_system(). Many drivers do that... Thoughts ? -- Damien Le Moal Western Digital Research
