[PATCH v3 0/3] scsi: hisi_sas: Fixed IO error caused by port id not updated

[PATCH v3 0/3] scsi: hisi_sas: Fixed IO error caused by port id not updated

[Xingui Yang]

This series of patches is used to solve the problem that after the SATA
device port ID is changed, IO may be sent to the incorrect disk.

Changes from v2:
- Use asynchronous scheduling

Changes from v1:
- Fix "BUG: Atomic scheduling in clear_itct_v3_hw()"

Xingui Yang (3):
  scsi: hisi_sas: Enable force phy when SATA disk directly connected
  scsi: libsas: Move sas_put_device() to libsas.h
  scsi: hisi_sas: Fixed IO error caused by port id not updated

 drivers/scsi/hisi_sas/hisi_sas_main.c  | 51 ++++++++++++++++++++++++++
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  9 ++++-
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 14 ++++++-
 drivers/scsi/libsas/sas_discover.c     |  1 +
 drivers/scsi/libsas/sas_internal.h     |  6 ---
 include/scsi/libsas.h                  |  6 +++
 6 files changed, 77 insertions(+), 10 deletions(-)

-- 
2.33.0

[PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[Xingui Yang]

the SAS controller determines the disk to which I/Os are delivered based
on the port id in the DQ entry when SATA disk directly connected.

When many phys were disconnected immediately and connected again during
I/O sending and port id of phys were changed and used by other link, I/O
may be sent to incorrect disk and data inconsistency on the SATA disk may
occur during I/O retry with the old port id. So enable force phy, then
force the command to be executed in a certain phy, and if the actual phy
id of the port does not match the phy configured in the command, the chip
will stop delivering the I/O to disk.

Fixes: ce60689e12dd ("scsi: hisi_sas: add v3 code to send ATA frame")
Signed-off-by: Xingui Yang <yangxingui@huawei.com>
Reviewed-by: Yihang Li <liyihang9@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  9 +++++++--
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 14 ++++++++++++--
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 6e7f99fcc824..3af991cad07e 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -2501,6 +2501,7 @@ static void prep_ata_v2_hw(struct hisi_hba *hisi_hba,
 	struct hisi_sas_port *port = to_hisi_sas_port(sas_port);
 	struct sas_ata_task *ata_task = &task->ata_task;
 	struct sas_tmf_task *tmf = slot->tmf;
+	int phy_id;
 	u8 *buf_cmd;
 	int has_data = 0, hdr_tag = 0;
 	u32 dw0, dw1 = 0, dw2 = 0;
@@ -2508,10 +2509,14 @@ 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 (parent_dev && dev_is_expander(parent_dev->dev_type)) {
 		dw0 |= 3 << CMD_HDR_CMD_OFF;
-	else
+	} else {
+		phy_id = device->phy->identify.phy_identifier;
+		dw0 |= (1U << phy_id) << CMD_HDR_PHY_ID_OFF;
+		dw0 |= CMD_HDR_FORCE_PHY_MSK;
 		dw0 |= 4 << CMD_HDR_CMD_OFF;
+	}
 
 	if (tmf && ata_task->force_phy) {
 		dw0 |= CMD_HDR_FORCE_PHY_MSK;
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 095bbf80c34e..6a0656f3b596 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -359,6 +359,10 @@
 #define CMD_HDR_RESP_REPORT_MSK		(0x1 << CMD_HDR_RESP_REPORT_OFF)
 #define CMD_HDR_TLR_CTRL_OFF		6
 #define CMD_HDR_TLR_CTRL_MSK		(0x3 << CMD_HDR_TLR_CTRL_OFF)
+#define CMD_HDR_PHY_ID_OFF		8
+#define CMD_HDR_PHY_ID_MSK		(0x1ff << CMD_HDR_PHY_ID_OFF)
+#define CMD_HDR_FORCE_PHY_OFF		17
+#define CMD_HDR_FORCE_PHY_MSK		(0x1U << CMD_HDR_FORCE_PHY_OFF)
 #define CMD_HDR_PORT_OFF		18
 #define CMD_HDR_PORT_MSK		(0xf << CMD_HDR_PORT_OFF)
 #define CMD_HDR_PRIORITY_OFF		27
@@ -1429,15 +1433,21 @@ static void prep_ata_v3_hw(struct hisi_hba *hisi_hba,
 	struct hisi_sas_cmd_hdr *hdr = slot->cmd_hdr;
 	struct asd_sas_port *sas_port = device->port;
 	struct hisi_sas_port *port = to_hisi_sas_port(sas_port);
+	int phy_id;
 	u8 *buf_cmd;
 	int has_data = 0, hdr_tag = 0;
 	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 (parent_dev && dev_is_expander(parent_dev->dev_type)) {
 		hdr->dw0 |= cpu_to_le32(3 << CMD_HDR_CMD_OFF);
-	else
+	} else {
+		phy_id = device->phy->identify.phy_identifier;
+		hdr->dw0 |= cpu_to_le32((1U << phy_id)
+				<< CMD_HDR_PHY_ID_OFF);
+		hdr->dw0 |= CMD_HDR_FORCE_PHY_MSK;
 		hdr->dw0 |= cpu_to_le32(4U << CMD_HDR_CMD_OFF);
+	}
 
 	switch (task->data_dir) {
 	case DMA_TO_DEVICE:
-- 
2.33.0

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 20/02/2025 13:05, Xingui Yang wrote:

-john.garry@huawei.com (this has not worked in over 2 years ...)

> the SAS controller determines the disk to which I/Os are delivered based
> on the port id in the DQ entry when SATA disk directly connected.
> 
> When many phys were disconnected immediately and connected again during
> I/O sending and port id of phys were changed and used by other link, I/O
> may be sent to incorrect disk and data inconsistency on the SATA disk may


So is the disk reported gone (from libsas point-of-view) after you 
unplug? If not, why not?

Thanks,
John

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/2/21 1:35, John Garry wrote:
> On 20/02/2025 13:05, Xingui Yang wrote:
> 
> -john.garry@huawei.com (this has not worked in over 2 years ...)
Sorry, I used the wrong one.
> 
>> the SAS controller determines the disk to which I/Os are delivered based
>> on the port id in the DQ entry when SATA disk directly connected.
>>
>> When many phys were disconnected immediately and connected again during
>> I/O sending and port id of phys were changed and used by other link, I/O
>> may be sent to incorrect disk and data inconsistency on the SATA disk may
> 
> 
> So is the disk reported gone (from libsas point-of-view) after you 
> unplug? If not, why not?

The problem may occur in a scenario where multiple SATA disks are 
inserted almost at the same time. When phy reset is executed in error 
processing, other phys are also up, which may cause the hw port id 
corresponding to the phy to change. The log is as follows:

[ 4588.608924] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy2 link_rate=10(sata)
[ 4588.609039] sas: phy-8:2 added to port-8:4, phy_mask:0x4 
(5000000000000802)
[ 4588.609267] sas: DOING DISCOVERY on port 4, pid:69294
[ 4588.609276] hisi_sas_v3_hw 0000:b4:02.0: dev[13:5] found
[ 4588.671362] sas: ata40: end_device-8:4: dev error handler
[ 4588.846387] hisi_sas_v3_hw 0000:b4:02.0: phydown: phy2 phy_state=0xc3 
// phy2's hw port id assign by chip is released
[ 4588.846393] hisi_sas_v3_hw 0000:b4:02.0: ignore flutter phy2 down
[ 4588.919837] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy3 
link_rate=10(sata) // phy3 is assigned the hw port id previously used by 
phy2
[ 4589.029656] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy2 
link_rate=10(sata) // phy2's hw port id is assigned a new one
[ 4589.220662] ata40.00: ATA-9: HUH721010ALE600, T3C0, max UDMA/133
[ 4589.220666] ata40.00: 19532873728 sectors, multi 0: LBA48 NCQ (depth 
32), AA
[ 4589.233022] ata40.00: configured for UDMA/133

In view of the situation corresponding to the above log, the 
hisi_sas_port.id corresponding to phy2 has not been updated, and the old 
port id is still used, which will cause the IO delivered to phy2 to be 
abnormally delivered to the disk of phy3.

After force phy, the chip will check whether the phy information matches 
the port id and intercept this abnormal IO.

Thanks.
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 21/02/2025 01:59, yangxingui wrote:
> Hi, John
> 
> On 2025/2/21 1:35, John Garry wrote:
>> On 20/02/2025 13:05, Xingui Yang wrote:
>>
>> -john.garry@huawei.com (this has not worked in over 2 years ...)
> Sorry, I used the wrong one.
>>
>>> the SAS controller determines the disk to which I/Os are delivered based
>>> on the port id in the DQ entry when SATA disk directly connected.
>>>
>>> When many phys were disconnected immediately and connected again during
>>> I/O sending and port id of phys were changed and used by other link, I/O
>>> may be sent to incorrect disk and data inconsistency on the SATA disk 
>>> may
>>
>>
>> So is the disk reported gone (from libsas point-of-view) after you 
>> unplug? If not, why not?
> 
> The problem may occur in a scenario where multiple SATA disks are 
> inserted almost at the same time. When phy reset is executed in error 
> processing, other phys are also up, which may cause the hw port id 
> corresponding to the phy to change. The log is as follows:
> 
> [ 4588.608924] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy2 link_rate=10(sata)
> [ 4588.609039] sas: phy-8:2 added to port-8:4, phy_mask:0x4 
> (5000000000000802)
> [ 4588.609267] sas: DOING DISCOVERY on port 4, pid:69294
> [ 4588.609276] hisi_sas_v3_hw 0000:b4:02.0: dev[13:5] found
> [ 4588.671362] sas: ata40: end_device-8:4: dev error handler
> [ 4588.846387] hisi_sas_v3_hw 0000:b4:02.0: phydown: phy2 
> phy_state=0xc3 // phy2's hw port id assign by chip is released
> [ 4588.846393] hisi_sas_v3_hw 0000:b4:02.0: ignore flutter phy2 down
> [ 4588.919837] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy3 
> link_rate=10(sata) // phy3 is assigned the hw port id previously used by 
> phy2
> [ 4589.029656] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy2 
> link_rate=10(sata) // phy2's hw port id is assigned a new one
> [ 4589.220662] ata40.00: ATA-9: HUH721010ALE600, T3C0, max UDMA/133
> [ 4589.220666] ata40.00: 19532873728 sectors, multi 0: LBA48 NCQ (depth 
> 32), AA
> [ 4589.233022] ata40.00: configured for UDMA/133
> 
> In view of the situation corresponding to the above log, the 
> hisi_sas_port.id corresponding to phy2 has not been updated, and the old 
> port id is still used, which will cause the IO delivered to phy2 to be 
> abnormally delivered to the disk of phy3.
> 
> After force phy, the chip will check whether the phy information matches 
> the port id and intercept this abnormal IO.
> 


So do you mean that all IO to this disk will error? If yes, then this is 
good.

But I still don't like the handling in this patch. If we get a phy up, 
then the directly-attached disk ideally should be gone already, so 
should not have to do this handling.

Thanks,
John

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi Jonh,

On 2025/2/24 16:29, John Garry wrote:
> On 21/02/2025 01:59, yangxingui wrote:
>> Hi, John
>>
>> On 2025/2/21 1:35, John Garry wrote:
>>> On 20/02/2025 13:05, Xingui Yang wrote:
>>>
>>> -john.garry@huawei.com (this has not worked in over 2 years ...)
>> Sorry, I used the wrong one.
>>>
>>>> the SAS controller determines the disk to which I/Os are delivered 
>>>> based
>>>> on the port id in the DQ entry when SATA disk directly connected.
>>>>
>>>> When many phys were disconnected immediately and connected again during
>>>> I/O sending and port id of phys were changed and used by other link, 
>>>> I/O
>>>> may be sent to incorrect disk and data inconsistency on the SATA 
>>>> disk may
>>>
>>>
>>> So is the disk reported gone (from libsas point-of-view) after you 
>>> unplug? If not, why not?
>>
>> The problem may occur in a scenario where multiple SATA disks are 
>> inserted almost at the same time. When phy reset is executed in error 
>> processing, other phys are also up, which may cause the hw port id 
>> corresponding to the phy to change. The log is as follows:
>>
>> [ 4588.608924] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy2 
>> link_rate=10(sata)
>> [ 4588.609039] sas: phy-8:2 added to port-8:4, phy_mask:0x4 
>> (5000000000000802)
>> [ 4588.609267] sas: DOING DISCOVERY on port 4, pid:69294
>> [ 4588.609276] hisi_sas_v3_hw 0000:b4:02.0: dev[13:5] found
>> [ 4588.671362] sas: ata40: end_device-8:4: dev error handler
>> [ 4588.846387] hisi_sas_v3_hw 0000:b4:02.0: phydown: phy2 
>> phy_state=0xc3 // phy2's hw port id assign by chip is released
>> [ 4588.846393] hisi_sas_v3_hw 0000:b4:02.0: ignore flutter phy2 down
>> [ 4588.919837] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy3 
>> link_rate=10(sata) // phy3 is assigned the hw port id previously used 
>> by phy2
>> [ 4589.029656] hisi_sas_v3_hw 0000:b4:02.0: phyup: phy2 
>> link_rate=10(sata) // phy2's hw port id is assigned a new one
>> [ 4589.220662] ata40.00: ATA-9: HUH721010ALE600, T3C0, max UDMA/133
>> [ 4589.220666] ata40.00: 19532873728 sectors, multi 0: LBA48 NCQ 
>> (depth 32), AA
>> [ 4589.233022] ata40.00: configured for UDMA/133
>>
>> In view of the situation corresponding to the above log, the 
>> hisi_sas_port.id corresponding to phy2 has not been updated, and the 
>> old port id is still used, which will cause the IO delivered to phy2 
>> to be abnormally delivered to the disk of phy3.
>>
>> After force phy, the chip will check whether the phy information 
>> matches the port id and intercept this abnormal IO.
>>
> 
> 
> So do you mean that all IO to this disk will error? If yes, then this is 
> good.
Yes, IO error or IO result does not meet expectations. As shown in the 
log below, due to an abnormal port ID, the SNs of the two disks read are 
the same.

[448000.504979] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 link_rate=10(sata)
[448000.505070] sas: phy-10:1 added to port-10:1, phy_mask:0x2 
(5000000000000a01)
[448000.505247] sas: DOING DISCOVERY on port 1, pid:2239187
[448000.505255] hisi_sas_v3_hw 0000:d4:02.0: dev[2:5] found
[448000.505274] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[448000.505295] sas: ata31: end_device-10:0: dev error handler
[448000.505299] sas: ata32: end_device-10:1: dev error handler
[448001.300517] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy1 phy_state=0x1 
  // phy1's hw port id released
[448001.300522] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy1 down
[448001.436187] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
link_rate=10(sata) // phy2 occupies the hardware port ID of phy1
[448001.608766] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
link_rate=10(sata) // phy1 was assigned a new hardware port ID
[448001.775605] ata32.00: ATA-11: WUH721816ALE6L4, PCGAW660, max UDMA/133
[448002.159364] sas: phy-10:2 added to port-10:2, phy_mask:0x4 
(5000000000000a02)
[448002.159575] sas: DOING DISCOVERY on port 2, pid:2239187
[448002.159581] hisi_sas_v3_hw 0000:d4:02.0: dev[3:5] found
[448002.159602] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[448002.159623] sas: ata31: end_device-10:0: dev error handler
[448002.159633] sas: ata32: end_device-10:1: dev error handler
[448002.159636] sas: ata33: end_device-10:2: dev error handler
[448002.393349] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy2 phy_state=0x3
[448002.393354] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy2 down
[448002.684937] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 link_rate=10(sata)
[448002.851639] ata33.00: ATA-11: WUH721816ALE6L4, PCGAW660, max UDMA/133
[448002.851644] ata33.00: 31251759104 sectors, multi 0: LBA48 NCQ (depth 32)

> 
> But I still don't like the handling in this patch. If we get a phy up, 
> then the directly-attached disk ideally should be gone already, so 
> should not have to do this handling.
There is no problem when the disk is removed. The current problem is 
that multiple phy up at the same time. When one of the phys up and 
enters error handler to execute hardreset, the phy will down and then 
up. other phy up will probably occupy the hw port id of the previous phy 
which do hardreset in EH.

Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 24/02/2025 09:36, yangxingui wrote:
>>
>>
>> So do you mean that all IO to this disk will error? If yes, then this 
>> is good.
> Yes, IO error or IO result does not meet expectations. As shown in the 
> log below, due to an abnormal port ID, the SNs of the two disks read are 
> the same.

Do you mean that this is mainline kernel behaviour, below:

> 
> [448000.504979] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 link_rate=10(sata)
> [448000.505070] sas: phy-10:1 added to port-10:1, phy_mask:0x2 
> (5000000000000a01)
> [448000.505247] sas: DOING DISCOVERY on port 1, pid:2239187
> [448000.505255] hisi_sas_v3_hw 0000:d4:02.0: dev[2:5] found
> [448000.505274] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
> [448000.505295] sas: ata31: end_device-10:0: dev error handler
> [448000.505299] sas: ata32: end_device-10:1: dev error handler
> [448001.300517] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy1 phy_state=0x1 
>   // phy1's hw port id released
> [448001.300522] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy1 down
> [448001.436187] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
> link_rate=10(sata) // phy2 occupies the hardware port ID of phy1
> [448001.608766] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
> link_rate=10(sata) // phy1 was assigned a new hardware port ID
> [448001.775605] ata32.00: ATA-11: WUH721816ALE6L4, PCGAW660, max UDMA/133
> [448002.159364] sas: phy-10:2 added to port-10:2, phy_mask:0x4 
> (5000000000000a02)
> [448002.159575] sas: DOING DISCOVERY on port 2, pid:2239187
> [448002.159581] hisi_sas_v3_hw 0000:d4:02.0: dev[3:5] found
> [448002.159602] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
> [448002.159623] sas: ata31: end_device-10:0: dev error handler
> [448002.159633] sas: ata32: end_device-10:1: dev error handler
> [448002.159636] sas: ata33: end_device-10:2: dev error handler
> [448002.393349] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy2 phy_state=0x3
> [448002.393354] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy2 down
> [448002.684937] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 link_rate=10(sata)
> [448002.851639] ata33.00: ATA-11: WUH721816ALE6L4, PCGAW660, max UDMA/133
> [448002.851644] ata33.00: 31251759104 sectors, multi 0: LBA48 NCQ (depth 
> 32)
> 
>>
>> But I still don't like the handling in this patch. If we get a phy up, 
>> then the directly-attached disk ideally should be gone already, so 
>> should not have to do this handling.
> There is no problem when the disk is removed. The current problem is 
> that multiple phy up at the same time. When one of the phys up and 
> enters error handler to execute hardreset, the phy will down and then 
> up. other phy up will probably occupy the hw port id of the previous phy 
> which do hardreset in EH.

Could you do this work (itct update) in lldd_ata_check_ready CB?

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/2/24 20:21, John Garry wrote:
> On 24/02/2025 09:36, yangxingui wrote:
>>>
>>>
>>> So do you mean that all IO to this disk will error? If yes, then this 
>>> is good.
>> Yes, IO error or IO result does not meet expectations. As shown in the 
>> log below, due to an abnormal port ID, the SNs of the two disks read 
>> are the same.
> 
> Do you mean that this is mainline kernel behaviour, below:
Yes
> 
>>
>> [448000.504979] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
>> link_rate=10(sata)
>> [448000.505070] sas: phy-10:1 added to port-10:1, phy_mask:0x2 
>> (5000000000000a01)
>> [448000.505247] sas: DOING DISCOVERY on port 1, pid:2239187
>> [448000.505255] hisi_sas_v3_hw 0000:d4:02.0: dev[2:5] found
>> [448000.505274] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
>> [448000.505295] sas: ata31: end_device-10:0: dev error handler
>> [448000.505299] sas: ata32: end_device-10:1: dev error handler
>> [448001.300517] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy1 
>> phy_state=0x1   // phy1's hw port id released
>> [448001.300522] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy1 down
>> [448001.436187] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
>> link_rate=10(sata) // phy2 occupies the hardware port ID of phy1
>> [448001.608766] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
>> link_rate=10(sata) // phy1 was assigned a new hardware port ID
>> [448001.775605] ata32.00: ATA-11: WUH721816ALE6L4, PCGAW660, max UDMA/133
>> [448002.159364] sas: phy-10:2 added to port-10:2, phy_mask:0x4 
>> (5000000000000a02)
>> [448002.159575] sas: DOING DISCOVERY on port 2, pid:2239187
>> [448002.159581] hisi_sas_v3_hw 0000:d4:02.0: dev[3:5] found
>> [448002.159602] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
>> [448002.159623] sas: ata31: end_device-10:0: dev error handler
>> [448002.159633] sas: ata32: end_device-10:1: dev error handler
>> [448002.159636] sas: ata33: end_device-10:2: dev error handler
>> [448002.393349] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy2 phy_state=0x3
>> [448002.393354] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy2 down
>> [448002.684937] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
>> link_rate=10(sata)
>> [448002.851639] ata33.00: ATA-11: WUH721816ALE6L4, PCGAW660, max UDMA/133
>> [448002.851644] ata33.00: 31251759104 sectors, multi 0: LBA48 NCQ 
>> (depth 32)
>>
>>>
>>> But I still don't like the handling in this patch. If we get a phy 
>>> up, then the directly-attached disk ideally should be gone already, 
>>> so should not have to do this handling.
>> There is no problem when the disk is removed. The current problem is 
>> that multiple phy up at the same time. When one of the phys up and 
>> enters error handler to execute hardreset, the phy will down and then 
>> up. other phy up will probably occupy the hw port id of the previous 
>> phy which do hardreset in EH.
> 
> Could you do this work (itct update) in lldd_ata_check_ready CB?

It's a good idea only for sata disks, but the current problem is not 
only the scenario of connecting the sata disk. This phenomenon 
occasionally occurs when the SAS disk is connected after the controller 
is reset. The following is the log of the stress test recurrence after 
incorporating the current repair patch. Although we called 
hisi_sas_refresh_port_id() on controller reset.

[ 5387.235015] hisi_sas_v3_hw 0000:74:02.0: I_T nexus reset: internal 
abort (-5)
[ 5387.242126] sas: clear nexus ha
[ 5387.245283] hisi_sas_v3_hw 0000:74:02.0: controller resetting...
[ 5388.908489] hisi_sas_v3_hw 0000:74:02.0: phyup: phy5 link_rate=10(sata)
[ 5388.915090] hisi_sas_v3_hw 0000:74:02.0: phyup: phy6 link_rate=10(sata)
[ 5388.934505] hisi_sas_v3_hw 0000:74:02.0: phyup: phy0 link_rate=9(sata)
[ 5388.941009] hisi_sas_v3_hw 0000:74:02.0: phyup: phy1 link_rate=9(sata)
[ 5388.950976] hisi_sas_v3_hw 0000:74:02.0: phyup: phy4 link_rate=11
[ 5388.957048] hisi_sas_v3_hw 0000:74:02.0: phyup: phy7 link_rate=11
[ 5388.980097] hisi_sas_v3_hw 0000:74:02.0: phyup: phy2 link_rate=11
[ 5388.986169] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=11 // 
phy3 attached a sas disk.
[ 5389.065103] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
attach device
[ 5389.072409] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
[ 5389.078492] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
attach device
[ 5389.085780] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
[ 5389.091861] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
attach device
[ 5389.099146] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
[ 5389.107419] hisi_sas_v3_hw 0000:74:02.0: controller reset complete 
// controller reset finished
[ 5389.113686] hisi_sas_v3_hw 0000:74:02.0: phydown: phy0 phy_state=0xfe
[ 5389.120099] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy0 down
[ 5389.136399] hisi_sas_v3_hw 0000:74:02.0: phy3's hw port id changed 
from 1 to 7
[ 5389.308114] hisi_sas_v3_hw 0000:74:02.0: phyup: phy0 link_rate=9(sata)

Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 24/02/2025 13:12, yangxingui wrote:
> Hi, John
> 
> On 2025/2/24 20:21, John Garry wrote:
>> On 24/02/2025 09:36, yangxingui wrote:
>>>>
>>>>
>>>> So do you mean that all IO to this disk will error? If yes, then 
>>>> this is good.
>>> Yes, IO error or IO result does not meet expectations. As shown in 
>>> the log below, due to an abnormal port ID, the SNs of the two disks 
>>> read are the same.
>>
>> Do you mean that this is mainline kernel behaviour, below:
> Yes
>>
>>>
>>> [448000.504979] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
>>> link_rate=10(sata)
>>> [448000.505070] sas: phy-10:1 added to port-10:1, phy_mask:0x2 
>>> (5000000000000a01)
>>> [448000.505247] sas: DOING DISCOVERY on port 1, pid:2239187
>>> [448000.505255] hisi_sas_v3_hw 0000:d4:02.0: dev[2:5] found
>>> [448000.505274] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
>>> [448000.505295] sas: ata31: end_device-10:0: dev error handler
>>> [448000.505299] sas: ata32: end_device-10:1: dev error handler
>>> [448001.300517] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy1 
>>> phy_state=0x1   // phy1's hw port id released
>>> [448001.300522] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy1 down
>>> [448001.436187] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
>>> link_rate=10(sata) // phy2 occupies the hardware port ID of phy1
>>> [448001.608766] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
>>> link_rate=10(sata) // phy1 was assigned a new hardware port ID
>>> [448001.775605] ata32.00: ATA-11: WUH721816ALE6L4, PCGAW660, max 
>>> UDMA/133
>>> [448002.159364] sas: phy-10:2 added to port-10:2, phy_mask:0x4 
>>> (5000000000000a02)
>>> [448002.159575] sas: DOING DISCOVERY on port 2, pid:2239187
>>> [448002.159581] hisi_sas_v3_hw 0000:d4:02.0: dev[3:5] found
>>> [448002.159602] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
>>> [448002.159623] sas: ata31: end_device-10:0: dev error handler
>>> [448002.159633] sas: ata32: end_device-10:1: dev error handler
>>> [448002.159636] sas: ata33: end_device-10:2: dev error handler
>>> [448002.393349] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy2 phy_state=0x3
>>> [448002.393354] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy2 down
>>> [448002.684937] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
>>> link_rate=10(sata)
>>> [448002.851639] ata33.00: ATA-11: WUH721816ALE6L4, PCGAW660, max 
>>> UDMA/133
>>> [448002.851644] ata33.00: 31251759104 sectors, multi 0: LBA48 NCQ 
>>> (depth 32)
>>>
>>>>
>>>> But I still don't like the handling in this patch. If we get a phy 
>>>> up, then the directly-attached disk ideally should be gone already, 
>>>> so should not have to do this handling.
>>> There is no problem when the disk is removed. The current problem is 
>>> that multiple phy up at the same time. When one of the phys up and 
>>> enters error handler to execute hardreset, the phy will down and then 
>>> up. other phy up will probably occupy the hw port id of the previous 
>>> phy which do hardreset in EH.
>>
>> Could you do this work (itct update) in lldd_ata_check_ready CB?
> 
> It's a good idea only for sata disks, but the current problem is not 
> only the scenario of connecting the sata disk. This phenomenon 
> occasionally occurs when the SAS disk is connected after the controller 
> is reset. The following is the log of the stress test recurrence after 
> incorporating the current repair patch. Although we called 
> hisi_sas_refresh_port_id() on controller reset.
> 
> [ 5387.235015] hisi_sas_v3_hw 0000:74:02.0: I_T nexus reset: internal 
> abort (-5)
> [ 5387.242126] sas: clear nexus ha
> [ 5387.245283] hisi_sas_v3_hw 0000:74:02.0: controller resetting...
> [ 5388.908489] hisi_sas_v3_hw 0000:74:02.0: phyup: phy5 link_rate=10(sata)
> [ 5388.915090] hisi_sas_v3_hw 0000:74:02.0: phyup: phy6 link_rate=10(sata)
> [ 5388.934505] hisi_sas_v3_hw 0000:74:02.0: phyup: phy0 link_rate=9(sata)
> [ 5388.941009] hisi_sas_v3_hw 0000:74:02.0: phyup: phy1 link_rate=9(sata)
> [ 5388.950976] hisi_sas_v3_hw 0000:74:02.0: phyup: phy4 link_rate=11
> [ 5388.957048] hisi_sas_v3_hw 0000:74:02.0: phyup: phy7 link_rate=11
> [ 5388.980097] hisi_sas_v3_hw 0000:74:02.0: phyup: phy2 link_rate=11
> [ 5388.986169] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=11 // 
> phy3 attached a sas disk.
> [ 5389.065103] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
> attach device
> [ 5389.072409] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
> [ 5389.078492] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
> attach device
> [ 5389.085780] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
> [ 5389.091861] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
> attach device
> [ 5389.099146] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
> [ 5389.107419] hisi_sas_v3_hw 0000:74:02.0: controller reset complete // 
> controller reset finished
> [ 5389.113686] hisi_sas_v3_hw 0000:74:02.0: phydown: phy0 phy_state=0xfe
> [ 5389.120099] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy0 down
> [ 5389.136399] hisi_sas_v3_hw 0000:74:02.0: phy3's hw port id changed 
> from 1 to 7
> [ 5389.308114] hisi_sas_v3_hw 0000:74:02.0: phyup: phy0 link_rate=9(sata)
> 


pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) link 
reset errors - can you consider doing that in hisi_sas_update_port_id() 
when you find an inconstant port id?

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

On 2025/2/25 1:34, John Garry wrote:
> On 24/02/2025 13:12, yangxingui wrote:
>> Hi, John
>>
>> On 2025/2/24 20:21, John Garry wrote:
>>> On 24/02/2025 09:36, yangxingui wrote:
>>>>>
>>>>>
>>>>> So do you mean that all IO to this disk will error? If yes, then 
>>>>> this is good.
>>>> Yes, IO error or IO result does not meet expectations. As shown in 
>>>> the log below, due to an abnormal port ID, the SNs of the two disks 
>>>> read are the same.
>>>
>>> Do you mean that this is mainline kernel behaviour, below:
>> Yes
>>>
>>>>
>>>> [448000.504979] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
>>>> link_rate=10(sata)
>>>> [448000.505070] sas: phy-10:1 added to port-10:1, phy_mask:0x2 
>>>> (5000000000000a01)
>>>> [448000.505247] sas: DOING DISCOVERY on port 1, pid:2239187
>>>> [448000.505255] hisi_sas_v3_hw 0000:d4:02.0: dev[2:5] found
>>>> [448000.505274] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
>>>> [448000.505295] sas: ata31: end_device-10:0: dev error handler
>>>> [448000.505299] sas: ata32: end_device-10:1: dev error handler
>>>> [448001.300517] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy1 
>>>> phy_state=0x1   // phy1's hw port id released
>>>> [448001.300522] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy1 down
>>>> [448001.436187] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
>>>> link_rate=10(sata) // phy2 occupies the hardware port ID of phy1
>>>> [448001.608766] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy1 
>>>> link_rate=10(sata) // phy1 was assigned a new hardware port ID
>>>> [448001.775605] ata32.00: ATA-11: WUH721816ALE6L4, PCGAW660, max 
>>>> UDMA/133
>>>> [448002.159364] sas: phy-10:2 added to port-10:2, phy_mask:0x4 
>>>> (5000000000000a02)
>>>> [448002.159575] sas: DOING DISCOVERY on port 2, pid:2239187
>>>> [448002.159581] hisi_sas_v3_hw 0000:d4:02.0: dev[3:5] found
>>>> [448002.159602] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
>>>> [448002.159623] sas: ata31: end_device-10:0: dev error handler
>>>> [448002.159633] sas: ata32: end_device-10:1: dev error handler
>>>> [448002.159636] sas: ata33: end_device-10:2: dev error handler
>>>> [448002.393349] hisi_sas_v3_hw 0000:d4:02.0: phydown: phy2 
>>>> phy_state=0x3
>>>> [448002.393354] hisi_sas_v3_hw 0000:d4:02.0: ignore flutter phy2 down
>>>> [448002.684937] hisi_sas_v3_hw 0000:d4:02.0: phyup: phy2 
>>>> link_rate=10(sata)
>>>> [448002.851639] ata33.00: ATA-11: WUH721816ALE6L4, PCGAW660, max 
>>>> UDMA/133
>>>> [448002.851644] ata33.00: 31251759104 sectors, multi 0: LBA48 NCQ 
>>>> (depth 32)
>>>>
>>>>>
>>>>> But I still don't like the handling in this patch. If we get a phy 
>>>>> up, then the directly-attached disk ideally should be gone already, 
>>>>> so should not have to do this handling.
>>>> There is no problem when the disk is removed. The current problem is 
>>>> that multiple phy up at the same time. When one of the phys up and 
>>>> enters error handler to execute hardreset, the phy will down and 
>>>> then up. other phy up will probably occupy the hw port id of the 
>>>> previous phy which do hardreset in EH.
>>>
>>> Could you do this work (itct update) in lldd_ata_check_ready CB?
>>
>> It's a good idea only for sata disks, but the current problem is not 
>> only the scenario of connecting the sata disk. This phenomenon 
>> occasionally occurs when the SAS disk is connected after the 
>> controller is reset. The following is the log of the stress test 
>> recurrence after incorporating the current repair patch. Although we 
>> called hisi_sas_refresh_port_id() on controller reset.
>>
>> [ 5387.235015] hisi_sas_v3_hw 0000:74:02.0: I_T nexus reset: internal 
>> abort (-5)
>> [ 5387.242126] sas: clear nexus ha
>> [ 5387.245283] hisi_sas_v3_hw 0000:74:02.0: controller resetting...
>> [ 5388.908489] hisi_sas_v3_hw 0000:74:02.0: phyup: phy5 
>> link_rate=10(sata)
>> [ 5388.915090] hisi_sas_v3_hw 0000:74:02.0: phyup: phy6 
>> link_rate=10(sata)
>> [ 5388.934505] hisi_sas_v3_hw 0000:74:02.0: phyup: phy0 link_rate=9(sata)
>> [ 5388.941009] hisi_sas_v3_hw 0000:74:02.0: phyup: phy1 link_rate=9(sata)
>> [ 5388.950976] hisi_sas_v3_hw 0000:74:02.0: phyup: phy4 link_rate=11
>> [ 5388.957048] hisi_sas_v3_hw 0000:74:02.0: phyup: phy7 link_rate=11
>> [ 5388.980097] hisi_sas_v3_hw 0000:74:02.0: phyup: phy2 link_rate=11
>> [ 5388.986169] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=11 
>> // phy3 attached a sas disk.
>> [ 5389.065103] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
>> attach device
>> [ 5389.072409] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
>> [ 5389.078492] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
>> attach device
>> [ 5389.085780] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
>> [ 5389.091861] hisi_sas_v3_hw 0000:74:02.0: task prep: SAS port1 not 
>> attach device
>> [ 5389.099146] sas: executing TMF task failed 5000c500ae49c8f1 (-70)
>> [ 5389.107419] hisi_sas_v3_hw 0000:74:02.0: controller reset complete 
>> // controller reset finished
>> [ 5389.113686] hisi_sas_v3_hw 0000:74:02.0: phydown: phy0 phy_state=0xfe
>> [ 5389.120099] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy0 down
>> [ 5389.136399] hisi_sas_v3_hw 0000:74:02.0: phy3's hw port id changed 
>> from 1 to 7
>> [ 5389.308114] hisi_sas_v3_hw 0000:74:02.0: phyup: phy0 link_rate=9(sata)
>>
> 
> 
> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) link 
> reset errors - can you consider doing that in hisi_sas_update_port_id() 
> when you find an inconstant port id?
Currently during phyup, the hw port id may change, and the corresponding 
hisi_sas_port.id and the port id in itct are not updated synchronously. 
The problem caused is not a link error, so we don't need deform port, 
just update the port id when phyup.

Thanks.
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 25/02/2025 01:48, yangxingui wrote:
>>
>>
>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>> link reset errors - can you consider doing that in 
>> hisi_sas_update_port_id() when you find an inconstant port id?
> Currently during phyup, the hw port id may change, and the corresponding 
> hisi_sas_port.id and the port id in itct are not updated synchronously. 
> The problem caused is not a link error, so we don't need deform port, 
> just update the port id when phyup.

Sure, but I am just trying to keep this simple. If you deform and reform 
the port - and so lose and find the disk (which does the itct config) - 
will that solve the problem?

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/2/25 16:19, John Garry wrote:
> On 25/02/2025 01:48, yangxingui wrote:
>>>
>>>
>>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>>> link reset errors - can you consider doing that in 
>>> hisi_sas_update_port_id() when you find an inconstant port id?
>> Currently during phyup, the hw port id may change, and the 
>> corresponding hisi_sas_port.id and the port id in itct are not updated 
>> synchronously. The problem caused is not a link error, so we don't 
>> need deform port, just update the port id when phyup.
> 
> Sure, but I am just trying to keep this simple. If you deform and reform 
> the port - and so lose and find the disk (which does the itct config) - 
> will that solve the problem?

1、phyup ->form port -> eh -> ata reset -> found hw port change -> 
deform port -> let dev gone -> refound

2、controller reset -> phyup -> finish controller reset -> found hw port 
change -> deform port -> let dev gone -> refound

I also thought about the plan you mentioned in the early days. The above 
will make the process more complicated and retriggering phyup may result 
in a new round of port id changes. Lose and find the disk will cause the 
upper layer IO to report error when controller reset. It seems that it 
is better to make the upper layer unaware of the hw port id change when 
phyup in reset, like ata reset or controller reset. ^_^

Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 25/02/2025 09:35, yangxingui wrote:
>>>>
>>>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>>>> link reset errors - can you consider doing that in 
>>>> hisi_sas_update_port_id() when you find an inconstant port id?
>>> Currently during phyup, the hw port id may change, and the 
>>> corresponding hisi_sas_port.id and the port id in itct are not 
>>> updated synchronously. The problem caused is not a link error, so we 
>>> don't need deform port, just update the port id when phyup.
>>
>> Sure, but I am just trying to keep this simple. If you deform and 
>> reform the port - and so lose and find the disk (which does the itct 
>> config) - will that solve the problem?
> 
> 1、phyup ->form port -> eh -> ata reset -> found hw port change -> 
> deform port -> let dev gone -> refound
> 
> 2、controller reset -> phyup -> finish controller reset -> found hw port 
> change -> deform port -> let dev gone -> refound
> 
> I also thought about the plan you mentioned in the early days. The above 
> will make the process more complicated and retriggering phyup may result 
> in a new round of port id changes. Lose and find the disk will cause the 
> upper layer IO to report error when controller reset. It seems that it 
> is better to make the upper layer unaware of the hw port id change when 
> phyup in reset, like ata reset or controller reset. ^_^

The lldd_dev_found CB is where you should set the itct, and it is only 
possible to do that if you report the device gone first. So that seems 
like a simpler solution.

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/2/26 16:57, John Garry wrote:

> 
> The lldd_dev_found CB is where you should set the itct, and it is only 
> possible to do that if you report the device gone first. So that seems 
> like a simpler solution.
Solution as follow?
+static bool hisi_sas_hw_port_id_changed(struct hisi_hba *hisi_hba, int 
phy_no)
+{
+       struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];
+       struct asd_sas_phy *sas_phy = &phy->sas_phy;
+       struct device *dev = hisi_hba->dev;
+       struct asd_sas_port *sas_port;
+       struct hisi_sas_port *port;
+
+       if (!sas_phy->port)
+               return false;
+
+       sas_port = sas_phy->port;
+       port = to_hisi_sas_port(sas_port);
+       if (phy->port_id == port->id)
+               return false;
+
+       dev_info(dev, "phy%d's hw port id changed from %d to %llu\n",
+                phy_no, port->id, phy->port_id);
+
+       return true;
+}
+
  static void hisi_sas_slot_index_clear(struct hisi_hba *hisi_hba, int 
slot_idx)
  {
         void *bitmap = hisi_hba->slot_index_tags;
@@ -856,6 +878,14 @@ static void hisi_sas_phyup_work_common(struct 
work_struct *work,
         struct asd_sas_phy *sas_phy = &phy->sas_phy;
         int phy_no = sas_phy->id;

+       if (hisi_sas_hw_port_id_changed(hisi_hba, phy_no)) {
+               sas_phy_disconnected(sas_phy);
+               phy->phy_attached = 0;
+               sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR, 
GFP_ATOMIC);
+               hisi_sas_notify_phy_event(phy, HISI_PHYE_LINK_RESET);
+               return;
+       }
+

Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 27/02/2025 08:33, yangxingui wrote:
> Hi, John
> 
> On 2025/2/26 16:57, John Garry wrote:
> 
>>
>> The lldd_dev_found CB is where you should set the itct, and it is only 
>> possible to do that if you report the device gone first. So that seems 
>> like a simpler solution.

Sure, something like that - you just need to get libsas to trigger the 
proper hw port id assignment for the device. As for specific 
implementation in the LLDD, that up to you guys.

Thanks,
John

> Solution as follow?
> +static bool hisi_sas_hw_port_id_changed(struct hisi_hba *hisi_hba, int 
> phy_no)
> +{
> +       struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];
> +       struct asd_sas_phy *sas_phy = &phy->sas_phy;
> +       struct device *dev = hisi_hba->dev;
> +       struct asd_sas_port *sas_port;
> +       struct hisi_sas_port *port;
> +
> +       if (!sas_phy->port)
> +               return false;
> +
> +       sas_port = sas_phy->port;
> +       port = to_hisi_sas_port(sas_port);
> +       if (phy->port_id == port->id)
> +               return false;
> +
> +       dev_info(dev, "phy%d's hw port id changed from %d to %llu\n",
> +                phy_no, port->id, phy->port_id);
> +
> +       return true;
> +}
> +
>   static void hisi_sas_slot_index_clear(struct hisi_hba *hisi_hba, int 
> slot_idx)
>   {
>          void *bitmap = hisi_hba->slot_index_tags;
> @@ -856,6 +878,14 @@ static void hisi_sas_phyup_work_common(struct 
> work_struct *work,
>          struct asd_sas_phy *sas_phy = &phy->sas_phy;
>          int phy_no = sas_phy->id;
> 
> +       if (hisi_sas_hw_port_id_changed(hisi_hba, phy_no)) {
> +               sas_phy_disconnected(sas_phy);
> +               phy->phy_attached = 0;
> +               sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR, 
> GFP_ATOMIC);
> +               hisi_sas_notify_phy_event(phy, HISI_PHYE_LINK_RESET);
> +               return;
> +       }
> +
> 
> Thanks,
> Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/3/4 17:48, John Garry wrote:
> On 27/02/2025 08:33, yangxingui wrote:
>> Hi, John
>>
>> On 2025/2/26 16:57, John Garry wrote:
>>
>>>
>>> The lldd_dev_found CB is where you should set the itct, and it is 
>>> only possible to do that if you report the device gone first. So that 
>>> seems like a simpler solution.
> 
> Sure, something like that - you just need to get libsas to trigger the 
> proper hw port id assignment for the device. As for specific 
> implementation in the LLDD, that up to you guys.

Thank you for your suggestions. The disk was finally restored to normal, 
but the error handling took a long time. Since the error handling would 
set the host to the recovery state, other disks on the same host would 
be blocked for a long time. Log as follow:

[ 1351.602899] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1351.610892] sas: phy-1:3 added to port-1:0, phy_mask:0x8 
(5000000000000103)
[ 1351.611057] sas: DOING DISCOVERY on port 0, pid:7
[ 1351.611068] hisi_sas_v3_hw 0000:74:02.0: dev[1:5] found
[ 1351.617396] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[ 1351.618272] hisi_sas_v3_hw 0000:74:02.0: phyup: phy2 link_rate=10(sata)
[ 1351.624300] sas: ata10: end_device-1:0: dev error handler
[ 1351.670954] hisi_sas_v3_hw 0000:74:02.0: phydown: phy3 phy_state=0xf7
[ 1351.678388] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy3 down
[ 1351.685465] hisi_sas_v3_hw 0000:74:02.0: phyup: phy4 link_rate=11
[ 1351.841167] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1351.849054] hisi_sas_v3_hw 0000:74:02.0: phy3's hw port id changed 
from 0 to 1
[ 1351.857967] hisi_sas_v3_hw 0000:74:02.0: phydown: phy3 phy_state=0xf7
[ 1352.006552] hisi_sas_v3_hw 0000:74:02.0: task prep: SATA/STP port0 
not attach device
[ 1352.015679] hisi_sas_v3_hw 0000:74:02.0: task exec: failed[-70]!
[ 1352.022734] sas: lldd_execute_task returned: -70
[ 1352.022754] ata10.00: failed to IDENTIFY (I/O error, err_mask=0x40)
[ 1352.041168] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1357.218587] hisi_sas_v3_hw 0000:74:02.0: erroneous completion 
iptt=4019 task=0000000055585b33 dev id=1 addr=5000000000000103 CQ hdr: 
0x8000007 0x10fb3 0x0 0x0 Error info: 0x0 0x0 0x0 0x0
[ 1357.237298] hisi_sas_v3_hw 0000:74:02.0: task prep: SATA/STP port0 
not attach device
[ 1357.246666] hisi_sas_v3_hw 0000:74:02.0: task exec: failed[-70]!
[ 1357.253850] hisi_sas_v3_hw 0000:74:02.0: abort tmf: executing 
internal task failed: -70
[ 1357.263487] hisi_sas_v3_hw 0000:74:02.0: ata disk 5000000000000103 
reset failed
[ 1357.272487] hisi_sas_v3_hw 0000:74:02.0: phydown: phy3 phy_state=0xf7
[ 1357.280131] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy3 down
[ 1357.453175] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1357.618532] hisi_sas_v3_hw 0000:74:02.0: task prep: SATA/STP port0 
not attach device
[ 1357.627915] hisi_sas_v3_hw 0000:74:02.0: task exec: failed[-70]!
[ 1357.635086] sas: lldd_execute_task returned: -70
[ 1357.635108] ata10.00: failed to IDENTIFY (I/O error, err_mask=0x40)
[ 1362.850591] hisi_sas_v3_hw 0000:74:02.0: erroneous completion 
iptt=4020 task=0000000055585b33 dev id=1 addr=5000000000000103 CQ hdr: 
0x8000007 0x10fb4 0x0 0x0 Error info: 0x0 0x0 0x0 0x0
[ 1362.869450] hisi_sas_v3_hw 0000:74:02.0: task prep: SATA/STP port0 
not attach device
[ 1362.878890] hisi_sas_v3_hw 0000:74:02.0: task exec: failed[-70]!
[ 1362.886100] hisi_sas_v3_hw 0000:74:02.0: abort tmf: executing 
internal task failed: -70
[ 1362.895779] hisi_sas_v3_hw 0000:74:02.0: ata disk 5000000000000103 
reset failed
[ 1362.904789] hisi_sas_v3_hw 0000:74:02.0: phydown: phy3 phy_state=0xf7
[ 1362.912447] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy3 down
[ 1363.089166] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1363.254527] hisi_sas_v3_hw 0000:74:02.0: task prep: SATA/STP port0 
not attach device
[ 1363.263891] hisi_sas_v3_hw 0000:74:02.0: task exec: failed[-70]!
[ 1363.271052] sas: lldd_execute_task returned: -70
[ 1363.271077] ata10.00: failed to IDENTIFY (I/O error, err_mask=0x40)
[ 1368.482569] hisi_sas_v3_hw 0000:74:02.0: erroneous completion 
iptt=4021 task=0000000055585b33 dev id=1 addr=5000000000000103 CQ hdr: 
0x8000007 0x10fb5 0x0 0x0 Error info: 0x0 0x0 0x0 0x0
[ 1368.501395] hisi_sas_v3_hw 0000:74:02.0: task prep: SATA/STP port0 
not attach device
[ 1368.510834] hisi_sas_v3_hw 0000:74:02.0: task exec: failed[-70]!
[ 1368.518058] hisi_sas_v3_hw 0000:74:02.0: abort tmf: executing 
internal task failed: -70
[ 1368.527745] hisi_sas_v3_hw 0000:74:02.0: ata disk 5000000000000103 
reset failed
[ 1368.536750] hisi_sas_v3_hw 0000:74:02.0: phydown: phy3 phy_state=0xf7
[ 1368.544401] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy3 down
[ 1368.721173] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1368.886544] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 
tries: 1
[ 1368.896602] sas: sas_probe_sata: for direct-attached device 
5000000000000103 returned -19
[ 1368.906510] hisi_sas_v3_hw 0000:74:02.0: dev[1:5] is gone
[ 1368.913148] hisi_sas_v3_hw 0000:74:02.0: erroneous completion 
iptt=4022 task=00000000ee9f5e36 dev id=1 addr=5000000000000103 CQ hdr: 
0x8000007 0x10fb6 0x0 0x0 Error info: 0x0 0x0 0x0 0x0
[ 1368.932057] sas: DONE DISCOVERY on port 0, pid:7, result:0
[ 1369.571030] sas: phy-1:3 added to port-1:0, phy_mask:0x8 
(5000000000000103)
[ 1369.571201] sas: DOING DISCOVERY on port 0, pid:7
[ 1369.571205] hisi_sas_v3_hw 0000:74:02.0: dev[9:5] found
[ 1369.577546] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[ 1369.584543] sas: ata12: end_device-1:0: dev error handler
[ 1369.584653] sas: ata11: end_device-1:1: dev error handler
[ 1369.611048] hisi_sas_v3_hw 0000:74:02.0: phydown: phy3 phy_state=0xf7
[ 1369.618639] hisi_sas_v3_hw 0000:74:02.0: ignore flutter phy3 down
[ 1369.793175] hisi_sas_v3_hw 0000:74:02.0: phyup: phy3 link_rate=10(sata)
[ 1374.447031] ata12.00: ATA-10: ST2000NM0055-1V4104, TN05, max UDMA/133
[ 1374.454648] ata12.00: 3907029168 sectors, multi 0: LBA48 NCQ (depth 32)
[ 1374.463850] ata12.00: configured for UDMA/133
[ 1374.469363] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 
tries: 1
[ 1374.506234] scsi 1:0:6:0: Direct-Access     ATA      ST2000NM0055-1V4 
TN05 PQ: 0 ANSI: 5
[ 1374.515932] sas: DONE DISCOVERY on port 0, pid:7, result:0
[ 1374.515986] sas: sas_form_port: phy3 belongs to port0 already(1)!
[ 1374.523240] sas: sas_form_port: phy3 belongs to port0 already(1)!
[ 1374.538915] sas: sas_form_port: phy3 belongs to port0 already(1)!

Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 05/03/2025 08:16, yangxingui wrote:
>>
>> Sure, something like that - you just need to get libsas to trigger the 
>> proper hw port id assignment for the device. As for specific 
>> implementation in the LLDD, that up to you guys.
> 
> Thank you for your suggestions. The disk was finally restored to normal, 
> but the error handling took a long time. Since the error handling would 
> set the host to the recovery state, other disks on the same host would 
> be blocked for a long time.

