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

[Damien Le Moal]

On 2026/04/12 21:40, Phil Pemberton wrote:
> 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.

Ideally, we want to keep the default 1 LUN value and change it to a higher count
only if we probe that we are dealing with an ATAPI device (device class is
ATAPI). Not sure if that is possible. Need to look again at that code.

>>>    - 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.

OK. I misunderstood your change. I really need to look again at that code, which
I have not done in a while.

I think your change may be generally OK, but I am worried that things like
suspend/resume may have issues. Have you tested that ?

Anyway, please give us some time to look into this (sorry, but I am super busy
these days, so it may take a couple of weeks).

-- 
Damien Le Moal
Western Digital Research