[PATCH v8 0/6] libata-scsi: multi-LUN ATAPI device support

[PATCH v8 0/6] libata-scsi: multi-LUN ATAPI device support

[Phil Pemberton]

Some ATAPI devices expose more than one logical unit behind a single ATA
target: Panasonic/COMPAQ PD/CD combo drives (LUN 0 = CD-ROM, LUN 1 =
PD), and Nakamichi CD changers (one LUN per disc slot, up to 7).

libata has historically hard-coded shost->max_lun = 1, so the SCSI
layer never scans past LUN 0 on any ATA-attached device.  This series
lifts that restriction for ATAPI devices gated by BLIST_FORCELUN.

Resend at Damien's request.

Changes since v7
================

No code changes.  v8 is v7 rebased onto libata/for-next
(c303c3619a1d) and with previously-given review tags collected.  The
rebase was clean; the per-patch diffs are byte-identical to v7.

Review tags collected
=====================

Several tags given on earlier postings were not carried forward into
v7.  They are restored here, along with the tags given on v7 itself:

  1/6  Reviewed-by: Damien Le Moal        (given on v5 1/6)
  2/6  Reviewed-by: Hannes Reinecke       (given on v7 2/6)
  4/6  Reviewed-by: Martin K. Petersen    (given on v6 4/6)
  6/6  Reviewed-by: Damien Le Moal        (given on v3 6/7)
       Reviewed-by: Hannes Reinecke       (given on v7 6/6)
       Reviewed-by: Martin K. Petersen    (given on v6 6/6)

Patches 3/6 and 5/6 carry the tags Hannes gave on v6; both patches
changed between v6 and v7 in response to his own review comments, and
he did not object when v7 was posted.  Hannes's tag on 1/6 is from v2,
where he signed as <hare@suse.de>; his later tags use <hare@kernel.org>,
and each is transcribed as given.

Damien reviewed both 4/6 and 6/6 on v3, but only the 6/6 tag is carried
here: 6/6 is a one-line scsi_devinfo table entry that has not changed
materially since, whereas 4/6 was reworked afterwards (the
pdt_1f_for_no_lun assignment moved from scsi_add_lun() to
scsi_probe_and_add_lun()), so carrying that one seemed wrong.

Series structure
================

  1/6  ata: libata-scsi: add atapi_max_lun module parameter
  2/6  ata: libata-scsi: convert dev->sdev to per-LUN array
  3/6  ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI
  4/6  scsi: add BLIST_NO_LUN_1F blacklist flag
  5/6  ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices
  6/6  scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

Testing
=======

Hardware testing was done on the v7 code (Panasonic/COMPAQ LF-1195C on
Intel ICH5 PATA); since v8 is the same code rebased, those results
carry over:

  [x] Boot with CD inserted: sr0 attaches, mount and read files
  [x] Boot with PD inserted: sda attaches at correct capacity
      (1298496 x 512 B = 634 MiB)
  [x] All seven LUNs scanned (atapi_max_lun=7); LUNs 2..6 correctly
      report PDT 0x1f and are silently skipped
  [x] Single-LUN ATAPI CD-ROM (LITE-ON iHAS124): no regression,
      only LUN 0 scanned

On the new base, each patch has been compile-tested individually so
the series stays bisectable.

Known limitations
=================

Media-change events are not propagated across LUNs of a SINGLELUN
multi-LUN device.  The SCSI layer's UA handling is per-sdev.  On the
PD/CD combo, swapping media and then accessing the other LUN may return
stale capacity until a manual rescan:

  echo 1 > /sys/class/scsi_device/H:0:0:1/device/rescan

A follow-up series addressing this (sibling-LUN media-change
propagation, gated on a new BLIST flag) is in preparation and will be
posted separately once this series lands.  It is kept out of this
series to avoid gating the LUN-scanning core on a new blacklist flag
and a retry policy that will need their own review.

Phil Pemberton (6):
  ata: libata-scsi: add atapi_max_lun module parameter
  ata: libata-scsi: convert dev->sdev to per-LUN array
  ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI
  scsi: add BLIST_NO_LUN_1F blacklist flag
  ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices
  scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

 drivers/ata/libata-acpi.c   |   9 +-
 drivers/ata/libata-core.c   |  16 ++-
 drivers/ata/libata-scsi.c   | 226 ++++++++++++++++++++++++------------
 drivers/ata/libata-zpodd.c  |  27 ++++-
 drivers/ata/libata.h        |   1 +
 drivers/scsi/scsi_devinfo.c |   2 +
 drivers/scsi/scsi_scan.c    |   3 +
 include/linux/libata.h      |  11 +-
 include/scsi/scsi_devinfo.h |   6 +-
 9 files changed, 210 insertions(+), 91 deletions(-)


base-commit: c303c3619a1d5cf7d4b457106062d16724724a80
-- 
2.43.0

[PATCH v8 1/6] ata: libata-scsi: add atapi_max_lun module parameter

[Phil Pemberton]

Until now libata has hard-coded shost->max_lun = 1 for every ATA host,
so the SCSI layer never scans past LUN 0.  This blocks support for
the small handful of multi-LUN ATAPI devices (Panasonic LF-1195C and
COMPAQ PD-1 PD/CD combos export CD on LUN 0 and PD on LUN 1; old
Nakamichi MJ-x.y CD changers expose one LUN per disc slot, up to 7).

Introduce a libata module parameter, atapi_max_lun, that controls the
upper bound of the per-host SCSI LUN scan.  Default is 1, preserving
current behaviour exactly: out-of-the-box only LUN 0 is scanned.
Range is clamped to 1..ATAPI_MAX_LUN (8, the SCSI-2 ceiling, covering
LUN values 0..7).

Subsequent patches gate actual LUN>0 probing on BLIST_FORCELUN, so a
device must both be on the SCSI device list (or carry the appropriate
quirk) and run on a host whose atapi_max_lun has been raised before
any extra LUNs are scanned.

Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
---
 drivers/ata/libata-core.c | 5 +++++
 drivers/ata/libata-scsi.c | 2 +-
 drivers/ata/libata.h      | 1 +
 include/linux/libata.h    | 1 +
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d893c916df0b..15ee44cf5bf2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -122,6 +122,11 @@ int atapi_passthru16 = 1;
 module_param(atapi_passthru16, int, 0444);
 MODULE_PARM_DESC(atapi_passthru16, "Enable ATA_16 passthru for ATAPI devices (0=off, 1=on [default])");
 