But that because you have IO inflight for the disk which was unplugged?

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/3/6 0:15, John Garry wrote:
> On 05/03/2025 08:16, yangxingui wrote:
>>>
>>> Sure, something like that - you just need to get libsas to trigger 
>>> the proper hw port id assignment for the device. As for specific 
>>> implementation in the LLDD, that up to you guys.
>>
>> Thank you for your suggestions. The disk was finally restored to 
>> normal, but the error handling took a long time. Since the error 
>> handling would set the host to the recovery state, other disks on the 
>> same host would be blocked for a long time.
> 
> But that because you have IO inflight for the disk which was unplugged?

No, these are all ATA_CMD_ID_ATA commands newly issued by 
ata_eh_revalidate_and_attach() during error handling, and will be 
retried three times. The reason is that after the hw port id is changed, 
we set it to invalid, and these ATA_CMD_ID_ATA cmds will fail to execute.

Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/2/25 16:19, John Garry wrote:
> On 25/02/2025 01:48, yangxingui wrote:
>>>
>>>
>>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>>> link reset errors - can you consider doing that in 
>>> hisi_sas_update_port_id() when you find an inconstant port id?
>> Currently during phyup, the hw port id may change, and the 
>> corresponding hisi_sas_port.id and the port id in itct are not updated 
>> synchronously. The problem caused is not a link error, so we don't 
>> need deform port, just update the port id when phyup.
> 
> Sure, but I am just trying to keep this simple. If you deform and reform 
> the port - and so lose and find the disk (which does the itct config) - 
> will that solve the problem?
> 
We found that we need to perform lose and find for all devices on the 
port including the local phy and the remote phy. This process still 
requires traversing the phy information corresponding to all devices to 
reset and it is also necessary to consider that there is a race between 
device removal and the current process.  it looks similar to solution of 
update port id directly. And there will be the problem mentioned above. 
e.g, during error handling, the recovery state will last for more than 
15 seconds, affecting the performance of other disks on the same host.


