Re: [PATCH 02/19] ata: libata-core: Fix port and device removal

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

On 9/11/23 15:37, Hannes Reinecke wrote:
> On 9/11/23 06:02, Damien Le Moal wrote:
>> Whenever an ATA adapter driver is removed (e.g. rmmod),
>> ata_port_detach() is called repeatedly for all the adapter ports to
>> remove (unload) the devices attached to the port and delete the port
>> device itself. Removing of devices is done using libata EH with the
>> ATA_PFLAG_UNLOADING port flag set. This causes libata EH to execute
>> ata_eh_unload() which disables all devices attached to the port.
>>
>> ata_port_detach() finishes by calling scsi_remove_host() to remove the
>> scsi host associated with the port. This function will trigger the
>> removal of all scsi devices attached to the host and in the case of
>> disks, calls to sd_shutdown() which will flush the device write cache
>> and stop the device. However, given that the devices were already
>> disabled by ata_eh_unload(), the synchronize write cache command and
>> start stop unit commands fail. E.g. running "rmmod ahci" with first
>> removing sd_mod results in error messages like:
>>
>> ata13.00: disable device
>> sd 0:0:0:0: [sda] Synchronizing SCSI cache
>> sd 0:0:0:0: [sda] Synchronize Cache(10) failed: Result:
>> hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
>> sd 0:0:0:0: [sda] Stopping disk
>> sd 0:0:0:0: [sda] Start/Stop Unit failed: Result: hostbyte=DID_BAD_TARGET
>> driverbyte=DRIVER_OK
>>
>> Fix this by removing all scsi devices of the ata devices connected to
>> the port before scheduling libata EH to disable the ATA devices.
>>
>> Fixes: 720ba12620ee ("[PATCH] libata-hp: update unload-unplug")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>   drivers/ata/libata-core.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index c4898483d716..693cb3cd70cd 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5952,11 +5952,30 @@ static void ata_port_detach(struct ata_port *ap)
>>       struct ata_link *link;
>>       struct ata_device *dev;
>>   -    /* tell EH we're leaving & flush EH */
>> +    /* Wait for any ongoing EH */
>> +    ata_port_wait_eh(ap);
>> +
>> +    mutex_lock(&ap->scsi_scan_mutex);
>>       spin_lock_irqsave(ap->lock, flags);
>> +
>> +    /* Remove scsi devices */
>> +    ata_for_each_link(link, ap, HOST_FIRST) {
>> +        ata_for_each_dev(dev, link, ALL) {
>> +            if (dev->sdev) {
>> +                spin_unlock_irqrestore(ap->lock, flags);
>> +                scsi_remove_device(dev->sdev);
>> +                spin_lock_irqsave(ap->lock, flags);
>> +                dev->sdev = NULL;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* Tell EH to disable all devices */
>>       ap->pflags |= ATA_PFLAG_UNLOADING;
>>       ata_port_schedule_eh(ap);
>> +
>>       spin_unlock_irqrestore(ap->lock, flags);
>> +    mutex_unlock(&ap->scsi_scan_mutex);
>>   
> Are you sure about releasing scan_mutex after ata_port_schedule_eh()?
> Technically ata_port_schedule_eh() will be calling into SCSI rescan, which
> would take scan_mutex ...
> Not that it matter much, seeing that we've disconnected all devices, but still ...

UNLOADING is set and in that case, EH does nothing else than removing the
devices. So I think this is OK.

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research