+int atapi_max_lun = 1;
+module_param(atapi_max_lun, int, 0444);
+MODULE_PARM_DESC(atapi_max_lun,
+	"Number of LUNs to scan on ATAPI devices flagged BLIST_FORCELUN (1 [default] = LUN 0 only, 8 = all SCSI-2 LUNs 0..7)");
+
 int libata_fua = 0;
 module_param_named(fua, libata_fua, int, 0444);
 MODULE_PARM_DESC(fua, "FUA support (0=off [default], 1=on)");
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 1d225ee9eb86..04c96f3fd865 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -5154,7 +5154,7 @@ int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *s
 		shost->transportt = &ata_scsi_transportt;
 		shost->unique_id = ap->print_id;
 		shost->max_id = 16;
-		shost->max_lun = 1;
+		shost->max_lun = clamp(atapi_max_lun, 1, ATAPI_MAX_LUN);
 		shost->max_channel = 1;
 		shost->max_cmd_len = 32;
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 39494ab206a2..7ee3bf1a4789 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -33,6 +33,7 @@ enum {
 #define ATA_PORT_TYPE_NAME	"ata_port"
 
 extern int atapi_passthru16;
+extern int atapi_max_lun;
 extern int libata_fua;
 extern int libata_noacpi;
 extern int libata_allow_tpm;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 18edb36c29fc..25e4b671adfb 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -180,6 +180,7 @@ enum {
 	ATA_SHORT_PAUSE		= 16,
 
 	ATAPI_MAX_DRAIN		= 16 << 10,
+	ATAPI_MAX_LUN		= 8,	/* SCSI-2 cap (LUN values 0..7) */
 
 	ATA_ALL_DEVICES		= (1 << ATA_MAX_DEVICES) - 1,
 
-- 
2.43.0

[PATCH v8 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array

[Phil Pemberton]

Multi-LUN ATAPI devices (PD/CD combos, CD changers) share a single
ata_device but expose multiple scsi_devices.  The previous single
dev->sdev pointer could only track one LUN, making all other LUNs
invisible to code that operates on sdevs: port detach, suspend/resume,
ACPI uevent, ZPODD, media change notification, and EH teardown.

Replace the scalar struct scsi_device *sdev with a fixed-size array
dev->sdev[ATAPI_MAX_LUN] indexed by LUN number, where ATAPI_MAX_LUN
is 8 (the SCSI-2 ceiling, LUN values 0..7).  All callers are updated
to iterate the full array and skip NULL slots; only populated LUN slots
are ever non-NULL so single-LUN devices (the vast majority) see no
change in behaviour.

Add an inline helper ata_dev_scsi_device(dev, lun) that returns
dev->sdev[lun] guarded by a WARN_ON_ONCE(lun >= ATAPI_MAX_LUN) bounds
check.  Use it for the hardcoded LUN-0 references in libata-acpi
(uevent kobj), libata-zpodd (disk events, wake notify for all LUNs),
and the door-lock and OF-node paths in libata-scsi.

Key changes per call site:
  - ata_scsi_dev_config:    bounds-check lun, assign sdev to dev->sdev[sdev->lun]
  - ata_scsi_sdev_destroy:  clear per-LUN slot; trigger ATA detach only
                            when the last populated LUN is destroyed
  - ata_port_detach:        iterate all ATAPI_MAX_LUN slots descending;
                            clear dev->sdev[lun] before unlock to close
                            the UAF window (Hannes Reinecke)
  - ata_scsi_offline_dev:   iterate all slots
  - ata_scsi_remove_dev:    snapshot all LUN slots then remove outside lock
  - ata_scsi_media_change_notify: send event to all populated LUNs
  - ata_scsi_dev_rescan:    snapshot all LUNs under lock, then resume and
                            rescan each; release remaining refs on early exit
  - ACPI, ZPODD, door-lock: use ata_dev_scsi_device(dev, 0)
  - ZPODD disk-events:      iterate all LUNs for enable/disable and wake

Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
---
 drivers/ata/libata-acpi.c  |   9 +-
 drivers/ata/libata-core.c  |  11 ++-
 drivers/ata/libata-scsi.c  | 164 +++++++++++++++++++++----------------
 drivers/ata/libata-zpodd.c |  27 ++++--
 include/linux/libata.h     |  10 ++-
 5 files changed, 138 insertions(+), 83 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index 4433f626246b..2d1662f6f064 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -153,10 +153,13 @@ static void ata_acpi_uevent(struct ata_port *ap, struct ata_device *dev,
 	char *envp[] = { event_string, NULL };
 
 	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
+		struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
+
+		if (sdev)
+			kobj = &sdev->sdev_gendev.kobj;
+	} else {
 		kobj = &ap->dev->kobj;
+	}
 
 	if (kobj) {
 		snprintf(event_string, 20, "BAY_EVENT=%d", event);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 15ee44cf5bf2..43d5221dc347 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6381,11 +6381,16 @@ static void ata_port_detach(struct ata_port *ap)
 	/* Remove scsi devices */
 	ata_for_each_link(link, ap, HOST_FIRST) {
 		ata_for_each_dev(dev, link, ALL) {
-			if (dev->sdev) {
+			int lun;
+
+			for (lun = ATAPI_MAX_LUN - 1; lun >= 0; lun--) {
+				struct scsi_device *sdev = dev->sdev[lun];
+				if (!sdev)
+					continue;
+				dev->sdev[lun] = NULL;
 				spin_unlock_irqrestore(ap->lock, flags);
-				scsi_remove_device(dev->sdev);
+				scsi_remove_device(sdev);
 				spin_lock_irqsave(ap->lock, flags);
-				dev->sdev = NULL;
 			}
 		}
 	}
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 04c96f3fd865..808368b952b5 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1129,7 +1129,9 @@ 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;
+	if (WARN_ON_ONCE(sdev->lun >= ATAPI_MAX_LUN))
+		return -EINVAL;
+	dev->sdev[sdev->lun] = sdev;
 	return 0;
 }
 
@@ -1200,10 +1202,10 @@ EXPORT_SYMBOL_GPL(ata_scsi_sdev_configure);
  *
  *	@sdev is about to be destroyed for hot/warm unplugging.  If
  *	this unplugging was initiated by libata as indicated by NULL
- *	dev->sdev, this function doesn't have to do anything.
+ *	dev->sdev[], this function doesn't have to do anything.
  *	Otherwise, SCSI layer initiated warm-unplug is in progress.
- *	Clear dev->sdev, schedule the device for ATA detach and invoke
- *	EH.
+ *	Clear the per-LUN slot; when the last LUN (LUN 0) is destroyed,
+ *	schedule ATA-level detach via EH.
  *
  *	LOCKING:
  *	Defined by SCSI layer.  We don't really care.
@@ -1218,11 +1220,23 @@ 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) {
-		/* SCSI device already in CANCEL state, no need to offline it */
-		dev->sdev = NULL;
-		dev->flags |= ATA_DFLAG_DETACH;
-		ata_port_schedule_eh(ap);
+	if (dev && !WARN_ON_ONCE(sdev->lun >= ATAPI_MAX_LUN) &&
+	    dev->sdev[sdev->lun] == sdev) {
+		int lun;
+		bool last;
+
+		dev->sdev[sdev->lun] = NULL;
+		last = true;
+		for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
+			if (dev->sdev[lun]) {
+				last = false;
+				break;
+			}
+		}
+		if (last) {
+			dev->flags |= ATA_DFLAG_DETACH;
+			ata_port_schedule_eh(ap);
+		}
 	}
 	spin_unlock_irqrestore(ap->lock, flags);
 
@@ -2963,12 +2977,9 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 		 *
 		 * If door lock fails, always clear sdev->locked to
 		 * avoid this infinite loop.
-		 *
-		 * This may happen before SCSI scan is complete.  Make
-		 * sure qc->dev->sdev isn't NULL before dereferencing.
 		 */
-		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL && qc->dev->sdev)
-			qc->dev->sdev->locked = 0;
+		if (qc->cdb[0] == ALLOW_MEDIUM_REMOVAL)
+			qc->scsicmd->device->locked = 0;
 
 		ata_scsi_qc_done(qc, true, SAM_STAT_CHECK_CONDITION);
 		return;
@@ -5185,7 +5196,7 @@ int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *s
 #ifdef CONFIG_OF
 static void ata_scsi_assign_ofnode(struct ata_device *dev, struct ata_port *ap)
 {
-	struct scsi_device *sdev = dev->sdev;
+	struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
 	struct device *d = ap->host->dev;
 	struct device_node *np = d->of_node;
 	struct device_node *child;
@@ -5223,7 +5234,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			struct scsi_device *sdev;
 			int channel = 0, id = 0;
 
-			if (dev->sdev)
+			if (dev->sdev[0])
 				continue;
 
 			if (ata_is_host_link(link))
@@ -5234,11 +5245,11 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
 						 NULL);
 			if (!IS_ERR(sdev)) {
-				dev->sdev = sdev;
+				dev->sdev[0] = sdev;
 				ata_scsi_assign_ofnode(dev, ap);
 				scsi_device_put(sdev);
 			} else {
-				dev->sdev = NULL;
+				dev->sdev[0] = NULL;
 			}
 		}
 	}
