Re: [PATCH 2/3] ata: libata-scsi: enable multi-LUN support for ATAPI devices

[Phil Pemberton]

On 12/04/2026 08:38, Damien Le Moal wrote:
> On 4/9/26 23:05, Phil Pemberton wrote:
> >    - Raises max_lun from 1 to 8 (matching the SCSI host default).
> >      Sequential LUN scanning stops at the first non-responding LUN, so
> >      single-LUN devices are unaffected.
>
> If the only case that we can encounter with libata are these special ATAPI
> devices with 2 LUNs, I would limit the maximum to 2.

I'm inclined to agree, but there are devices with higher LUN counts: the 
Nakamichi CD changers. The MJ-4.4 and MJ-5.16 were available in an ATAPI 
variant which exposed a LUN for each disc in the changer stack. There's 
a Cathode Ray Dude video demonstrating the latter.

I like the idea of the lower LUN cap for compatibility, but I think I'd 
hedge bets by also having a module parameter (e.g. libata.atapi_max_lun) 
to override it. Default 2 seems like a good compromise.

In any case, the BLIST_FORCELUN gate should limit things to one LUN for 
any device which isn't on the device list.


> >    - ata_scsi_dev_config() previously assigned dev->sdev = sdev for every
> >      LUN configured.  With multiple LUNs sharing one ata_device, this
> >      caused dev->sdev to be overwritten by each non-LUN-0 sdev.  Restrict
> >      the assignment to LUN 0 so that dev->sdev always tracks the
> >      canonical scsi_device for the underlying ATA device.
>
> Special casing this does not seem nice at all. Why not simply increasing the
> sdev reference count when it is assigned to a LUN that is not LUN 0 ? And drop
> that reference when the LUN is torn down ? That will remove any dependency on
> the order in which LUNs are torn down too.

The if (sdev->lun == 0) guard isn't about teardown ordering; it's about 
which device dev->sdev points at.

dev->sdev is a single pointer, but with multi-LUN ATAPI there are now 
multiple sdevs sharing one ata_device. Without the guard, each call to 
ata_scsi_dev_config() overwrites the pointer, so it ends up tracking the 
last-configured LUN (likely the highest-numbered one).

This is really made clear by ata_scsi_sdev_destroy(). It uses
  dev->sdev == sdev
to decide when to schedule ATA-level detach. If the pointer has been 
overwritten, destroying the higher LUN will tear down the whole device, 
and destroying LUN 0 won't trigger a detach.

Refcounting keeps whichever sdev is stored there alive, but it doesn't 
decide which one should be stored in the first place. Picking LUN 0 
keeps the existing invariant intact for single-LUN devices, and the 
other users of dev->sdev (scsi_remove_device() in ata_port_detach(), 
ACPI uevents, zpodd) continue to operate on a stable, well-defined sdev.

Happy to add scsi_device_get() on the LUN-0 sdev when a higher LUN is 
configured, and the matching put in sdev_destroy, so LUN 0 can't be 
freed while higher LUNs still exist. That gives you the ordering 
guarantee on top of the pointer-stability guarantee.

> >    - ata_scsi_sdev_destroy() detached the entire ATA device whenever
> >      dev->sdev was non-NULL.  When a spurious multi-LUN scan result was
> >      removed, this incorrectly tore down the underlying device.  Detach
> >      only when the canonical (LUN 0) sdev is being destroyed.
>
> This should not happen with the reference count change suggested above.
>
> This is a lot of changes for one patch. So I also suggest splitting this patch.

Will do.

All other comments acked.


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