Thanks,
Xingui

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

On 10/03/2025 13:09, yangxingui wrote:
> On 2025/2/25 16:19, John Garry wrote:
>> On 25/02/2025 01:48, yangxingui wrote:
>>>>
>>>>
>>>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>>>> link reset errors - can you consider doing that in 
>>>> hisi_sas_update_port_id() when you find an inconstant port id?
>>> Currently during phyup, the hw port id may change, and the 
>>> corresponding hisi_sas_port.id and the port id in itct are not 
>>> updated synchronously. The problem caused is not a link error, so we 
>>> don't need deform port, just update the port id when phyup.
>>
>> Sure, but I am just trying to keep this simple. If you deform and 
>> reform the port - and so lose and find the disk (which does the itct 
>> config) - will that solve the problem?
>>
> We found that we need to perform lose and find for all devices on the 
> port including the local phy and the remote phy. This process still 
> requires traversing the phy information corresponding to all devices to 
> reset and it is also necessary to consider that there is a race between 
> device removal and the current process.  it looks similar to solution of 
> update port id directly. And there will be the problem mentioned above. 
> e.g, during error handling, the recovery state will last for more than 
> 15 seconds, affecting the performance of other disks on the same host.

How do you even detect the port id inconsistency for the device attached 
at the remote phy? For this series, you could detect it at the phy 
up/down handler for the directly attached device - how would it be 
triggered for the remote phy?

