Hi Nathan and Colin,
Thank you for your report.
在 2021/12/28 1:25, Nathan Chancellor 写道:
Hi Xiang,
On Mon, Dec 20, 2021 at 07:21:28PM +0800, chenxiang wrote:
From: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
Most places that use asd_sas_port->phy_list are protected by spinlock
asd_sas_port->phy_list_lock, but there are some places which lack of it
in hisi_sas driver, so add it in function hisi_sas_refresh_port_id() when
accessing asd_sas_port->phy_list. But it has a risk that list mutates while
dropping the lock at the same time in function
hisi_sas_send_ata_reset_each_phy(), so read asd_sas_port->phy_mask
instead of accessing asd_sas_port->phy_list to avoid the risk.
Signed-off-by: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
Acked-by: John Garry <john.garry@xxxxxxxxxx>
---
drivers/scsi/hisi_sas/hisi_sas_main.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index ad64ccd41420..051092e294f7 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1428,11 +1428,13 @@ static void hisi_sas_refresh_port_id(struct hisi_hba *hisi_hba)
sas_port = device->port;
port = to_hisi_sas_port(sas_port);
+ spin_lock(&sas_port->phy_list_lock);
list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el)
if (state & BIT(sas_phy->id)) {
phy = sas_phy->lldd_phy;
break;
}
+ spin_unlock(&sas_port->phy_list_lock);
if (phy) {
port->id = phy->port_id;
@@ -1509,22 +1511,25 @@ static void hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba,
struct ata_link *link;
u8 fis[20] = {0};
u32 state;
+ int i;
state = hisi_hba->hw->get_phys_state(hisi_hba);
- list_for_each_entry(sas_phy, &sas_port->phy_list, port_phy_el) {
+ for (i = 0; i < hisi_hba->n_phy; i++) {
if (!(state & BIT(sas_phy->id)))
continue;
+ if (!(sas_port->phy_mask & BIT(i)))
+ continue;
ata_for_each_link(link, ap, EDGE) {
int pmp = sata_srst_pmp(link);
- tmf_task.phy_id = sas_phy->id;
+ tmf_task.phy_id = i;
hisi_sas_fill_ata_reset_cmd(link->device, 1, pmp, fis);
rc = hisi_sas_exec_internal_tmf_task(device, fis, s,
&tmf_task);
if (rc != TMF_RESP_FUNC_COMPLETE) {
dev_err(dev, "phy%d ata reset failed rc=%d\n",
- sas_phy->id, rc);
+ i, rc);
break;
}
}
--
2.33.0
Please ignore this if it was already reported, I do not see any reports
of it on lore.kernel.org nor a commit fixing it in Martin's tree.
This commit as commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues
related to asd_sas_port->phy_list") in -next causes the following clang
warning, which will break the build under -Werror:
drivers/scsi/hisi_sas/hisi_sas_main.c:1536:21: error: variable 'sas_phy' is uninitialized when used here [-Werror,-Wuninitialized]
if (!(state & BIT(sas_phy->id)))
^~~~~~~
./include/vdso/bits.h:7:30: note: expanded from macro 'BIT'
#define BIT(nr) (UL(1) << (nr))
^~
drivers/scsi/hisi_sas/hisi_sas_main.c:1528:29: note: initialize the variable 'sas_phy' to silence this warning
struct asd_sas_phy *sas_phy;
^
= NULL
1 error generated.
It seems like this variable is entirely unused now, should it be removed
along with this check?
Right, it needs to be removed as the additional check is enough.
@Martin and @John Garry, could you have a review and consider to merge
following patch ?
From: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
Date: Tue, 28 Dec 2021 09:40:01 +0800
Subject: [PATCH] scsi: libsas: Remove unused variable and check in function
hisi_sas_send_ata_reset_each_phy()
In commit 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
asd_sas_port->phy_list"), we use asd_sas_port->phy_mask instead of
accessing asd_sas_port->phy_list, and it is enough to use
asd_sas_port->phy_mask to check the state of phy, so removing the
old and unused check.
Fixes: 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to
asd_sas_port->phy_list")
Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
Signed-off-by: Xiang Chen <chenxiang66@xxxxxxxxxxxxx>
---
drivers/scsi/hisi_sas/hisi_sas_main.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
b/drivers/scsi/hisi_sas/hisi_sas_main.c
index f46f679fe825..a05ec7aece5a 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -1525,16 +1525,11 @@ static void
hisi_sas_send_ata_reset_each_phy(struct hisi_hba *hisi_hba,
struct device *dev = hisi_hba->dev;
int s = sizeof(struct host_to_dev_fis);
int rc = TMF_RESP_FUNC_FAILED;
- struct asd_sas_phy *sas_phy;
struct ata_link *link;
u8 fis[20] = {0};
- u32 state;
int i;
- state = hisi_hba->hw->get_phys_state(hisi_hba);
for (i = 0; i < hisi_hba->n_phy; i++) {
- if (!(state & BIT(sas_phy->id)))
- continue;
if (!(sas_port->phy_mask & BIT(i)))
continue;
--
2.33.0
