[PATCH v2 00/10] scsi: pm80xx: Fix expander support

[PATCH v2 00/10] scsi: pm80xx: Fix expander support

[Niklas Cassel]

Hello all,

Some recent patches broke expander support for the pm80xx driver.

The first two patches in this series make sure that expanders work with
the pm80xx driver again.

It also fixes a bug in pm8001_abort_task() that was found through code
review.

There is also some patches that make the pm80xx driver more robust, so it
is less likely that the expander support will break again in the future.

There is also some minor changes to some other libsas drivers to make use
of the new dev_parent_is_expander() helper.

Please test and review.


Kind regards,
Niklas


Changes since V1:
-Addressed Damien's review comments.
-Picked up tags from Igor. Did not pick up tags on patches that were
 changed.


Niklas Cassel (10):
  scsi: pm80xx: Restore support for expanders
  scsi: pm80xx: Fix array-index-out-of-of-bounds on rmmod
  scsi: libsas: Add dev_parent_is_expander() helper
  scsi: hisi_sas: Use dev_parent_is_expander() helper
  scsi: isci: Use dev_parent_is_expander() helper
  scsi: mvsas: Use dev_parent_is_expander() helper
  scsi: pm80xx: Use dev_parent_is_expander() helper
  scsi: pm80xx: Add helper function to get the local phy id
  scsi: pm80xx: Fix pm8001_abort_task() for chip_8006 when using an
    expander
  scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array

 drivers/scsi/hisi_sas/hisi_sas_main.c  |  2 +-
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  6 ++---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  6 ++---
 drivers/scsi/isci/remote_device.c      |  2 +-
 drivers/scsi/libsas/sas_expander.c     |  5 +---
 drivers/scsi/mvsas/mv_sas.c            |  2 +-
 drivers/scsi/pm8001/pm8001_hwi.c       | 11 +++------
 drivers/scsi/pm8001/pm8001_sas.c       | 34 ++++++++++++++++++++------
 drivers/scsi/pm8001/pm8001_sas.h       |  1 +
 drivers/scsi/pm8001/pm80xx_hwi.c       | 10 +++-----
 include/scsi/libsas.h                  |  8 ++++++
 11 files changed, 50 insertions(+), 37 deletions(-)

-- 
2.50.1

[PATCH v2 01/10] scsi: pm80xx: Restore support for expanders

[Niklas Cassel]

Commit 0f630c58e31a ("scsi: pm80xx: Do not use libsas port ID") broke
support for expanders. After the commit, devices behind an expander are no
longer detected.

Simply reverting the commit restores support for devices behind an
expander.

Instead of reverting the commit (and reintroducing a helper to get the
port), get the port directly from the lldd_port pointer in struct
asd_sas_port.

Fixes: 0f630c58e31a ("scsi: pm80xx: Do not use libsas port ID")
Suggested-by: Igor Pylypiv <ipylypiv@google.com>
Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/pm8001/pm8001_sas.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index f7067878b34f..753c09363cbb 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -477,7 +477,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
 	struct pm8001_device *pm8001_dev = dev->lldd_dev;
 	bool internal_abort = sas_is_internal_abort(task);
 	struct pm8001_hba_info *pm8001_ha;
-	struct pm8001_port *port = NULL;
+	struct pm8001_port *port;
 	struct pm8001_ccb_info *ccb;
 	unsigned long flags;
 	u32 n_elem = 0;
@@ -502,8 +502,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
 
 	spin_lock_irqsave(&pm8001_ha->lock, flags);
 