John

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

On 2025/3/11 1:45, John Garry wrote:
>>> Sure, but I am just trying to keep this simple. If you deform and 
>>> reform the port - and so lose and find the disk (which does the itct 
>>> config) - will that solve the problem?
>>>
>> We found that we need to perform lose and find for all devices on the 
>> port including the local phy and the remote phy. This process still 
>> requires traversing the phy information corresponding to all devices 
>> to reset and it is also necessary to consider that there is a race 
>> between device removal and the current process.  it looks similar to 
>> solution of update port id directly. And there will be the problem 
>> mentioned above. e.g, during error handling, the recovery state will 
>> last for more than 15 seconds, affecting the performance of other 
>> disks on the same host.
> 
> How do you even detect the port id inconsistency for the device attached 
> at the remote phy? For this series, you could detect it at the phy 
> up/down handler for the directly attached device - how would it be 
> triggered for the remote phy?

When the hardware port ID of the EXP is detected to have changed, the 
link reset is performed on the local phy of EXP in sequence, which will 
not trigger the lose and find process of the EXP device, and it will not 
trigger the lose and find process of the disks connected to the remote 
phy. Therefore, it is necessary to lose and found all devices separately 
to update the device's port id in itct.

local phy0 --------------------------- disk0

local phy1 --------------------------- disk1

