Re: [PATCH v6 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array

[Phil Pemberton]

On 09/06/2026 08:22, Hannes Reinecke wrote:
> > +            for (lun = dev->nr_luns - 1; lun >= 0; lun--) {
> > +                if (!dev->sdev[lun])
> > +                    continue;
> >                   spin_unlock_irqrestore(ap->lock, flags);
> > -                scsi_remove_device(dev->sdev);
> > +                scsi_remove_device(dev->sdev[lun]);
> >                   spin_lock_irqsave(ap->lock, flags);
> > -                dev->sdev = NULL;
> > +                dev->sdev[lun] = NULL;
>
> As pointed out by sashiko, this is racy.
> Please move 'dev->sdev[lun] = NULL' before unlock, and hold
> 'sdev' in a temporary variable.
>
> Maybe even make this a separate patch, then this patch can be kept
> as just the interface change.

The race was introduced by the patch itself and only exists because v6 
got the NULL placement wrong. I think splitting it out would leave patch 
2 transiently broken, which would make bisecting harder not easier.

IMHO a series where every commit is correct is more useful than one with 
a known unfixed bug. The fix is only 3 lines and I've kept it in patch 2 
where the cause and correction sit near each other and are easy to review.

I'm happy to split it if you feel strongly about it - or was this 
thinking out loud?

> > -        scsi_remove_device(sdev);
> > -        scsi_device_put(sdev);
> > +                 dev_name(&sdevs[lun]->sdev_gendev));
> > +        scsi_remove_device(sdevs[lun]);
> > +        scsi_device_put(sdevs[lun]);
>  >       }>   }
> Wouldn't it be simpler to have another mutex under 'dev' to protect
> 'dev->sdev[]' ?
> That would get us out of this mess, and we could do away with the
> temporary adev array.

I looked at that, but dev->sdev[] is also accessed from IRQ context: 
ata_scsi_media_change_notify runs under ap->lock with IRQs disabled, and 
so does ata_scsi_offline_dev. A mutex would only protect sleepable 
paths, leaving the IRQ paths unprotected. That feels worse to me than 
the current consistent use of ap->lock for all accesses.

The snapshot approach in ata_scsi_remove_dev is boilerplate but it is 
correct.

If you think a mutex-based redesign is the way to go, I'm happy to look 
at it as a follow-up.


> > @@ -4872,9 +4872,12 @@ static void ata_scsi_handle_link_detach(struct 
> > ata_link *link)
> >    */
> >   void ata_scsi_media_change_notify(struct ata_device *dev)
> >   {
> > -    if (dev->sdev)
> > -        sdev_evt_send_simple(dev->sdev, SDEV_EVT_MEDIA_CHANGE,
> > -                     GFP_ATOMIC);
> > +    int lun;
> > +
> > +    for (lun = 0; lun < dev->nr_luns; lun++)
> > +        if (dev->sdev[lun])
> > +            sdev_evt_send_simple(dev->sdev[lun],
> > +                         SDEV_EVT_MEDIA_CHANGE, GFP_ATOMIC);
> >   }
>
> I guess the iteration need to be protected somehow, either by
> taking 'ap->lock' or with the dedicated mutex from the above
> comments.

Both call sites are inside sata_async_notification(). That's documented 
as "LOCKING: spin_lock_irqsave(host lock)", so ap->lock is already held 
and the iteration is protected - QED.


> > diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
> > index 414e7c63bd85..dca774d8ec05 100644
> > --- a/drivers/ata/libata-zpodd.c
> > +++ b/drivers/ata/libata-zpodd.c
> > @@ -185,7 +185,7 @@ void zpodd_enable_run_wake(struct ata_device *dev)
> >   {
> >       struct zpodd *zpodd = dev->zpodd;
> > -    sdev_disable_disk_events(dev->sdev);
> > +    sdev_disable_disk_events(ata_dev_scsi_device(dev, 0));
>
> I _think_  we should call this for every LUN.

Done in v7, all three ZPODD functions (enable_run_wake, post_poweron, 
wake_dev) iterate over all LUN slots.


Thanks,
--
Phil.
philpem@xxxxxxxxxxxxx
https://www.philpem.me.uk/