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

Phil Pemberton
2026-04-12T19:40:22Z

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@philpem.me.uk