Re: [PATCH v5 2/9] ata: libata-scsi: Improve ata_scsi_handle_link_detach()

[Niklas Cassel <cassel@kernel.org>]

On Fri, Sep 06, 2024 at 10:58:40AM +0900, Damien Le Moal wrote:
> A struct ata_device flags are always set iand cleared with the device

s/iand/and/

I can see many cases function that currently set or clear dev->flags
withouth holding ap->lock, done by functions that run in EH context,
which will thus not hold ap->lock.

Perhaps rephrase this to:
struct ata_device flags should always be set and cleared with the device


> port locked.  Testing for a flag should thus also be done while holding
> the device port lock. In accordance to this, modify
> ata_scsi_handle_link_detach() to both test and clear the
> ATA_DFLAG_DETACHED flag while holding the device port lock.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-scsi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a3ffce4b218d..1a135d44286c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4616,10 +4616,12 @@ static void ata_scsi_handle_link_detach(struct ata_link *link)
>  	ata_for_each_dev(dev, link, ALL) {
>  		unsigned long flags;
>  
> -		if (!(dev->flags & ATA_DFLAG_DETACHED))
> +		spin_lock_irqsave(ap->lock, flags);
> +		if (!(dev->flags & ATA_DFLAG_DETACHED)) {
> +			spin_unlock_irqrestore(ap->lock, flags);
>  			continue;
> +		}
>  
> -		spin_lock_irqsave(ap->lock, flags);
>  		dev->flags &= ~ATA_DFLAG_DETACHED;
>  		spin_unlock_irqrestore(ap->lock, flags);
>  
> -- 
> 2.46.0
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>