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

[Damien Le Moal]

On 4/9/26 23:05, Phil Pemberton wrote:
> libata has never supported multi-LUN ATAPI devices like the Panasonic
> and NEC PD/CD combo drives due to three limitations:
> 
>   - shost->max_lun is hardcoded to 1 in ata_scsi_add_hosts(), which
>     stops the SCSI layer from probing any LUN other than 0.
> 
>   - __ata_scsi_find_dev() rejects all commands where scsidev->lun != 0,
>     returning NULL which causes DID_BAD_TARGET.
> 
>   - The SCSI-2 CDB LUN field (byte 1, bits 7:5) is never set. Older
>     multi-LUN ATAPI devices rely on this field to route commands to the
>     correct LUN, as transport-layer LUN addressing (per SPC-3+) is not
>     available over the ATA PACKET interface.
> 
> To fix all three, this change:
> 
>   - 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.

>   - In __ata_scsi_find_dev(), allow non-zero LUNs for ATAPI devices by
>     routing them to the same ata_device as LUN 0.
> 
>   - In atapi_xlat(), encode the target LUN into CDB byte 1 bits 7:5
>     before passing the command packet to the device.
> 
> These changes are prerequisites for probing additional LUNs during
> host scanning, which is done in a subsequent patch.
> 
> Additionally, fix two related issues exposed by multi-LUN scanning:
> 
>   - 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.

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

> 
> Assisted-by: Claude:claude-opus-4-6
> Signed-off-by: Phil Pemberton <philpem@xxxxxxxxxxxxx>
> ---
>  drivers/ata/libata-scsi.c | 38 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 3b65df914ebb..dc6829e60fb3 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -25,6 +25,7 @@
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_device.h>
>  #include <scsi/scsi_tcq.h>
> +#include <scsi/scsi_devinfo.h>
>  #include <scsi/scsi_transport.h>
>  #include <linux/libata.h>
>  #include <linux/hdreg.h>
> @@ -1131,7 +1132,14 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct queue_limits *lim,
>  	if (dev->flags & ATA_DFLAG_TRUSTED)
>  		sdev->security_supported = 1;
>  
> -	dev->sdev = sdev;
> +	/*
> +	 * Only LUN 0 is treated as the canonical scsi_device for the ATA
> +	 * device.  Multi-LUN ATAPI devices share a single ata_device, so
> +	 * dev->sdev must continue to track LUN 0 even when additional LUNs
> +	 * are added or removed.
> +	 */
> +	if (sdev->lun == 0)
> +		dev->sdev = sdev;
>  	return 0;
>  }
>  
> @@ -1220,7 +1228,12 @@ void ata_scsi_sdev_destroy(struct scsi_device *sdev)
>  
>  	spin_lock_irqsave(ap->lock, flags);
>  	dev = __ata_scsi_find_dev(ap, sdev);
> -	if (dev && dev->sdev) {
> +	/*
> +	 * Only detach when the canonical (LUN 0) scsi_device is going away.
> +	 * Removing a non-LUN-0 sdev (e.g. a spurious multi-LUN scan result)
> +	 * must not tear down the underlying ATA device.
> +	 */
> +	if (dev && dev->sdev == sdev) {
>  		/* SCSI device already in CANCEL state, no need to offline it */
>  		dev->sdev = NULL;
>  		dev->flags |= ATA_DFLAG_DETACH;
> @@ -2950,6 +2963,15 @@ static unsigned int atapi_xlat(struct ata_queued_cmd *qc)
>  	memset(qc->cdb, 0, dev->cdb_len);
>  	memcpy(qc->cdb, scmd->cmnd, scmd->cmd_len);
>  
> +	/*
> +	 * Encode LUN in CDB byte 1 bits 7:5 for multi-LUN ATAPI devices
> +	 * that use the SCSI-2 CDB LUN convention (e.g. Panasonic PD/CD
> +	 * combo drives).
> +	 */
> +	if (scmd->device->lun)
> +		qc->cdb[1] = (qc->cdb[1] & 0x1f) |
> +			      ((scmd->device->lun & 0x7) << 5);

Splitting the line after the "=" should look nicer.

> +
>  	qc->complete_fn = atapi_qc_complete;
>  
>  	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> @@ -3062,9 +3084,17 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
>  
>  	/* skip commands not addressed to targets we simulate */
>  	if (!sata_pmp_attached(ap)) {
> -		if (unlikely(scsidev->channel || scsidev->lun))
> +		if (unlikely(scsidev->channel))
>  			return NULL;
>  		devno = scsidev->id;
> +		/* Allow non-zero LUNs for ATAPI devices (e.g. PD/CD combos) */
> +		if (unlikely(scsidev->lun)) {
> +			struct ata_device *dev = ata_find_dev(ap, devno);
> +
> +			if (!dev || dev->class != ATA_DEV_ATAPI)
> +				return NULL;
> +			return dev;
> +		}

Hmmm... What if the multi-LUN ATAPI device is attached to a port multiplier ?

>  	} else {
>  		if (unlikely(scsidev->id || scsidev->lun))
>  			return NULL;
> @@ -4620,7 +4650,7 @@ int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *s
>  		shost->transportt = ata_scsi_transport_template;
>  		shost->unique_id = ap->print_id;
>  		shost->max_id = 16;
> -		shost->max_lun = 1;
> +		shost->max_lun = 8;
>  		shost->max_channel = 1;
>  		shost->max_cmd_len = 32;
>  


-- 
Damien Le Moal
Western Digital Research