-	pm8001_dev = dev->lldd_dev;
-	port = pm8001_ha->phy[pm8001_dev->attached_phy].port;
+	port = dev->port->lldd_port;
 
 	if (!internal_abort &&
 	    (DEV_IS_GONE(pm8001_dev) || !port || !port->port_attached)) {
-- 
2.50.1

[PATCH v2 02/10] scsi: pm80xx: Fix array-index-out-of-of-bounds on rmmod

[Niklas Cassel]

Since commit f7b705c238d1 ("scsi: pm80xx: Set phy_attached to zero when
device is gone") UBSAN reports:
  UBSAN: array-index-out-of-bounds in drivers/scsi/pm8001/pm8001_sas.c:786:17
  index 28 is out of range for type 'pm8001_phy [16]'
on rmmod when using an expander.

For a direct attached device, attached_phy contains the local phy id.
For a device behind an expander, attached_phy contains the remote phy id,
not the local phy id.

I.e. while pm8001_ha will have pm8001_ha->chip->n_phy local phys, for a
device behind an expander, attached_phy can be much larger than
pm8001_ha->chip->n_phy (depending on the amount of phys of the expander).

E.g. on my system pm8001_ha has 8 phys with phy ids 0-7.
One of the ports has an expander connected.
The expander has 31 phys with phy ids 0-30.

The pm8001_ha->phy array only contains the phys of the HBA.
It does not contain the phys of the expander.
Thus, it is wrong to use attached_phy to index the pm8001_ha->phy array
for a device behind an expander.

Thus, we can only clear phy_attached for devices that are directly
attached.

Fixes: f7b705c238d1 ("scsi: pm80xx: Set phy_attached to zero when device is gone")
Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/pm8001/pm8001_sas.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 753c09363cbb..3e1dac4b820f 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -749,6 +749,7 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
 	unsigned long flags = 0;
 	struct pm8001_hba_info *pm8001_ha;
 	struct pm8001_device *pm8001_dev = dev->lldd_dev;
+	struct domain_device *parent_dev = dev->parent;
 
 	pm8001_ha = pm8001_find_ha_by_dev(dev);
 	spin_lock_irqsave(&pm8001_ha->lock, flags);
@@ -765,7 +766,13 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
 			spin_lock_irqsave(&pm8001_ha->lock, flags);
 		}
 		PM8001_CHIP_DISP->dereg_dev_req(pm8001_ha, device_id);
-		pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
+
+		/*
+		 * The phy array only contains local phys. Thus, we cannot clear
+		 * phy_attached for a device behind an expander.
+		 */
+		if (!(parent_dev && dev_is_expander(parent_dev->dev_type)))
+			pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
 		pm8001_free_dev(pm8001_dev);
 	} else {
 		pm8001_dbg(pm8001_ha, DISC, "Found dev has gone.\n");
-- 
2.50.1

[PATCH v2 03/10] scsi: libsas: Add dev_parent_is_expander() helper

[Niklas Cassel]

Many libsas drivers check if the parent of the device is an expander.
Create a helper that the libsas drivers will use in follow up commits.

Suggested-by: Damien Le Moal <dlemoal@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/libsas/sas_expander.c | 5 +----
 include/scsi/libsas.h              | 8 ++++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 869b5d4db44c..d953225f6cc2 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1313,10 +1313,7 @@ static int sas_check_parent_topology(struct domain_device *child)
 	int i;
 	int res = 0;
 
-	if (!child->parent)
-		return 0;
-
-	if (!dev_is_expander(child->parent->dev_type))
+	if (!dev_parent_is_expander(child))
 		return 0;
 
 	parent_ex = &child->parent->ex_dev;
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ba460b6c0374..8d38565e99fa 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -203,6 +203,14 @@ static inline bool dev_is_expander(enum sas_device_type type)
 	       type == SAS_FANOUT_EXPANDER_DEVICE;
 }
 