local phy2 --------------------------- disk2

local phy3 --------------------------- disk3
                     _________
local phy4 --------|         |-------- disk4
                    |         |
local phy5 --------|         |-------- disk5
                    |         |
local phy6 --------|   EXP   |-------- disk6
                    |         |
local phy7 --------|         |-------- disk7
                    |_________|


Thanks,
Xingui
.

Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[yangxingui]

Hi, John

On 2025/3/11 1:45, John Garry wrote:
> On 10/03/2025 13:09, yangxingui wrote:
>> On 2025/2/25 16:19, John Garry wrote:
>>> On 25/02/2025 01:48, yangxingui wrote:
>>>>>
>>>>>
>>>>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>>>>> link reset errors - can you consider doing that in 
>>>>> hisi_sas_update_port_id() when you find an inconstant port id?
>>>> Currently during phyup, the hw port id may change, and the 
>>>> corresponding hisi_sas_port.id and the port id in itct are not 
>>>> updated synchronously. The problem caused is not a link error, so we 
>>>> don't need deform port, just update the port id when phyup.
>>>
>>> Sure, but I am just trying to keep this simple. If you deform and 
>>> reform the port - and so lose and find the disk (which does the itct 
>>> config) - will that solve the problem?
>>>
>> We found that we need to perform lose and find for all devices on the 
>> port including the local phy and the remote phy. This process still 
>> requires traversing the phy information corresponding to all devices 
>> to reset and it is also necessary to consider that there is a race 
>> between device removal and the current process.  it looks similar to 
>> solution of update port id directly. And there will be the problem 
>> mentioned above. e.g, during error handling, the recovery state will 
>> last for more than 15 seconds, affecting the performance of other 
>> disks on the same host.
> 
> How do you even detect the port id inconsistency for the device attached 
> at the remote phy? For this series, you could detect it at the phy 
> up/down handler for the directly attached device - how would it be 
> triggered for the remote phy?

