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

Damien Le Moal
2026-04-12T07:38:16Z

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@philpem.me.uk>
> ---
> 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