+static inline bool dev_parent_is_expander(struct domain_device *dev)
+{
+	if (!dev->parent)
+		return false;
+
+	return dev_is_expander(dev->parent->dev_type);
+}
+
 static inline void INIT_SAS_WORK(struct sas_work *sw, void (*fn)(struct work_struct *))
 {
 	INIT_WORK(&sw->work, fn);
-- 
2.50.1

[PATCH v2 04/10] scsi: hisi_sas: Use dev_parent_is_expander() helper

[Niklas Cassel]

Make use of the dev_parent_is_expander() helper.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 2 +-
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 6 ++----
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 ++----
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index d1a4cc69d408..30a9c6612651 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -876,7 +876,7 @@ static int hisi_sas_dev_found(struct domain_device *device)
 	device->lldd_dev = sas_dev;
 	hisi_hba->hw->setup_itct(hisi_hba, sas_dev);
 
-	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
+	if (dev_parent_is_expander(device)) {
 		int phy_no;
 
 		phy_no = sas_find_attached_phy_id(&parent_dev->ex_dev, device);
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 4431698a5d78..f3516a0611dd 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -925,7 +925,6 @@ static void setup_itct_v2_hw(struct hisi_hba *hisi_hba,
 	struct device *dev = hisi_hba->dev;
 	u64 qw0, device_id = sas_dev->device_id;
 	struct hisi_sas_itct *itct = &hisi_hba->itct[device_id];
-	struct domain_device *parent_dev = device->parent;
 	struct asd_sas_port *sas_port = device->port;
 	struct hisi_sas_port *port = to_hisi_sas_port(sas_port);
 	u64 sas_addr;
@@ -942,7 +941,7 @@ static void setup_itct_v2_hw(struct hisi_hba *hisi_hba,
 		break;
 	case SAS_SATA_DEV:
 	case SAS_SATA_PENDING:
-		if (parent_dev && dev_is_expander(parent_dev->dev_type))
+		if (dev_parent_is_expander(device))
 			qw0 = HISI_SAS_DEV_TYPE_STP << ITCT_HDR_DEV_TYPE_OFF;
 		else
 			qw0 = HISI_SAS_DEV_TYPE_SATA << ITCT_HDR_DEV_TYPE_OFF;
@@ -2494,7 +2493,6 @@ static void prep_ata_v2_hw(struct hisi_hba *hisi_hba,
 {
 	struct sas_task *task = slot->task;
 	struct domain_device *device = task->dev;
-	struct domain_device *parent_dev = device->parent;
 	struct hisi_sas_device *sas_dev = device->lldd_dev;
 	struct hisi_sas_cmd_hdr *hdr = slot->cmd_hdr;
 	struct asd_sas_port *sas_port = device->port;
@@ -2509,7 +2507,7 @@ static void prep_ata_v2_hw(struct hisi_hba *hisi_hba,
 	/* create header */
 	/* dw0 */
 	dw0 = port->id << CMD_HDR_PORT_OFF;
-	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
+	if (dev_parent_is_expander(device)) {
 		dw0 |= 3 << CMD_HDR_CMD_OFF;
 	} else {
 		phy_id = device->phy->identify.phy_identifier;
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 2f3d61abab3a..2f9e01717ef3 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -874,7 +874,6 @@ static void setup_itct_v3_hw(struct hisi_hba *hisi_hba,
 	struct device *dev = hisi_hba->dev;
 	u64 qw0, device_id = sas_dev->device_id;
 	struct hisi_sas_itct *itct = &hisi_hba->itct[device_id];
-	struct domain_device *parent_dev = device->parent;
 	struct asd_sas_port *sas_port = device->port;
 	struct hisi_sas_port *port = to_hisi_sas_port(sas_port);
 	u64 sas_addr;
@@ -891,7 +890,7 @@ static void setup_itct_v3_hw(struct hisi_hba *hisi_hba,
 		break;
 	case SAS_SATA_DEV:
 	case SAS_SATA_PENDING:
-		if (parent_dev && dev_is_expander(parent_dev->dev_type))
+		if (dev_parent_is_expander(device))
 			qw0 = HISI_SAS_DEV_TYPE_STP << ITCT_HDR_DEV_TYPE_OFF;
 		else
 			qw0 = HISI_SAS_DEV_TYPE_SATA << ITCT_HDR_DEV_TYPE_OFF;
@@ -1476,7 +1475,6 @@ static void prep_ata_v3_hw(struct hisi_hba *hisi_hba,
 {
 	struct sas_task *task = slot->task;
 	struct domain_device *device = task->dev;
-	struct domain_device *parent_dev = device->parent;
 	struct hisi_sas_device *sas_dev = device->lldd_dev;
 	struct hisi_sas_cmd_hdr *hdr = slot->cmd_hdr;
 	struct asd_sas_port *sas_port = device->port;
@@ -1487,7 +1485,7 @@ static void prep_ata_v3_hw(struct hisi_hba *hisi_hba,
 	u32 dw1 = 0, dw2 = 0;
 
 	hdr->dw0 = cpu_to_le32(port->id << CMD_HDR_PORT_OFF);
-	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
+	if (dev_parent_is_expander(device)) {
 		hdr->dw0 |= cpu_to_le32(3 << CMD_HDR_CMD_OFF);
 	} else {
 		phy_id = device->phy->identify.phy_identifier;
-- 
2.50.1

[PATCH v2 05/10] scsi: isci: Use dev_parent_is_expander() helper

[Niklas Cassel]

Make use of the dev_parent_is_expander() helper.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/isci/remote_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/isci/remote_device.c b/drivers/scsi/isci/remote_device.c
index 82deb6a83a8c..4c7462965ea1 100644
--- a/drivers/scsi/isci/remote_device.c
+++ b/drivers/scsi/isci/remote_device.c
@@ -1434,7 +1434,7 @@ static enum sci_status isci_remote_device_construct(struct isci_port *iport,
 	struct domain_device *dev = idev->domain_dev;
 	enum sci_status status;
 
-	if (dev->parent && dev_is_expander(dev->parent->dev_type))
+	if (dev_parent_is_expander(dev))
 		status = sci_remote_device_ea_construct(iport, idev);
 	else
 		status = sci_remote_device_da_construct(iport, idev);
-- 
2.50.1

[PATCH v2 06/10] scsi: mvsas: Use dev_parent_is_expander() helper

[Niklas Cassel]

Make use of the dev_parent_is_expander() helper.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/mvsas/mv_sas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 15b3d9d55a4b..f2e7997d5b9d 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -1175,7 +1175,7 @@ static int mvs_dev_found_notify(struct domain_device *dev, int lock)
 	mvi_device->dev_type = dev->dev_type;
 	mvi_device->mvi_info = mvi;
 	mvi_device->sas_device = dev;
-	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
+	if (dev_parent_is_expander(dev)) {
 		int phy_id;
 
 		phy_id = sas_find_attached_phy_id(&parent_dev->ex_dev, dev);
-- 
2.50.1

[PATCH v2 07/10] scsi: pm80xx: Use dev_parent_is_expander() helper

[Niklas Cassel]

Make use of the dev_parent_is_expander() helper.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/pm8001/pm8001_hwi.c | 8 +++-----
 drivers/scsi/pm8001/pm8001_sas.c | 5 ++---
 drivers/scsi/pm8001/pm80xx_hwi.c | 8 +++-----
 3 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index 42a4eeac24c9..fb4913547b00 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -2163,8 +2163,7 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
 	/* Print sas address of IO failed device */
 	if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
 		(status != IO_UNDERFLOW)) {
-		if (!((t->dev->parent) &&
-			(dev_is_expander(t->dev->parent->dev_type)))) {
+		if (!dev_parent_is_expander(t->dev)) {
 			for (i = 0, j = 4; j <= 7 && i <= 3; i++, j++)
 				sata_addr_low[i] = pm8001_ha->sas_addr[j];
 			for (i = 0, j = 0; j <= 3 && i <= 3; i++, j++)
@@ -4168,7 +4167,6 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 	u16 firstBurstSize = 0;
 	u16 ITNT = 2000;
 	struct domain_device *dev = pm8001_dev->sas_device;
-	struct domain_device *parent_dev = dev->parent;
 	struct pm8001_port *port = dev->port->lldd_port;
 
 	memset(&payload, 0, sizeof(payload));
@@ -4186,8 +4184,8 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 			dev_is_expander(pm8001_dev->dev_type))
 			stp_sspsmp_sata = 0x01; /*ssp or smp*/
 	}
-	if (parent_dev && dev_is_expander(parent_dev->dev_type))
-		phy_id = parent_dev->ex_dev.ex_phy->phy_id;
+	if (dev_parent_is_expander(dev))
+		phy_id = dev->parent->ex_dev.ex_phy->phy_id;
 	else
 		phy_id = pm8001_dev->attached_phy;
 	opc = OPC_INB_REG_DEV;
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 3e1dac4b820f..2bdeace6c6bf 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -700,7 +700,7 @@ static int pm8001_dev_found_notify(struct domain_device *dev)
 	dev->lldd_dev = pm8001_device;
 	pm8001_device->dev_type = dev->dev_type;
 	pm8001_device->dcompletion = &completion;
-	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
+	if (dev_parent_is_expander(dev)) {
 		int phy_id;
 
 		phy_id = sas_find_attached_phy_id(&parent_dev->ex_dev, dev);
@@ -749,7 +749,6 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
 	unsigned long flags = 0;
 	struct pm8001_hba_info *pm8001_ha;
 	struct pm8001_device *pm8001_dev = dev->lldd_dev;
-	struct domain_device *parent_dev = dev->parent;
 
 	pm8001_ha = pm8001_find_ha_by_dev(dev);
 	spin_lock_irqsave(&pm8001_ha->lock, flags);
@@ -771,7 +770,7 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
 		 * The phy array only contains local phys. Thus, we cannot clear
 		 * phy_attached for a device behind an expander.
 		 */
-		if (!(parent_dev && dev_is_expander(parent_dev->dev_type)))
+		if (!dev_parent_is_expander(dev))
 			pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
 		pm8001_free_dev(pm8001_dev);
 	} else {
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index c1bae995a412..546d0d26f7a1 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -2340,8 +2340,7 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
 	/* Print sas address of IO failed device */
 	if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
 		(status != IO_UNDERFLOW)) {
-		if (!((t->dev->parent) &&
-			(dev_is_expander(t->dev->parent->dev_type)))) {
+		if (!dev_parent_is_expander(t->dev)) {
 			for (i = 0, j = 4; i <= 3 && j <= 7; i++, j++)
 				sata_addr_low[i] = pm8001_ha->sas_addr[j];
 			for (i = 0, j = 0; i <= 3 && j <= 3; i++, j++)
@@ -4780,7 +4779,6 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 	u16 firstBurstSize = 0;
 	u16 ITNT = 2000;
 	struct domain_device *dev = pm8001_dev->sas_device;
-	struct domain_device *parent_dev = dev->parent;
 	struct pm8001_port *port = dev->port->lldd_port;
 
 	memset(&payload, 0, sizeof(payload));
@@ -4799,8 +4797,8 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 			dev_is_expander(pm8001_dev->dev_type))
 			stp_sspsmp_sata = 0x01; /*ssp or smp*/
 	}
-	if (parent_dev && dev_is_expander(parent_dev->dev_type))
-		phy_id = parent_dev->ex_dev.ex_phy->phy_id;
+	if (dev_parent_is_expander(dev))
+		phy_id = dev->parent->ex_dev.ex_phy->phy_id;
 	else
 		phy_id = pm8001_dev->attached_phy;
 
-- 
2.50.1

[PATCH v2 08/10] scsi: pm80xx: Add helper function to get the local phy id

[Niklas Cassel]

Avoid duplicated code by adding a helper to get the local phy id.

No functional changes intended.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/pm8001/pm8001_hwi.c |  7 +++----
 drivers/scsi/pm8001/pm8001_sas.c | 10 ++++++++++
 drivers/scsi/pm8001/pm8001_sas.h |  1 +
 drivers/scsi/pm8001/pm80xx_hwi.c |  6 ++----
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index fb4913547b00..8005995a317c 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -4184,10 +4184,9 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 			dev_is_expander(pm8001_dev->dev_type))
 			stp_sspsmp_sata = 0x01; /*ssp or smp*/
 	}
-	if (dev_parent_is_expander(dev))
-		phy_id = dev->parent->ex_dev.ex_phy->phy_id;
-	else
-		phy_id = pm8001_dev->attached_phy;
+
+	phy_id = pm80xx_get_local_phy_id(dev);
+
 	opc = OPC_INB_REG_DEV;
 	linkrate = (pm8001_dev->sas_device->linkrate < dev->port->linkrate) ?
 			pm8001_dev->sas_device->linkrate : dev->port->linkrate;
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 2bdeace6c6bf..5595913eb7fc 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -130,6 +130,16 @@ static void pm80xx_get_tag_opcodes(struct sas_task *task, int *ata_op,
 	}
 }
 
+u32 pm80xx_get_local_phy_id(struct domain_device *dev)
+{
+	struct pm8001_device *pm8001_dev = dev->lldd_dev;
+
+	if (dev_parent_is_expander(dev))
+		return dev->parent->ex_dev.ex_phy->phy_id;
+
+	return pm8001_dev->attached_phy;
+}
+
 void pm80xx_show_pending_commands(struct pm8001_hba_info *pm8001_ha,
 				  struct pm8001_device *target_pm8001_dev)
 {
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 334485bb2c12..91b2cdf3535c 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -798,6 +798,7 @@ void pm8001_setds_completion(struct domain_device *dev);
 void pm8001_tmf_aborted(struct sas_task *task);
 void pm80xx_show_pending_commands(struct pm8001_hba_info *pm8001_ha,
 				  struct pm8001_device *dev);
+u32 pm80xx_get_local_phy_id(struct domain_device *dev);
 
 #endif
 
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 546d0d26f7a1..31960b72c1e9 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -4797,10 +4797,8 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
 			dev_is_expander(pm8001_dev->dev_type))
 			stp_sspsmp_sata = 0x01; /*ssp or smp*/
 	}
-	if (dev_parent_is_expander(dev))
-		phy_id = dev->parent->ex_dev.ex_phy->phy_id;
-	else
-		phy_id = pm8001_dev->attached_phy;
+
+	phy_id = pm80xx_get_local_phy_id(dev);
 
 	opc = OPC_INB_REG_DEV;
 
-- 
2.50.1

[PATCH v2 09/10] scsi: pm80xx: Fix pm8001_abort_task() for chip_8006 when using an expander

[Niklas Cassel]

For a direct attached device, attached_phy contains the local phy id.
For a device behind an expander, attached_phy contains the remote phy id,
not the local phy id.

The pm8001_ha->phy array only contains the phys of the HBA.
It does not contain the phys of the expander.

Thus, you cannot use attached_phy to index the pm8001_ha->phy array,
without first verifying that the device is directly attached.

Use the pm80xx_get_local_phy_id() helper to make sure that we use the
local phy id to index the array, regardless if the device is directly
attached or not.

Fixes: 869ddbdcae3b ("scsi: pm80xx: corrected SATA abort handling sequence.")
Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/pm8001/pm8001_sas.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 5595913eb7fc..c5354263b45e 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -1063,7 +1063,7 @@ int pm8001_abort_task(struct sas_task *task)
 	struct pm8001_hba_info *pm8001_ha;
 	struct pm8001_device *pm8001_dev;
 	int rc = TMF_RESP_FUNC_FAILED, ret;
-	u32 phy_id, port_id;
+	u32 port_id;
 	struct sas_task_slow slow_task;
 
 	if (!task->lldd_task || !task->dev)
@@ -1072,7 +1072,6 @@ int pm8001_abort_task(struct sas_task *task)
 	dev = task->dev;
 	pm8001_dev = dev->lldd_dev;
 	pm8001_ha = pm8001_find_ha_by_dev(dev);
-	phy_id = pm8001_dev->attached_phy;
 
 	if (PM8001_CHIP_DISP->fatal_errors(pm8001_ha)) {
 		// If the controller is seeing fatal errors
@@ -1104,7 +1103,8 @@ int pm8001_abort_task(struct sas_task *task)
 		if (pm8001_ha->chip_id == chip_8006) {
 			DECLARE_COMPLETION_ONSTACK(completion_reset);
 			DECLARE_COMPLETION_ONSTACK(completion);
-			struct pm8001_phy *phy = pm8001_ha->phy + phy_id;
+			u32 phy_id = pm80xx_get_local_phy_id(dev);
+			struct pm8001_phy *phy = &pm8001_ha->phy[phy_id];
 			port_id = phy->port->port_id;
 
 			/* 1. Set Device state as Recovery */
-- 
2.50.1

[PATCH v2 10/10] scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array

[Niklas Cassel]

While the current code is perfectly fine (because we verify that the
device is directly attached before using attached_phy to index the
pm8001_ha->phy array), let's use the pm80xx_get_local_phy_id() helper
anyway, to reduce the chance that someone will copy paste this pattern to
other parts of the driver.

Note that in this specific case, we still need to keep the check that the
device is not behind an expander, because we do not want to clear
attached_phy of the expander if a device behind the expander disappears
(as that would disable all the other devices behind the expander).

However, if it is the expander itself that disappears, attached_phy will
be cleared, just like it would for any other directly attached device.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/scsi/pm8001/pm8001_sas.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index c5354263b45e..6a8d35aea93a 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -780,8 +780,11 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
 		 * The phy array only contains local phys. Thus, we cannot clear
 		 * phy_attached for a device behind an expander.
 		 */
-		if (!dev_parent_is_expander(dev))
-			pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
+		if (!dev_parent_is_expander(dev)) {
+			u32 phy_id = pm80xx_get_local_phy_id(dev);
+
+			pm8001_ha->phy[phy_id].phy_attached = 0;
+		}
 		pm8001_free_dev(pm8001_dev);
 	} else {
 		pm8001_dbg(pm8001_ha, DISC, "Found dev has gone.\n");
-- 
2.50.1

Re: [PATCH v2 01/10] scsi: pm80xx: Restore support for expanders

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Commit 0f630c58e31a ("scsi: pm80xx: Do not use libsas port ID") broke
> support for expanders. After the commit, devices behind an expander are no
> longer detected.
> 
> Simply reverting the commit restores support for devices behind an
> expander.
> 
> Instead of reverting the commit (and reintroducing a helper to get the
> port), get the port directly from the lldd_port pointer in struct
> asd_sas_port.
> 
> Fixes: 0f630c58e31a ("scsi: pm80xx: Do not use libsas port ID")
> Suggested-by: Igor Pylypiv <ipylypiv@google.com>
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Tested-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 02/10] scsi: pm80xx: Fix array-index-out-of-of-bounds on rmmod

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Since commit f7b705c238d1 ("scsi: pm80xx: Set phy_attached to zero when
> device is gone") UBSAN reports:
>   UBSAN: array-index-out-of-bounds in drivers/scsi/pm8001/pm8001_sas.c:786:17
>   index 28 is out of range for type 'pm8001_phy [16]'
> on rmmod when using an expander.
> 
> For a direct attached device, attached_phy contains the local phy id.
> For a device behind an expander, attached_phy contains the remote phy id,
> not the local phy id.
> 
> I.e. while pm8001_ha will have pm8001_ha->chip->n_phy local phys, for a
> device behind an expander, attached_phy can be much larger than
> pm8001_ha->chip->n_phy (depending on the amount of phys of the expander).
> 
> E.g. on my system pm8001_ha has 8 phys with phy ids 0-7.
> One of the ports has an expander connected.
> The expander has 31 phys with phy ids 0-30.
> 
> The pm8001_ha->phy array only contains the phys of the HBA.
> It does not contain the phys of the expander.
> Thus, it is wrong to use attached_phy to index the pm8001_ha->phy array
> for a device behind an expander.
> 
> Thus, we can only clear phy_attached for devices that are directly
> attached.
> 
> Fixes: f7b705c238d1 ("scsi: pm80xx: Set phy_attached to zero when device is gone")
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Tested-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 03/10] scsi: libsas: Add dev_parent_is_expander() helper

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Many libsas drivers check if the parent of the device is an expander.
> Create a helper that the libsas drivers will use in follow up commits.
> 
> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 04/10] scsi: hisi_sas: Use dev_parent_is_expander() helper

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 05/10] scsi: isci: Use dev_parent_is_expander() helper

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 06/10] scsi: mvsas: Use dev_parent_is_expander() helper

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 07/10] scsi: pm80xx: Use dev_parent_is_expander() helper

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 08/10] scsi: pm80xx: Add helper function to get the local phy id

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> Avoid duplicated code by adding a helper to get the local phy id.
> 
> No functional changes intended.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 09/10] scsi: pm80xx: Fix pm8001_abort_task() for chip_8006 when using an expander

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> For a direct attached device, attached_phy contains the local phy id.
> For a device behind an expander, attached_phy contains the remote phy id,
> not the local phy id.
> 
> The pm8001_ha->phy array only contains the phys of the HBA.
> It does not contain the phys of the expander.
> 
> Thus, you cannot use attached_phy to index the pm8001_ha->phy array,
> without first verifying that the device is directly attached.
> 
> Use the pm80xx_get_local_phy_id() helper to make sure that we use the
> local phy id to index the array, regardless if the device is directly
> attached or not.
> 
> Fixes: 869ddbdcae3b ("scsi: pm80xx: corrected SATA abort handling sequence.")
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 10/10] scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array

[Damien Le Moal]

On 8/15/25 02:32, Niklas Cassel wrote:
> While the current code is perfectly fine (because we verify that the
> device is directly attached before using attached_phy to index the
> pm8001_ha->phy array), let's use the pm80xx_get_local_phy_id() helper
> anyway, to reduce the chance that someone will copy paste this pattern to
> other parts of the driver.
> 
> Note that in this specific case, we still need to keep the check that the
> device is not behind an expander, because we do not want to clear
> attached_phy of the expander if a device behind the expander disappears
> (as that would disable all the other devices behind the expander).
> 
> However, if it is the expander itself that disappears, attached_phy will
> be cleared, just like it would for any other directly attached device.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 03/10] scsi: libsas: Add dev_parent_is_expander() helper

[John Garry]

On 14/08/2025 18:32, Niklas Cassel wrote:
> Many libsas drivers check if the parent of the device is an expander.
> Create a helper that the libsas drivers will use in follow up commits.
> 
> Suggested-by: Damien Le Moal<dlemoal@kernel.org>
> Signed-off-by: Niklas Cassel<cassel@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v2 04/10] scsi: hisi_sas: Use dev_parent_is_expander() helper

[John Garry]

On 14/08/2025 18:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel<cassel@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v2 05/10] scsi: isci: Use dev_parent_is_expander() helper

[John Garry]

On 14/08/2025 18:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel<cassel@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v2 06/10] scsi: mvsas: Use dev_parent_is_expander() helper

[John Garry]

On 14/08/2025 18:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel<cassel@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v2 07/10] scsi: pm80xx: Use dev_parent_is_expander() helper

[John Garry]

On 14/08/2025 18:32, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel<cassel@kernel.org>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH v2 03/10] scsi: libsas: Add dev_parent_is_expander() helper

[John Garry]

On 15/08/2025 08:07, John Garry wrote:
> On 14/08/2025 18:32, Niklas Cassel wrote:
>> Many libsas drivers check if the parent of the device is an expander.
>> Create a helper that the libsas drivers will use in follow up commits.
>>
>> Suggested-by: Damien Le Moal<dlemoal@kernel.org>
>> Signed-off-by: Niklas Cassel<cassel@kernel.org>
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 

BTW, I was going to suggest to make dev_is_expander() to accept a 
domain_device * (like dev_parent_is_expander()). That would make the 
code more concise. However there are a couple of callsites which pass 
phy->attached_dev_type, and not dev->dev_type.

Re: [PATCH v2 07/10] scsi: pm80xx: Use dev_parent_is_expander() helper

[Igor Pylypiv]

On Thu, Aug 14, 2025 at 07:32:22PM +0200, Niklas Cassel wrote:
> Make use of the dev_parent_is_expander() helper.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Re: [PATCH v2 10/10] scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array

[Igor Pylypiv]

On Thu, Aug 14, 2025 at 07:32:25PM +0200, Niklas Cassel wrote:
> While the current code is perfectly fine (because we verify that the
> device is directly attached before using attached_phy to index the
> pm8001_ha->phy array), let's use the pm80xx_get_local_phy_id() helper
> anyway, to reduce the chance that someone will copy paste this pattern to
> other parts of the driver.
> 
> Note that in this specific case, we still need to keep the check that the
> device is not behind an expander, because we do not want to clear
> attached_phy of the expander if a device behind the expander disappears
> (as that would disable all the other devices behind the expander).
> 
> However, if it is the expander itself that disappears, attached_phy will
> be cleared, just like it would for any other directly attached device.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Re: [PATCH v2 03/10] scsi: libsas: Add dev_parent_is_expander() helper

[Jason Yan]

在 2025/8/15 1:32, Niklas Cassel 写道:
> Many libsas drivers check if the parent of the device is an expander.
> Create a helper that the libsas drivers will use in follow up commits.
> 
> Suggested-by: Damien Le Moal<dlemoal@kernel.org>
> Signed-off-by: Niklas Cassel<cassel@kernel.org>
> ---
>   drivers/scsi/libsas/sas_expander.c | 5 +----
>   include/scsi/libsas.h              | 8 ++++++++
>   2 files changed, 9 insertions(+), 4 deletions(-)

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Re: [PATCH v2 07/10] scsi: pm80xx: Use dev_parent_is_expander() helper

[Jinpu Wang]

On Thu, Aug 14, 2025 at 7:32 PM Niklas Cassel <cassel@kernel.org> wrote:
>
> Make use of the dev_parent_is_expander() helper.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c | 8 +++-----
>  drivers/scsi/pm8001/pm8001_sas.c | 5 ++---
>  drivers/scsi/pm8001/pm80xx_hwi.c | 8 +++-----
>  3 files changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index 42a4eeac24c9..fb4913547b00 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -2163,8 +2163,7 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha, void *piomb)
>         /* Print sas address of IO failed device */
>         if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
>                 (status != IO_UNDERFLOW)) {
> -               if (!((t->dev->parent) &&
> -                       (dev_is_expander(t->dev->parent->dev_type)))) {
> +               if (!dev_parent_is_expander(t->dev)) {
>                         for (i = 0, j = 4; j <= 7 && i <= 3; i++, j++)
>                                 sata_addr_low[i] = pm8001_ha->sas_addr[j];
>                         for (i = 0, j = 0; j <= 3 && i <= 3; i++, j++)
> @@ -4168,7 +4167,6 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         u16 firstBurstSize = 0;
>         u16 ITNT = 2000;
>         struct domain_device *dev = pm8001_dev->sas_device;
> -       struct domain_device *parent_dev = dev->parent;
>         struct pm8001_port *port = dev->port->lldd_port;
>
>         memset(&payload, 0, sizeof(payload));
> @@ -4186,8 +4184,8 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>                         dev_is_expander(pm8001_dev->dev_type))
>                         stp_sspsmp_sata = 0x01; /*ssp or smp*/
>         }
> -       if (parent_dev && dev_is_expander(parent_dev->dev_type))
> -               phy_id = parent_dev->ex_dev.ex_phy->phy_id;
> +       if (dev_parent_is_expander(dev))
> +               phy_id = dev->parent->ex_dev.ex_phy->phy_id;
>         else
>                 phy_id = pm8001_dev->attached_phy;
>         opc = OPC_INB_REG_DEV;
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 3e1dac4b820f..2bdeace6c6bf 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -700,7 +700,7 @@ static int pm8001_dev_found_notify(struct domain_device *dev)
>         dev->lldd_dev = pm8001_device;
>         pm8001_device->dev_type = dev->dev_type;
>         pm8001_device->dcompletion = &completion;
> -       if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
> +       if (dev_parent_is_expander(dev)) {
>                 int phy_id;
>
>                 phy_id = sas_find_attached_phy_id(&parent_dev->ex_dev, dev);
> @@ -749,7 +749,6 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
>         unsigned long flags = 0;
>         struct pm8001_hba_info *pm8001_ha;
>         struct pm8001_device *pm8001_dev = dev->lldd_dev;
> -       struct domain_device *parent_dev = dev->parent;
>
>         pm8001_ha = pm8001_find_ha_by_dev(dev);
>         spin_lock_irqsave(&pm8001_ha->lock, flags);
> @@ -771,7 +770,7 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
>                  * The phy array only contains local phys. Thus, we cannot clear
>                  * phy_attached for a device behind an expander.
>                  */
> -               if (!(parent_dev && dev_is_expander(parent_dev->dev_type)))
> +               if (!dev_parent_is_expander(dev))
>                         pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
>                 pm8001_free_dev(pm8001_dev);
>         } else {
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index c1bae995a412..546d0d26f7a1 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -2340,8 +2340,7 @@ mpi_sata_completion(struct pm8001_hba_info *pm8001_ha,
>         /* Print sas address of IO failed device */
>         if ((status != IO_SUCCESS) && (status != IO_OVERFLOW) &&
>                 (status != IO_UNDERFLOW)) {
> -               if (!((t->dev->parent) &&
> -                       (dev_is_expander(t->dev->parent->dev_type)))) {
> +               if (!dev_parent_is_expander(t->dev)) {
>                         for (i = 0, j = 4; i <= 3 && j <= 7; i++, j++)
>                                 sata_addr_low[i] = pm8001_ha->sas_addr[j];
>                         for (i = 0, j = 0; i <= 3 && j <= 3; i++, j++)
> @@ -4780,7 +4779,6 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>         u16 firstBurstSize = 0;
>         u16 ITNT = 2000;
>         struct domain_device *dev = pm8001_dev->sas_device;
> -       struct domain_device *parent_dev = dev->parent;
>         struct pm8001_port *port = dev->port->lldd_port;
>
>         memset(&payload, 0, sizeof(payload));
> @@ -4799,8 +4797,8 @@ static int pm80xx_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha,
>                         dev_is_expander(pm8001_dev->dev_type))
>                         stp_sspsmp_sata = 0x01; /*ssp or smp*/
>         }
> -       if (parent_dev && dev_is_expander(parent_dev->dev_type))
> -               phy_id = parent_dev->ex_dev.ex_phy->phy_id;
> +       if (dev_parent_is_expander(dev))
> +               phy_id = dev->parent->ex_dev.ex_phy->phy_id;
>         else
>                 phy_id = pm8001_dev->attached_phy;
>
> --
> 2.50.1
>

Re: [PATCH v2 10/10] scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array

[Jinpu Wang]

On Thu, Aug 14, 2025 at 7:32 PM Niklas Cassel <cassel@kernel.org> wrote:
>
> While the current code is perfectly fine (because we verify that the
> device is directly attached before using attached_phy to index the
> pm8001_ha->phy array), let's use the pm80xx_get_local_phy_id() helper
> anyway, to reduce the chance that someone will copy paste this pattern to
> other parts of the driver.
>
> Note that in this specific case, we still need to keep the check that the
> device is not behind an expander, because we do not want to clear
> attached_phy of the expander if a device behind the expander disappears
> (as that would disable all the other devices behind the expander).
>
> However, if it is the expander itself that disappears, attached_phy will
> be cleared, just like it would for any other directly attached device.
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
Acked-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/scsi/pm8001/pm8001_sas.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index c5354263b45e..6a8d35aea93a 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -780,8 +780,11 @@ static void pm8001_dev_gone_notify(struct domain_device *dev)
>                  * The phy array only contains local phys. Thus, we cannot clear
>                  * phy_attached for a device behind an expander.
>                  */
> -               if (!dev_parent_is_expander(dev))
> -                       pm8001_ha->phy[pm8001_dev->attached_phy].phy_attached = 0;
> +               if (!dev_parent_is_expander(dev)) {
> +                       u32 phy_id = pm80xx_get_local_phy_id(dev);
> +
> +                       pm8001_ha->phy[phy_id].phy_attached = 0;
> +               }
>                 pm8001_free_dev(pm8001_dev);
>         } else {
>                 pm8001_dbg(pm8001_ha, DISC, "Found dev has gone.\n");
> --
> 2.50.1
>

Re: [PATCH v2 00/10] scsi: pm80xx: Fix expander support

[Martin K. Petersen]

Niklas,

> Some recent patches broke expander support for the pm80xx driver.

Applied to 6.18/scsi-staging, thanks!

-- 
Martin K. Petersen

Re: [PATCH v2 00/10] scsi: pm80xx: Fix expander support

[Martin K. Petersen]

On Thu, 14 Aug 2025 19:32:15 +0200, Niklas Cassel wrote:

> Some recent patches broke expander support for the pm80xx driver.
> 
> The first two patches in this series make sure that expanders work with
> the pm80xx driver again.
> 
> It also fixes a bug in pm8001_abort_task() that was found through code
> review.
> 
> [...]

Applied to 6.18/scsi-queue, thanks!

[01/10] scsi: pm80xx: Restore support for expanders
        https://git.kernel.org/mkp/scsi/c/eeee1086073e
[02/10] scsi: pm80xx: Fix array-index-out-of-of-bounds on rmmod
        https://git.kernel.org/mkp/scsi/c/251be2f6037f
[03/10] scsi: libsas: Add dev_parent_is_expander() helper
        https://git.kernel.org/mkp/scsi/c/e5eb72c92eb7
[04/10] scsi: hisi_sas: Use dev_parent_is_expander() helper
        https://git.kernel.org/mkp/scsi/c/0c0188dd200e
[05/10] scsi: isci: Use dev_parent_is_expander() helper
        https://git.kernel.org/mkp/scsi/c/ad6ae22927a7
[06/10] scsi: mvsas: Use dev_parent_is_expander() helper
        https://git.kernel.org/mkp/scsi/c/3adf77948983
[07/10] scsi: pm80xx: Use dev_parent_is_expander() helper
        https://git.kernel.org/mkp/scsi/c/35e388696c3f
[08/10] scsi: pm80xx: Add helper function to get the local phy id
        https://git.kernel.org/mkp/scsi/c/b4ec98303f9f
[09/10] scsi: pm80xx: Fix pm8001_abort_task() for chip_8006 when using an expander
        https://git.kernel.org/mkp/scsi/c/ad70c6bc776b
[10/10] scsi: pm80xx: Use pm80xx_get_local_phy_id() to access phy array
        https://git.kernel.org/mkp/scsi/c/03f69351b63e

-- 
Martin K. Petersen