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

[Damien Le Moal]

On 2026/05/12 22:27, 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.  Hoist a non-zero LUN early-exit ahead of the original
>    channel/id checks: when scsidev->lun is non-zero, allow it through
>    only if the underlying ata_device is ATAPI class.  The original
>    LUN-0 path is left structurally unchanged.
> 
> 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.
> 
>    The SCSI layer caps the LUN at shost->max_lun, so a value beyond
>    the device's nr_luns should never reach this point; guard with
>    WARN_ON_ONCE() and return AC_ERR_INVALID if it does, since the
>    3-bit CDB field cannot represent it.
> 
> Signed-off-by: Phil Pemberton <philpem@xxxxxxxxxxxxx>
> ---
>  drivers/ata/libata-scsi.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 7c3d31dc49a1..2d714efc855f 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2953,6 +2953,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);
>  
> +	/*
> +	 * SCSI-2 CDB LUN encoding: bits 7:5 of byte 1 (3-bit field).
> +	 * The SCSI layer caps the LUN at shost->max_lun (<= ATAPI_MAX_LUN),
> +	 * so this should never trip; warn and reject if it does.
> +	 */
> +	if (WARN_ON_ONCE(scmd->device->lun >= dev->nr_luns))
> +		return AC_ERR_INVALID;
> +	qc->cdb[1] = (qc->cdb[1] & 0x1f) | ((u8)scmd->device->lun << 5);

Let's change this as follows:

	qc->cdb[1] = qc->cdb[1] & 0x1f;
	if (unlikely(scmd->device->lun)) {
		if (WARN_ON_ONCE(scmd->device->lun >= dev->nr_luns))
			return AC_ERR_INVALID;
		qc->cdb[1] |= (u8)scmd->device->lun << 5;
	}




-- 
Damien Le Moal
Western Digital Research