The current problem we are facing only involves directly attached 
devices. a new version based on your suggestion.

Thanks,
Xingui

RE: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

[John Garry]

I'll have a look when I get a chance

-----Original Message-----
From: yangxingui <yangxingui@huawei.com> 
Sent: Wednesday, March 12, 2025 9:45 AM
To: John Garry <john.g.garry@oracle.com>; liyihang9@huawei.com; yanaijie@huawei.com
Cc: jejb@linux.ibm.com; Martin Petersen <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux-kernel@vger.kernel.org; linuxarm@huawei.com; prime.zeng@huawei.com; liuyonglong@huawei.com; kangfenglong@huawei.com; liyangyang20@huawei.com; f.fangjian@huawei.com; xiabing14@h-partners.com
Subject: Re: [PATCH v3 1/3] scsi: hisi_sas: Enable force phy when SATA disk directly connected

Hi, John

On 2025/3/11 1:45, John Garry wrote:
> On 10/03/2025 13:09, yangxingui wrote:
>> On 2025/2/25 16:19, John Garry wrote:
>>> On 25/02/2025 01:48, yangxingui wrote:
>>>>>
>>>>>
>>>>> pm8001 sends sas_notify_port_event(sas_phy, PORTE_LINK_RESET_ERR,) 
>>>>> link reset errors - can you consider doing that in 
>>>>> hisi_sas_update_port_id() when you find an inconstant port id?
>>>> Currently during phyup, the hw port id may change, and the 
>>>> corresponding hisi_sas_port.id and the port id in itct are not 
>>>> updated synchronously. The problem caused is not a link error, so we 
>>>> don't need deform port, just update the port id when phyup.
>>>
>>> Sure, but I am just trying to keep this simple. If you deform and 
>>> reform the port - and so lose and find the disk (which does the itct 
>>> config) - will that solve the problem?
>>>
>> We found that we need to perform lose and find for all devices on the 
>> port including the local phy and the remote phy. This process still 
>> requires traversing the phy information corresponding to all devices 
>> to reset and it is also necessary to consider that there is a race 
>> between device removal and the current process.  it looks similar to 
>> solution of update port id directly. And there will be the problem 
>> mentioned above. e.g, during error handling, the recovery state will 
>> last for more than 15 seconds, affecting the performance of other 
>> disks on the same host.
> 
> How do you even detect the port id inconsistency for the device attached 
> at the remote phy? For this series, you could detect it at the phy 
> up/down handler for the directly attached device - how would it be 
> triggered for the remote phy?