@@ -5249,7 +5260,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 	 */
 	ata_for_each_link(link, ap, EDGE) {
 		ata_for_each_dev(dev, link, ENABLED) {
-			if (!dev->sdev)
+			if (!dev->sdev[0])
 				goto exit_loop;
 		}
 	}
@@ -5290,7 +5301,7 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
  *
  *	This function is called from ata_eh_detach_dev() and is responsible for
  *	taking the SCSI device attached to @dev offline.  This function is
- *	called with host lock which protects dev->sdev against clearing.
+ *	called with host lock which protects dev->sdev[] against clearing.
  *
  *	LOCKING:
  *	spin_lock_irqsave(host lock)
@@ -5300,11 +5311,16 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
  */
 bool ata_scsi_offline_dev(struct ata_device *dev)
 {
-	if (dev->sdev) {
-		scsi_device_set_state(dev->sdev, SDEV_OFFLINE);
-		return true;
+	bool found = false;
+	int lun;
+
+	for (lun = ATAPI_MAX_LUN - 1; lun >= 0; lun--) {
+		if (dev->sdev[lun]) {
+			scsi_device_set_state(dev->sdev[lun], SDEV_OFFLINE);
+			found = true;
+		}
 	}
-	return false;
+	return found;
 }
 
 /**
@@ -5320,49 +5336,38 @@ bool ata_scsi_offline_dev(struct ata_device *dev)
 static void ata_scsi_remove_dev(struct ata_device *dev)
 {
 	struct ata_port *ap = dev->link->ap;
-	struct scsi_device *sdev;
+	struct scsi_device *sdevs[ATAPI_MAX_LUN] = {};
 	unsigned long flags;
+	int lun;
 
-	/* Alas, we need to grab scan_mutex to ensure SCSI device
-	 * state doesn't change underneath us and thus
-	 * scsi_device_get() always succeeds.  The mutex locking can
-	 * be removed if there is __scsi_device_get() interface which
-	 * increments reference counts regardless of device state.
-	 */
 	mutex_lock(&ap->scsi_host->scan_mutex);
 	spin_lock_irqsave(ap->lock, flags);
 
-	/* clearing dev->sdev is protected by host lock */
-	sdev = dev->sdev;
-	dev->sdev = NULL;
+	for (lun = ATAPI_MAX_LUN - 1; lun >= 0; lun--) {
+		struct scsi_device *sdev = dev->sdev[lun];
+
+		dev->sdev[lun] = NULL;
+		if (!sdev)
+			continue;
 
-	if (sdev) {
-		/* If user initiated unplug races with us, sdev can go
-		 * away underneath us after the host lock and
-		 * scan_mutex are released.  Hold onto it.
-		 */
 		if (scsi_device_get(sdev) == 0) {
-			/* The following ensures the attached sdev is
-			 * offline on return from ata_scsi_offline_dev()
-			 * regardless it wins or loses the race
-			 * against this function.
-			 */
 			scsi_device_set_state(sdev, SDEV_OFFLINE);
+			sdevs[lun] = sdev;
 		} else {
 			WARN_ON(1);
-			sdev = NULL;
 		}
 	}
 
 	spin_unlock_irqrestore(ap->lock, flags);
 	mutex_unlock(&ap->scsi_host->scan_mutex);
 
-	if (sdev) {
+	for (lun = ATAPI_MAX_LUN - 1; lun >= 0; lun--) {
+		if (!sdevs[lun])
+			continue;
 		ata_dev_info(dev, "detaching (SCSI %s)\n",
-			     dev_name(&sdev->sdev_gendev));
-
-		scsi_remove_device(sdev);
-		scsi_device_put(sdev);
+			     dev_name(&sdevs[lun]->sdev_gendev));
+		scsi_remove_device(sdevs[lun]);
+		scsi_device_put(sdevs[lun]);
 	}
 }
 
@@ -5399,9 +5404,12 @@ static void ata_scsi_handle_link_detach(struct ata_link *link)
  */
 void ata_scsi_media_change_notify(struct ata_device *dev)
 {
-	if (dev->sdev)
-		sdev_evt_send_simple(dev->sdev, SDEV_EVT_MEDIA_CHANGE,
-				     GFP_ATOMIC);
+	int lun;
+
+	for (lun = 0; lun < ATAPI_MAX_LUN; lun++)
+		if (dev->sdev[lun])
+			sdev_evt_send_simple(dev->sdev[lun],
+					     SDEV_EVT_MEDIA_CHANGE, GFP_ATOMIC);
 }
 
 /**
@@ -5534,7 +5542,8 @@ void ata_scsi_dev_rescan(struct work_struct *work)
 
 	ata_for_each_link(link, ap, EDGE) {
 		ata_for_each_dev(dev, link, ENABLED) {
-			struct scsi_device *sdev = dev->sdev;
+			struct scsi_device *sdevs[ATAPI_MAX_LUN] = {};
+			int lun;
 
 			/*
 			 * If the port was suspended before this was scheduled,
@@ -5543,28 +5552,43 @@ void ata_scsi_dev_rescan(struct work_struct *work)
 			if (ap->pflags & ATA_PFLAG_SUSPENDED)
 				goto unlock_ap;
 
-			if (!sdev)
-				continue;
-			if (scsi_device_get(sdev))
-				continue;
+			for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
+				if (dev->sdev[lun] &&
+				    !scsi_device_get(dev->sdev[lun]))
+					sdevs[lun] = dev->sdev[lun];
+			}
 
 			do_resume = dev->flags & ATA_DFLAG_RESUMING;
 
-			spin_unlock_irqrestore(ap->lock, flags);
-			if (do_resume) {
-				ret = scsi_resume_device(sdev);
-				if (ret == -EWOULDBLOCK) {
-					scsi_device_put(sdev);
-					goto unlock_scan;
+			for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
+				if (!sdevs[lun])
+					continue;
+
+				spin_unlock_irqrestore(ap->lock, flags);
+				if (do_resume) {
+					ret = scsi_resume_device(sdevs[lun]);
+					if (ret == -EWOULDBLOCK) {
+						scsi_device_put(sdevs[lun]);
+						while (++lun < ATAPI_MAX_LUN)
+							if (sdevs[lun])
+								scsi_device_put(sdevs[lun]);
+						goto unlock_scan;
+					}
+				}
+				ret = scsi_rescan_device(sdevs[lun]);
+				scsi_device_put(sdevs[lun]);
+				spin_lock_irqsave(ap->lock, flags);
+
+				if (ret) {
+					while (++lun < ATAPI_MAX_LUN)
+						if (sdevs[lun])
+							scsi_device_put(sdevs[lun]);
+					goto unlock_ap;
 				}
-				dev->flags &= ~ATA_DFLAG_RESUMING;
 			}
-			ret = scsi_rescan_device(sdev);
-			scsi_device_put(sdev);
-			spin_lock_irqsave(ap->lock, flags);
 
-			if (ret)
-				goto unlock_ap;
+			if (do_resume)
+				dev->flags &= ~ATA_DFLAG_RESUMING;
 		}
 	}
 
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
index 414e7c63bd85..151ae5726aca 100644
--- a/drivers/ata/libata-zpodd.c
+++ b/drivers/ata/libata-zpodd.c
@@ -184,8 +184,13 @@ bool zpodd_zpready(struct ata_device *dev)
 void zpodd_enable_run_wake(struct ata_device *dev)
 {
 	struct zpodd *zpodd = dev->zpodd;
+	int lun;
 
-	sdev_disable_disk_events(dev->sdev);
+	for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
+		struct scsi_device *sdev = dev->sdev[lun];
+		if (sdev)
+			sdev_disable_disk_events(sdev);
+	}
 
 	zpodd->powered_off = true;
 	acpi_pm_set_device_wakeup(&dev->tdev, true);
@@ -218,6 +223,7 @@ void zpodd_disable_run_wake(struct ata_device *dev)
 void zpodd_post_poweron(struct ata_device *dev)
 {
 	struct zpodd *zpodd = dev->zpodd;
+	int lun;
 
 	if (!zpodd->powered_off)
 		return;
@@ -233,18 +239,27 @@ void zpodd_post_poweron(struct ata_device *dev)
 	zpodd->zp_sampled = false;
 	zpodd->zp_ready = false;
 
-	sdev_enable_disk_events(dev->sdev);
+	for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
+		struct scsi_device *sdev = dev->sdev[lun];
+		if (sdev)
+			sdev_enable_disk_events(sdev);
+	}
 }
 
 static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context)
 {
 	struct ata_device *ata_dev = context;
 	struct zpodd *zpodd = ata_dev->zpodd;
-	struct device *dev = &ata_dev->sdev->sdev_gendev;
+	int lun;
 
-	if (event == ACPI_NOTIFY_DEVICE_WAKE && pm_runtime_suspended(dev)) {
-		zpodd->from_notify = true;
-		pm_runtime_resume(dev);
+	if (event != ACPI_NOTIFY_DEVICE_WAKE)
+		return;
+	for (lun = 0; lun < ATAPI_MAX_LUN; lun++) {
+		struct scsi_device *sdev = ata_dev->sdev[lun];
+		if (sdev && pm_runtime_suspended(&sdev->sdev_gendev)) {
+			zpodd->from_notify = true;
+			pm_runtime_resume(&sdev->sdev_gendev);
+		}
 	}
 }
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 25e4b671adfb..3b9207a9f334 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -733,7 +733,7 @@ struct ata_device {
 	unsigned int		devno;		/* 0 or 1 */
 	u64			quirks;		/* List of broken features */
 	unsigned long		flags;		/* ATA_DFLAG_xxx */
-	struct scsi_device	*sdev;		/* attached SCSI device */
+	struct scsi_device	*sdev[ATAPI_MAX_LUN];	/* per-LUN SCSI devices */
 	void			*private_data;
 #ifdef CONFIG_ATA_ACPI
 	union acpi_object	*gtf_cache;
@@ -1726,6 +1726,14 @@ static inline unsigned int ata_dev_absent(const struct ata_device *dev)
 	return ata_class_absent(dev->class);
 }
 
