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

[Hannes Reinecke]

On 4/12/26 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.
Don't get too excited. This is ancient technology, with an extremely 
narrow user-base :-)
'Were available' is not identical to 'in current use', and I'm somewhat
disinclined to add support for technology with no users.

> 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.
Hmm. I really would like to restrict max lun to 1 (as it somewhat mimics 
the master/slave thingie on IDE). But higher than that really is 
arbitrary; the next sensible limit would be '7', as this is what SCSI-2
could handle. I completely fail to see the motivation to have a limit
somewhere in between.

> 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).
And how would you address the 'sdev' of LUN 1?
(and how of LUN 2, if we decide to have one?)

Please, don't. The correct way would be to convert 'dev->sdev' into
a list.

Cheers,

Hannes
--
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@xxxxxxx                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich