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?
Cheers,
Nathan