+static inline struct scsi_device *
+ata_dev_scsi_device(struct ata_device *dev, unsigned int lun)
+{
+	if (WARN_ON_ONCE(lun >= ATAPI_MAX_LUN))
+		return NULL;
+	return dev->sdev[lun];
+}
+
 /*
  * link helpers
  */
-- 
2.43.0

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

[Phil Pemberton]

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.  Always clear those bits first, then encode
   scmd->device->lun into them for non-zero LUNs.  This is required by
   both the Panasonic PD/CD combos and Nakamichi CD changers.

   Guard with WARN_ON_ONCE() and fail the command (setting scmd->result
   to DID_ERROR) if the LUN is out of range, since the 3-bit CDB field
   cannot represent it.

Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
---
 drivers/ata/libata-scsi.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 808368b952b5..0b1e4842860c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3012,6 +3012,20 @@ 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).
+	 * Always clear those bits; only set them for non-zero LUNs.
+	 */
+	qc->cdb[1] = qc->cdb[1] & 0x1f;
+	if (unlikely(scmd->device->lun)) {
+		if (WARN_ON_ONCE(scmd->device->host->max_lun > ATAPI_MAX_LUN ||
+				 scmd->device->lun >= scmd->device->host->max_lun)) {
+			scmd->result = DID_ERROR << 16;
+			return 1;
+		}
+		qc->cdb[1] |= (u8)scmd->device->lun << 5;
+	}
+
 	qc->complete_fn = atapi_qc_complete;
 
 	qc->tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