The current problem we are facing only involves directly attached 
devices. a new version based on your suggestion.

Thanks,
Xingui

[PATCH v3 2/3] scsi: libsas: Move sas_put_device() to libsas.h

[Xingui Yang]

As sas_put_device() needs to be called in lldd, it is now moved to libsas.h

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_discover.c | 1 +
 drivers/scsi/libsas/sas_internal.h | 6 ------
 include/scsi/libsas.h              | 6 ++++++
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
index 951bdc554a10..43a65d0542ab 100644
--- a/drivers/scsi/libsas/sas_discover.c
+++ b/drivers/scsi/libsas/sas_discover.c
@@ -309,6 +309,7 @@ void sas_free_device(struct kref *kref)
 
 	kfree(dev);
 }
+EXPORT_SYMBOL_GPL(sas_free_device);
 
 static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_device *dev)
 {
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 03d6ec1eb970..a1e364deb3ee 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -98,7 +98,6 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
 			     u8 *sas_addr, enum sas_device_type *type);
 int sas_try_ata_reset(struct asd_sas_phy *phy);
 
-void sas_free_device(struct kref *kref);
 void sas_destruct_devices(struct asd_sas_port *port);
 
 extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS];
@@ -217,9 +216,4 @@ static inline struct domain_device *sas_alloc_device(void)
 	return dev;
 }
 
