Re: [PATCH 3/3] ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

[Damien Le Moal <dlemoal@kernel.org>]

On 4/9/26 23:05, Phil Pemberton wrote:
> Some ATAPI devices (e.g. the Panasonic PD/CD combo drives) implement
> multiple logical units. For instance the aforementioned PD/CD has a CD
> drive on LUN 0 and the rewritable Phase-change Dual (PD) block device on
> LUN 1.
> 
> ata_scsi_scan_host() previously only scanned LUN 0 via
> __scsi_add_device(). With the multi-LUN work now in place, extend this
> scan to probe for additional LUNs on devices which have BLIST_FORCELUN
> set in the SCSI device info table.
> 
> Scanning stops when __scsi_add_device() fails, or the device reports
> device type 0x1f (unknown or no device type). The PD drive returns this
> for unimplemented LUNs.
> 
> The aforementioned BLIST_FORCELUN guard prevents non-multilun devices
> from being affected.
> 
> Tested with a Panasonic LF-1195C PD/CD with Compaq firmware, which now
> correctly enumerates as a CD drive (sr) and PD optical drive (sd).
> 
> Also tested with a LITE-ON iHAS124 DVD drive, which has a single LUN and
> ignores the LUN parameter in the CDB. As a result, without the
> BLIST_FORCELUN guard, this drive would pop up as eight separate devices.
> 
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
> ---
>  drivers/ata/libata-scsi.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index dc6829e60fb3..0a7ce44118fe 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4732,6 +4732,35 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>  			if (!IS_ERR(sdev)) {
>  				dev->sdev = sdev;
>  				ata_scsi_assign_ofnode(dev, ap);
> +				/*
> +				 * Multi-LUN ATAPI devices (e.g. PD/CD combo
> +				 * drives) are flagged BLIST_FORCELUN in
> +				 * scsi_devinfo.  Probe additional LUNs when
> +				 * the flag is set.
> +				 */
> +				if (dev->class == ATA_DEV_ATAPI &&
> +				    (sdev->sdev_bflags & BLIST_FORCELUN)) {
> +					u64 lun;
> +
> +					for (lun = 1; lun < ap->scsi_host->max_lun;
> +					     lun++) {
> +						struct scsi_device *extra;
> +
> +						extra = __scsi_add_device(
> +							ap->scsi_host,
> +							channel, id, lun,
> +							NULL);
> +						if (IS_ERR(extra))
> +							break;
> +						/* PDT 0x1f: no device type */
> +						if (extra->type == 0x1f) {
> +							scsi_remove_device(extra);
> +							scsi_device_put(extra);
> +							break;
> +						}
> +						scsi_device_put(extra);
> +					}
> +				}

It would be a lot nicer and more readable to have this hunk as a helper
function, e.g.: ata_scsi_scan_atapi_luns().

>  				scsi_device_put(sdev);
>  			} else {
>  				dev->sdev = NULL;


-- 
Damien Le Moal
Western Digital Research