@@ -3122,6 +3136,29 @@ static struct ata_device *__ata_scsi_find_dev(struct ata_port *ap,
 {
 	int devno;
 
+	/*
+	 * Non-zero LUN is only legal for ATAPI devices, since they can
+	 * legitimately expose more than one LUN (PD/CD combos, CD changers).
+	 * Handle that case up front so the LUN-0 path below stays unchanged.
+	 */
+	if (unlikely(scsidev->lun)) {
+		struct ata_device *dev;
+
+		if (!sata_pmp_attached(ap)) {
+			if (unlikely(scsidev->channel))
+				return NULL;
+			devno = scsidev->id;
+		} else {
+			if (unlikely(scsidev->id))
+				return NULL;
+			devno = scsidev->channel;
+		}
+		dev = ata_find_dev(ap, devno);
+		if (!dev || dev->class != ATA_DEV_ATAPI)
+			return NULL;
+		return dev;
+	}
+
 	/* skip commands not addressed to targets we simulate */
 	if (!sata_pmp_attached(ap)) {
 		if (unlikely(scsidev->channel || scsidev->lun))
-- 
2.43.0

[PATCH v8 4/6] scsi: add BLIST_NO_LUN_1F blacklist flag

[Phil Pemberton]

Some multi-LUN devices respond to INQUIRY on unpopulated LUNs with
PQ=0 / PDT=0x1f instead of the standard PQ=3.  The SCSI scan layer
normally adds such devices (PQ=0 means "connected"), producing
spurious "No Device" entries.

The scsi_target field pdt_1f_for_no_lun already exists to suppress
this, but was previously only set by the USB UFI driver.

Add BLIST_NO_LUN_1F so the flag can be set per-device from
scsi_devinfo, and wire it up in scsi_probe_and_add_lun() to set
starget->pdt_1f_for_no_lun from the blacklist flags.  This is placed
immediately before the PDT=0x1f check so it takes effect for all LUNs,
including LUN 0, without waiting for scsi_add_lun() to run.

Reviewed-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
---
 drivers/scsi/scsi_scan.c    | 3 +++
 include/scsi/scsi_devinfo.h | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e27da038603a..98bd4d49fd62 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1296,6 +1296,9 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 	 * PDT=00h Direct-access device (floppy)
 	 * PDT=1Fh none (no FDD connected to the requested logical unit)
 	 */
+	if (bflags & BLIST_NO_LUN_1F)
+		starget->pdt_1f_for_no_lun = 1;
+
 	if (((result[0] >> 5) == 1 || starget->pdt_1f_for_no_lun) &&
 	    (result[0] & 0x1f) == 0x1f &&
 	    !scsi_is_wlun(lun)) {
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 1d79a3b536ce..6957b0705510 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -34,7 +34,8 @@
 #define BLIST_NOSTARTONADD	((__force blist_flags_t)(1ULL << 12))
 /* do not ask for VPD page size first on some broken targets */
 #define BLIST_NO_VPD_SIZE	((__force blist_flags_t)(1ULL << 13))
-#define __BLIST_UNUSED_14	((__force blist_flags_t)(1ULL << 14))
+/* PDT 0x1f with PQ 0 means no LUN present (e.g. some ATAPI multi-LUN) */
+#define BLIST_NO_LUN_1F		((__force blist_flags_t)(1ULL << 14))
 #define __BLIST_UNUSED_15	((__force blist_flags_t)(1ULL << 15))
 #define __BLIST_UNUSED_16	((__force blist_flags_t)(1ULL << 16))
 /* try REPORT_LUNS even for SCSI-2 devs (if HBA supports more than 8 LUNs) */
@@ -77,8 +78,7 @@
 #define __BLIST_HIGH_UNUSED (~(__BLIST_LAST_USED | \
 			       (__force blist_flags_t) \
 			       ((__force __u64)__BLIST_LAST_USED - 1ULL)))
-#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_14 | \
-			     __BLIST_UNUSED_15 | \
+#define __BLIST_UNUSED_MASK (__BLIST_UNUSED_15 | \
 			     __BLIST_UNUSED_16 | \
 			     __BLIST_UNUSED_24 | \
 			     __BLIST_UNUSED_27 | \
-- 
2.43.0

[PATCH v8 5/6] ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

[Phil Pemberton]

After LUN 0 is added for an ATAPI device, check its BLIST_FORCELUN
flag.  If set, call scsi_scan_target() with SCAN_WILD_CARD to trigger
the SCSI layer's built-in sequential LUN scan for that target only.
This probes LUNs 1..shost->max_lun, driven by the libata atapi_max_lun
module parameter.

Devices without BLIST_FORCELUN (the vast majority of ATAPI devices)
are left with only LUN 0 -- no sequential scan is triggered, so
single-LUN devices like the iHAS124 DVD writer are completely
unaffected.

Non-responding LUNs (PQ=0/PDT=0x1f) are silently skipped by
scsi_probe_and_add_lun() when BLIST_NO_LUN_1F is set on the device
via scsi_devinfo.

Also fix a TOCTOU window: call ata_scsi_assign_ofnode() before
scsi_device_put() so the reference to dev->sdev[0] is held while
the OF node is assigned.

Reviewed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
---
 drivers/ata/libata-scsi.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0b1e4842860c..5bbb3169bea7 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -26,6 +26,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsi_transport.h>
+#include <scsi/scsi_devinfo.h>
 #include <linux/libata.h>
 #include <linux/hdreg.h>
 #include <linux/uaccess.h>
@@ -5281,13 +5282,27 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
 
 			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
 						 NULL);
-			if (!IS_ERR(sdev)) {
-				dev->sdev[0] = sdev;
-				ata_scsi_assign_ofnode(dev, ap);
-				scsi_device_put(sdev);
-			} else {
+			if (IS_ERR(sdev)) {
 				dev->sdev[0] = NULL;
+				continue;
 			}
+
+			/*
+			 * For multi-LUN ATAPI (BLIST_FORCELUN), trigger a
+			 * sequential scan for this target.  pdt_1f_for_no_lun,
+			 * set during LUN 0 configure, ensures non-responding
+			 * LUNs are silently skipped; dev->sdev[] is populated
+			 * by ata_scsi_dev_config() during the scan.
+			 */
+			if (dev->class == ATA_DEV_ATAPI &&
+			    sdev->sdev_bflags & BLIST_FORCELUN &&
+			    !WARN_ON_ONCE(ap->scsi_host->max_lun > ATAPI_MAX_LUN))
+				scsi_scan_target(&ap->scsi_host->shost_gendev,
+						 channel, id, SCAN_WILD_CARD,
+						 SCSI_SCAN_RESCAN);
+			if (dev->sdev[0])
+				ata_scsi_assign_ofnode(dev, ap);
+			scsi_device_put(sdev);
 		}
 	}
 