-static inline void sas_put_device(struct domain_device *dev)
-{
-	kref_put(&dev->kref, sas_free_device);
-}
-
 #endif /* _SAS_INTERNAL_H_ */
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index ba460b6c0374..f67137f50980 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -425,6 +425,12 @@ static inline void sas_put_local_phy(struct sas_phy *phy)
 	put_device(&phy->dev);
 }
 
+void sas_free_device(struct kref *kref);
+static inline void sas_put_device(struct domain_device *dev)
+{
+	kref_put(&dev->kref, sas_free_device);
+}
+
 #ifdef CONFIG_SCSI_SAS_HOST_SMP
 int try_test_sas_gpio_gp_bit(unsigned int od, u8 *data, u8 index, u8 count);
 #else
-- 
2.33.0

Re: [PATCH v3 2/3] scsi: libsas: Move sas_put_device() to libsas.h

[Jason Yan]

在 2025/2/20 21:05, Xingui Yang 写道:
> As sas_put_device() needs to be called in lldd, it is now moved to libsas.h
> 
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
>   drivers/scsi/libsas/sas_discover.c | 1 +
>   drivers/scsi/libsas/sas_internal.h | 6 ------
>   include/scsi/libsas.h              | 6 ++++++
>   3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c
> index 951bdc554a10..43a65d0542ab 100644
> --- a/drivers/scsi/libsas/sas_discover.c
> +++ b/drivers/scsi/libsas/sas_discover.c
> @@ -309,6 +309,7 @@ void sas_free_device(struct kref *kref)
>   
>   	kfree(dev);
>   }
> +EXPORT_SYMBOL_GPL(sas_free_device);
>   
>   static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_device *dev)
>   {
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index 03d6ec1eb970..a1e364deb3ee 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -98,7 +98,6 @@ int sas_get_phy_attached_dev(struct domain_device *dev, int phy_id,
>   			     u8 *sas_addr, enum sas_device_type *type);
>   int sas_try_ata_reset(struct asd_sas_phy *phy);
>   
> -void sas_free_device(struct kref *kref);
>   void sas_destruct_devices(struct asd_sas_port *port);
>   
>   extern const work_func_t sas_phy_event_fns[PHY_NUM_EVENTS];
> @@ -217,9 +216,4 @@ static inline struct domain_device *sas_alloc_device(void)
>   	return dev;
>   }
>   
> -static inline void sas_put_device(struct domain_device *dev)
> -{
> -	kref_put(&dev->kref, sas_free_device);
> -}
> -
>   #endif /* _SAS_INTERNAL_H_ */
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index ba460b6c0374..f67137f50980 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -425,6 +425,12 @@ static inline void sas_put_local_phy(struct sas_phy *phy)
>   	put_device(&phy->dev);
>   }
>   
> +void sas_free_device(struct kref *kref);
> +static inline void sas_put_device(struct domain_device *dev)
> +{
> +	kref_put(&dev->kref, sas_free_device);
> +}

As we have discussed internally, I still think exporting 
sas_put_device() is not a good idea. I prefer exporting an interface to 
iterate sas_port->dev_list and let the caller operate the domain device.

Thanks,
Jason

> +
>   #ifdef CONFIG_SCSI_SAS_HOST_SMP
>   int try_test_sas_gpio_gp_bit(unsigned int od, u8 *data, u8 index, u8 count);
>   #else

[PATCH v3 3/3] scsi: hisi_sas: Fixed IO error caused by port id not updated

[Xingui Yang]

After phy up and its asd_sas_port is not null, the hisi_sas_port
information will not be updated although its hardware port id changes,
then the old port id will cause IO exception. Therefore, update the port
id and itct information when the phy's hw port id changes.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 51 +++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 3596414d970b..26d60d21779f 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -176,6 +176,56 @@ void hisi_sas_stop_phys(struct hisi_hba *hisi_hba)
 }
 EXPORT_SYMBOL_GPL(hisi_sas_stop_phys);
 
+static void hisi_sas_update_itct(void *data, async_cookie_t cookie)
+{
+	struct domain_device *device = data;
+	struct hisi_sas_device *sas_dev = device->lldd_dev;
+	struct hisi_hba *hisi_hba = sas_dev->hisi_hba;
+
+	hisi_hba->hw->clear_itct(hisi_hba, sas_dev);
+	hisi_hba->hw->setup_itct(hisi_hba, sas_dev);
+	sas_put_device(device);
+}
+
+static void hisi_sas_update_port_id(struct hisi_hba *hisi_hba, int phy_no)
+{
+	struct hisi_sas_phy *phy = &hisi_hba->phy[phy_no];
+	struct asd_sas_phy *sas_phy = &phy->sas_phy;
+	struct device *dev = hisi_hba->dev;
+	struct asd_sas_port *sas_port;
+	struct domain_device *device;
+	struct hisi_sas_port *port;
+	ASYNC_DOMAIN_EXCLUSIVE(async);
+
+	if (test_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags) ||
+	    !sas_phy->port)
+		return;
+
+	sas_port = sas_phy->port;
+	port = to_hisi_sas_port(sas_port);
+	if (phy->port_id == port->id)
+		return;
+
+	dev_info(dev, "phy%d's hw port id changed from %d to %llu\n",
+		 phy_no, port->id, phy->port_id);
+	port->id = phy->port_id;
+	spin_lock(&sas_port->dev_list_lock);
+	list_for_each_entry(device, &sas_port->dev_list, dev_list_node) {
+		if (!device->parent)
+			device->linkrate = phy->sas_phy.linkrate;
+
+		/*
+		 * Update itct may trigger scheduling, it cannot be within
+		 * an atomic context, so use asynchronous scheduling and
+		 * hold a reference to avoid racing with final remove.
+		 */
+		kref_get(&device->kref);
+		async_schedule_domain(hisi_sas_update_itct, device, &async);
+	}
+	spin_unlock(&sas_port->dev_list_lock);
+	async_synchronize_full_domain(&async);
+}
+
 static void hisi_sas_slot_index_clear(struct hisi_hba *hisi_hba, int slot_idx)
 {
 	void *bitmap = hisi_hba->slot_index_tags;
@@ -937,6 +987,7 @@ static void hisi_sas_phyup_work_common(struct work_struct *work,
 	struct asd_sas_phy *sas_phy = &phy->sas_phy;
 	int phy_no = sas_phy->id;
 
+	hisi_sas_update_port_id(hisi_hba, phy_no);
 	phy->wait_phyup_cnt = 0;
 	if (phy->identify.target_port_protocols == SAS_PROTOCOL_SSP)
 		hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);
-- 
2.33.0