Re: [PATCH v3 3/7] ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI

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

On 4/27/26 4:09 AM, Phil Pemberton wrote:
> Two changes are required to route commands to ATAPI LUNs other than 0:
> 
> 1. __ata_scsi_find_dev():  The existing code rejects any scsi_device
>    with a non-zero LUN, returning NULL and dropping the command on
>    the floor.  Relax both the PMP and non-PMP branches to allow
>    non-zero LUNs through when the underlying ata_device is ATAPI
>    class, since ATAPI devices can legitimately expose multiple LUNs.
> 
> 2. atapi_xlat():  Older ATAPI devices (SCSI-2 era) expect the LUN in
>    CDB byte 1 bits 7:5 rather than relying on transport-level LUN
>    addressing.  Encode scmd->device->lun into those bits, preserving
>    the existing command-specific bits in 4:0.  This is required by
>    both the Panasonic PD/CD combos and Nakamichi CD changers.  LUNs
>    beyond 7 cannot be encoded in the 3-bit CDB field; reject them
>    with AC_ERR_INVALID.
> 
> Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
> ---
>  drivers/ata/libata-scsi.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 317883bac25f..48c7d323d6f9 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2951,6 +2951,11 @@ 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);
>  
> +	/* SCSI-2 CDB LUN encoding: bits 7:5 of byte 1 (3-bit field) */
> +	if (scmd->device->lun >= 8)

Instead of 8, please use ATAPI_MAX_LUN.
Since this should never happen, this also warrants the use of WARN_ON_ONCE()
for the if condition.

> +		return AC_ERR_INVALID;
> +	qc->cdb[1] = (qc->cdb[1] & 0x1f) | ((u8)scmd->device->lun << 5);
> +
>  	qc->complete_fn = atapi_qc_complete;
>  
>  	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> @@ -3059,19 +3064,27 @@ static struct ata_device *ata_find_dev(struct ata_port *ap, unsigned int devno)
>  static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
>  					      const struct scsi_device *scsidev)
>  {
> +	struct ata_device *dev;
>  	int devno;
>  
>  	/* 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;
>  	} else {
> -		if (unlikely(scsidev->id || scsidev->lun))
> +		if (unlikely(scsidev->id))
>  			return NULL;
>  		devno = scsidev->channel;
>  	}
>  
> +	if (unlikely(scsidev->lun)) {
> +		dev = ata_find_dev(ap, devno);
> +		if (!dev || dev->class != ATA_DEV_ATAPI)
> +			return NULL;
> +		return dev;
> +	}

This really should come first in the function. Otherwise, you are radically
changing the function since you removed the scsidev->lun checks for the
preceding checks.

> +
>  	return ata_find_dev(ap, devno);
>  }
>  


-- 
Damien Le Moal
Western Digital Research