-- 
2.43.0

[PATCH v8 6/6] scsi: scsi_devinfo: add COMPAQ PD-1 multi-LUN ATAPI device quirk

[Phil Pemberton]

The Compaq PD-1 (and equivalent Panasonic LF-1195C) is a combination
PD/CD-ROM drive that exposes two LUNs: LUN 0 is the CD-ROM and LUN 1
is the PD (Phase-change rewritable) drive.

Add a scsi_devinfo entry with BLIST_FORCELUN to enable multi-LUN
scanning, BLIST_SINGLELUN to prevent issuing LUN-aware commands
simultaneously, and BLIST_NO_LUN_1F to suppress spurious "No Device"
entries for unpopulated LUNs (which respond with PQ=0/PDT=0x1f).

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Phil Pemberton <philpem@philpem.me.uk>
---
 drivers/scsi/scsi_devinfo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
index 15ffbe93ac72..b3fbfcd5dd93 100644
--- a/drivers/scsi/scsi_devinfo.c
+++ b/drivers/scsi/scsi_devinfo.c
@@ -150,6 +150,8 @@ static struct {
 	{"COMPAQ", "MSA1000", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
 	{"COMPAQ", "MSA1000 VOLUME", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
 	{"COMPAQ", "HSV110", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
+	{"COMPAQ", "PD-1", NULL, BLIST_FORCELUN | BLIST_SINGLELUN |
+				 BLIST_NO_LUN_1F},
 	{"DDN", "SAN DataDirector", "*", BLIST_SPARSELUN},
 	{"DEC", "HSG80", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
 	{"DELL", "PV660F", NULL, BLIST_SPARSELUN},
-- 
2.43.0

Re: [PATCH v8 4/6] scsi: add BLIST_NO_LUN_1F blacklist flag

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent modification of `struct scsi_target` bitfields introduces a data race, potentially causing updates to `pdt_1f_for_no_lun` or `expecting_lun_change` to be lost.
--

--- Patch [4]: [PATCH v8 4/6] scsi: add BLIST_NO_LUN_1F blacklist flag ---
commit 90f0f3c59ea5fbec1b6c95c81d334666e21cd3a9
Author: Phil Pemberton <philpem@philpem.me.uk>

scsi: add BLIST_NO_LUN_1F blacklist flag

This commit adds the BLIST_NO_LUN_1F blacklist flag to allow devices to
suppress spurious "No Device" entries for unpopulated LUNs returning
PQ=0/PDT=0x1f. The flag is wired up in scsi_probe_and_add_lun() to set
starget->pdt_1f_for_no_lun.

> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c

[ ... ]

> @@ -1296,6 +1296,9 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  	 * PDT=00h Direct-access device (floppy)
>  	 * PDT=1Fh none (no FDD connected to the requested logical unit)
>  	 */
> +	if (bflags & BLIST_NO_LUN_1F)
> +		starget->pdt_1f_for_no_lun = 1;
> +
[Severity: High]
This isn't a bug introduced by this patch, but could this bitfield assignment
introduce a data race with other bitfields in struct scsi_target?

Looking at include/scsi/scsi_device.h, pdt_1f_for_no_lun shares the same
memory word with other bitfields:

    unsigned int            single_lun:1;
    unsigned int            pdt_1f_for_no_lun:1;
    unsigned int            no_report_luns:1;
    unsigned int            expecting_lun_change:1;

If a multi-LUN device reports a LUN change (REPORT LUNS DATA HAS CHANGED) on
an active LUN, scsi_report_lun_change() will set expecting_lun_change = 1 in
softirq context.

Simultaneously, if the system handles the LUN change by rescanning the
target, scsi_probe_and_add_lun() executes in process context and sets
pdt_1f_for_no_lun = 1.

Because these contexts perform read-modify-write operations on the same
memory word without mutual exclusion (the softirq does not hold
shost->scan_mutex), one of the updates might be lost.

If expecting_lun_change is lost, the SCSI layer fails to handle a LUN change
UNIT ATTENTION correctly, which on removable devices triggers a spurious
media-change event (cmd->device->changed = 1 and ACTION_FAIL), failing
in-flight I/O. If pdt_1f_for_no_lun is lost, spurious LUNs may be added.

>  	if (((result[0] >> 5) == 1 || starget->pdt_1f_for_no_lun) &&
>  	    (result[0] & 0x1f) == 0x1f &&
>  	    !scsi_is_wlun(lun)) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731213503.2379771-1-philpem@philpem.me.uk?part=4

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Flawed LUN validation logic in atapi_xlat() incorrectly checks the host's max_lun instead of just the device limit, triggering a WARN_ON_ONCE and failing commands for ATAPI devices attached to SAS HBAs.
- [High] Unconditionally clearing bits 7:5 of CDB byte 1 in `atapi_xlat()` corrupts commands that repurpose these bits, such as SECURITY PROTOCOL IN/OUT.
--

--- Patch [3]: [PATCH v8 3/6] ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI ---
commit 3d48864344b5c5d234865e96735b1a7de55c13b5
Author: Phil Pemberton <philpem@philpem.me.uk>

ata: libata-scsi: route non-zero LUN commands for multi-LUN ATAPI

This commit modifies libata-scsi to support routing commands to non-zero
ATAPI LUNs. It updates __ata_scsi_find_dev to allow non-zero LUNs for
ATAPI devices and modifies atapi_xlat to encode the LUN into the SCSI-2
CDB for non-zero LUNs.

> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3012,6 +3012,20 @@ 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).
> +	 * Always clear those bits; only set them for non-zero LUNs.
> +	 */
> +	qc->cdb[1] = qc->cdb[1] & 0x1f;

[Severity: High]
Does unconditionally clearing bits 7:5 of CDB byte 1 corrupt commands that
repurpose these bits?

For example, in SECURITY PROTOCOL IN (0xA2) and OUT (0xB5) commands, byte 1
is used as the 8-bit Security Protocol field. Stripping the top 3 bits would
mutate protocol 0xEF into 0x0F, which could break features like Trusted
Computing (e.g., AACS on Blu-Ray, Opal) if a userspace application issues 
raw SG_IO commands to the ATAPI device.

> +	if (unlikely(scmd->device->lun)) {
> +		if (WARN_ON_ONCE(scmd->device->host->max_lun > ATAPI_MAX_LUN ||

[Severity: High]
Will this WARN_ON_ONCE incorrectly trigger and fail commands for ATAPI 
devices attached to SAS HBAs?

If a multi-LUN ATAPI device is attached to a host controller that advertises
a large max_lun (such as libsas drivers like hisi_sas which set
shost->max_lun = ~0), the condition checking if the host's max_lun is
greater than ATAPI_MAX_LUN evaluates to true.

This would unconditionally fail commands directed to non-zero LUNs with
DID_ERROR, breaking multi-LUN ATAPI support on modern HBAs.

> +				 scmd->device->lun >= scmd->device->host->max_lun)) {
> +			scmd->result = DID_ERROR << 16;
> +			return 1;
> +		}
> +		qc->cdb[1] |= (u8)scmd->device->lun << 5;
> +	}
> +
>  	qc->complete_fn = atapi_qc_complete;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731213503.2379771-1-philpem@philpem.me.uk?part=3

Re: [PATCH v8 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Sleeping in atomic context due to `scsi_device_put()` under spinlock in `ata_scsi_dev_rescan` error path

Pre-existing issues:
- [High] Double-free or reference count underflow of OF device node in `ata_scsi_assign_ofnode`
- [High] Use-After-Free in `ata_port_detach` due to missing reference count before `scsi_remove_device`
--

--- Patch [2]: [PATCH v8 2/6] ata: libata-scsi: convert dev->sdev to per-LUN array ---
commit 576f81319ae7ef4943e02f2cfa1da008a5f4256c
Author: Phil Pemberton <philpem@philpem.me.uk>
Subject: ata: libata-scsi: convert dev->sdev to per-LUN array

This commit replaces the scalar dev->sdev pointer with an array of
scsi_device pointers to support multi-LUN ATAPI devices. Code paths
are updated to iterate over all ATAPI_MAX_LUN slots instead of assuming
a single LUN.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 15ee44cf5bf23..43d5221dc347c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6381,11 +6381,16 @@ static void ata_port_detach(struct ata_port *ap)
>  	/* Remove scsi devices */
>  	ata_for_each_link(link, ap, HOST_FIRST) {
>  		ata_for_each_dev(dev, link, ALL) {
> -			if (dev->sdev) {
> +			int lun;
> +
> +			for (lun = ATAPI_MAX_LUN - 1; lun >= 0; lun--) {
> +				struct scsi_device *sdev = dev->sdev[lun];
> +				if (!sdev)
> +					continue;
> +				dev->sdev[lun] = NULL;
>  				spin_unlock_irqrestore(ap->lock, flags);
> -				scsi_remove_device(dev->sdev);
> +				scsi_remove_device(sdev);

[Severity: High]
This is a pre-existing issue, but does dropping the lock here without
holding a reference to sdev expose us to a use-after-free?

The commit message mentions that clearing the array slot closes the UAF
window, but if a concurrent sysfs removal drops the final reference
while the lock is released, could scsi_remove_device(sdev) end up
dereferencing freed memory?

Would it be safer to call scsi_device_get(sdev) before dropping the lock?

>  				spin_lock_irqsave(ap->lock, flags);
> -				dev->sdev = NULL;
>  			}
>  		}
>  	}

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 04c96f3fd8652..808368b952b51 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -5185,7 +5196,7 @@ int ata_scsi_add_hosts(struct ata_host *host, const struct scsi_host_template *s
>  #ifdef CONFIG_OF
>  static void ata_scsi_assign_ofnode(struct ata_device *dev, struct ata_port *ap)
>  {
> -	struct scsi_device *sdev = dev->sdev;
> +	struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
>  	struct device *d = ap->host->dev;
>  	struct device_node *np = d->of_node;
>  	struct device_node *child;

[Severity: High]
This isn't a bug introduced by this patch, but does
ata_scsi_assign_ofnode() cause a reference count underflow or
double-free on the device node?

If for_each_available_child_of_node() assigns the child pointer to both
dev->tdev.of_node and sdev->sdev_gendev.of_node, we have two pointers
sharing a single acquired reference. When both devices are destroyed,
they will each call of_node_put(), potentially leading to a double-free.

Should we add an of_node_get() for the second assignment?

[ ... ]
> @@ -5534,7 +5542,8 @@ void ata_scsi_dev_rescan(struct work_struct *work)
[ ... ]
>  				ret = scsi_rescan_device(sdevs[lun]);
>  				scsi_device_put(sdevs[lun]);
>  				spin_lock_irqsave(ap->lock, flags);
>  
> -			if (ret)
> -				goto unlock_ap;
> +				if (ret) {
> +					while (++lun < ATAPI_MAX_LUN)
> +						if (sdevs[lun])
> +							scsi_device_put(sdevs[lun]);
> +					goto unlock_ap;
>  				}

[Severity: High]
Can this call to scsi_device_put() sleep while holding the spinlock?

Since this block runs after spin_lock_irqsave(ap->lock, flags) is
called, interrupts are disabled. If a concurrent removal has dropped
other references, this scsi_device_put() call might drop the final
reference and invoke the release handler scsi_device_dev_release().

Since the release handler contains blocking calls like
cancel_work_sync(), could this cause a schedule-while-atomic BUG?

> -			ret = scsi_rescan_device(sdev);
> -			scsi_device_put(sdev);
> -			spin_lock_irqsave(ap->lock, flags);
>  
> -			if (ret)
> -				goto unlock_ap;
> +			if (do_resume)
> +				dev->flags &= ~ATA_DFLAG_RESUMING;
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731213503.2379771-1-philpem@philpem.me.uk?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition exists between checking `dev->sdev[0]` and dereferencing it inside `ata_scsi_assign_ofnode()`, potentially leading to a NULL pointer dereference.
--

--- Patch [5]: [PATCH v8 5/6] ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices ---
commit 26698e8a692fea36d598b32f39fd7513b22c4cf9
Author: Phil Pemberton <philpem@philpem.me.uk>

ata: libata-scsi: probe additional LUNs for multi-LUN ATAPI devices

This commit checks the BLIST_FORCELUN flag after LUN 0 is added for an ATAPI
device. If set, it triggers a sequential LUN scan for that target using
scsi_scan_target() to probe additional LUNs. It also moves the call to
ata_scsi_assign_ofnode() before scsi_device_put() to hold the device reference.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 0b1e4842860c1..5bbb3169bea70 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -5281,13 +5282,27 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>  
>  			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>  						 NULL);
> -			if (!IS_ERR(sdev)) {
> -				dev->sdev[0] = sdev;
> -				ata_scsi_assign_ofnode(dev, ap);
> -				scsi_device_put(sdev);
> -			} else {
> +			if (IS_ERR(sdev)) {
>  				dev->sdev[0] = NULL;
> +				continue;
>  			}
> +
> +			/*
> +			 * For multi-LUN ATAPI (BLIST_FORCELUN), trigger a
> +			 * sequential scan for this target.  pdt_1f_for_no_lun,
> +			 * set during LUN 0 configure, ensures non-responding
> +			 * LUNs are silently skipped; dev->sdev[] is populated
> +			 * by ata_scsi_dev_config() during the scan.
> +			 */
> +			if (dev->class == ATA_DEV_ATAPI &&
> +			    sdev->sdev_bflags & BLIST_FORCELUN &&
> +			    !WARN_ON_ONCE(ap->scsi_host->max_lun > ATAPI_MAX_LUN))
> +				scsi_scan_target(&ap->scsi_host->shost_gendev,
> +						 channel, id, SCAN_WILD_CARD,
> +						 SCSI_SCAN_RESCAN);
> +			if (dev->sdev[0])
> +				ata_scsi_assign_ofnode(dev, ap);

[Severity: High]
This is a pre-existing issue, but does this check leave a race condition where
concurrent device removals can cause a NULL pointer dereference?

In ata_scsi_scan_host(), the lockless if (dev->sdev[0]) check is evaluated
after releasing shost->scan_mutex (which was temporarily held by the preceding
scsi_scan_target and __scsi_add_device calls).

Concurrently, a hot-unplug event can trigger ata_scsi_remove_dev() on a
workqueue, which takes the scan_mutex and sets dev->sdev[0] = NULL.

If this concurrent removal occurs exactly after the check but before
ata_scsi_assign_ofnode() re-reads dev->sdev[0]:

drivers/ata/libata-scsi.c:ata_scsi_assign_ofnode() {
	struct scsi_device *sdev = ata_dev_scsi_device(dev, 0);
	...
			sdev->sdev_gendev.of_node = child;
}

Can sdev be NULL when it is unconditionally dereferenced? Would it be safer
to pass the safely refcounted local sdev directly to ata_scsi_assign_ofnode()?

> +			scsi_device_put(sdev);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731213503.2379771-1-philpem@philpem.me.uk?part=5