* Re: [PATCH v2 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list [not found] ` <1639999298-244569-6-git-send-email-chenxiang66@hisilicon.com> @ 2021-12-27 17:25 ` Nathan Chancellor 2021-12-28 2:03 ` chenxiang (M) 0 siblings, 1 reply; 4+ messages in thread From: Nathan Chancellor @ 2021-12-27 17:25 UTC (permalink / raw) To: chenxiang; +Cc: jejb, martin.petersen, linux-scsi, linuxarm, john.garry, llvm Hi Xiang, On Mon, Dec 20, 2021 at 07:21:28PM +0800, chenxiang wrote: > From: Xiang Chen <chenxiang66@hisilicon.com> > > 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@hisilicon.com> > Acked-by: John Garry <john.garry@huawei.com> > --- > 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? Cheers, Nathan ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list 2021-12-27 17:25 ` [PATCH v2 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list Nathan Chancellor @ 2021-12-28 2:03 ` chenxiang (M) 2022-01-04 10:02 ` John Garry 0 siblings, 1 reply; 4+ messages in thread From: chenxiang (M) @ 2021-12-28 2:03 UTC (permalink / raw) To: Nathan Chancellor, martin.petersen, john.garry Cc: jejb, linux-scsi, linuxarm, llvm, colin.i.king 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@hisilicon.com> >> >> 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@hisilicon.com> >> Acked-by: John Garry <john.garry@huawei.com> >> --- >> 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@hisilicon.com> 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@kernel.org> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list 2021-12-28 2:03 ` chenxiang (M) @ 2022-01-04 10:02 ` John Garry 2022-01-04 11:38 ` chenxiang (M) 0 siblings, 1 reply; 4+ messages in thread From: John Garry @ 2022-01-04 10:02 UTC (permalink / raw) To: chenxiang (M), Nathan Chancellor, martin.petersen@oracle.com Cc: jejb@linux.ibm.com, linux-scsi@vger.kernel.org, Linuxarm, llvm@lists.linux.dev, colin.i.king@gmail.com > > @Martin and @John Garry, could you have a review and consider to merge > following patch ? This series has not made it to Martin's 5.17 queue, but I suggest that you send it as a patch in case it does. > > From: Xiang Chen <chenxiang66@hisilicon.com> > 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() please try to condense these subjects, like just "Remove broken legacy code in hisi_sas_send_ata_reset_each_phy()" Or at least remove "function", as this is obvious > > 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 /s/removing/remove/ > old and unused check. > > Fixes: 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to > asd_sas_port->phy_list") > Reported-by: Nathan Chancellor <nathan@kernel.org> Colin King also reported this, so please add him. > Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> > --- > 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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list 2022-01-04 10:02 ` John Garry @ 2022-01-04 11:38 ` chenxiang (M) 0 siblings, 0 replies; 4+ messages in thread From: chenxiang (M) @ 2022-01-04 11:38 UTC (permalink / raw) To: John Garry, Nathan Chancellor, martin.petersen@oracle.com Cc: jejb@linux.ibm.com, linux-scsi@vger.kernel.org, Linuxarm, llvm@lists.linux.dev, colin.i.king@gmail.com Hi John, 在 2022/1/4 18:02, John Garry 写道: >> >> @Martin and @John Garry, could you have a review and consider to merge >> following patch ? > > This series has not made it to Martin's 5.17 queue, but I suggest that > you send it as a patch in case it does. Ok, i will send it as a patch. > >> >> From: Xiang Chen <chenxiang66@hisilicon.com> >> 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() > > please try to condense these subjects, like just "Remove broken legacy > code in hisi_sas_send_ata_reset_each_phy()" > > Or at least remove "function", as this is obvious I will remove "function". > >> >> 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 > > /s/removing/remove/ Ok > >> old and unused check. >> >> Fixes: 29e2bac87421 ("scsi: hisi_sas: Fix some issues related to >> asd_sas_port->phy_list") >> Reported-by: Nathan Chancellor <nathan@kernel.org> > > Colin King also reported this, so please add him. Right, i will add him. > >> Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com> >> --- >> 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 >> >> >> > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-04 11:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1639999298-244569-1-git-send-email-chenxiang66@hisilicon.com>
[not found] ` <1639999298-244569-6-git-send-email-chenxiang66@hisilicon.com>
2021-12-27 17:25 ` [PATCH v2 05/15] scsi: hisi_sas: Fix some issues related to asd_sas_port->phy_list Nathan Chancellor
2021-12-28 2:03 ` chenxiang (M)
2022-01-04 10:02 ` John Garry
2022-01-04 11:38 ` chenxiang (M)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox