linux-kernel - Re: [PATCH] scsi: libsas: no need to join wide port again in sas_ex_discover

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 20 May 2019 20:06:43 +0800
From:   Jason Yan <yanaijie@...wei.com>
To:     John Garry <john.garry@...wei.com>, <martin.petersen@...cle.com>,
        <jejb@...ux.vnet.ibm.com>
CC:     <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <hare@...e.com>, <dan.j.williams@...el.com>, <jthumshirn@...e.de>,
        <hch@....de>, <huangdaode@...ilicon.com>,
        <chenxiang66@...ilicon.com>, <miaoxie@...wei.com>,
        <zhaohongjiang@...wei.com>
Subject: Re: [PATCH] scsi: libsas: no need to join wide port again in
 sas_ex_discover_dev()



On 2019/5/20 18:54, John Garry wrote:
> On 18/05/2019 10:40, Jason Yan wrote:
>> Since we are processing events synchronously now, the second call of
>> sas_ex_join_wide_port() in sas_ex_discover_dev() is not needed. There
>> will be no races with other works in disco workqueue. So remove the
>> second sas_ex_join_wide_port().
>>
>> I did not change the return value of 'res' to error when discover failed
>> because we need to continue to discover other phys if one phy discover
>> failed. So let's keep that logic as before and just add a debug log to
>> detect the failure.
>>
>> Signed-off-by: Jason Yan <yanaijie@...wei.com>
>> ---
>>  drivers/scsi/libsas/sas_expander.c | 24 +++---------------------
>>  1 file changed, 3 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index 83f2fd70ce76..8f90dd497dfe 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1116,27 +1116,9 @@ static int sas_ex_discover_dev(struct 
>> domain_device *dev, int phy_id)
>>          break;
>>      }
>>
>> -    if (child) {
>> -        int i;
>> -
>> -        for (i = 0; i < ex->num_phys; i++) {
>> -            if (ex->ex_phy[i].phy_state == PHY_VACANT ||
>> -                ex->ex_phy[i].phy_state == PHY_NOT_PRESENT)
>> -                continue;
>> -            /*
>> -             * Due to races, the phy might not get added to the
>> -             * wide port, so we add the phy to the wide port here.
>> -             */
>> -            if (SAS_ADDR(ex->ex_phy[i].attached_sas_addr) ==
>> -                SAS_ADDR(child->sas_addr)) {
>> -                ex->ex_phy[i].phy_state= PHY_DEVICE_DISCOVERED;
>> -                if (sas_ex_join_wide_port(dev, i))
>> -                    pr_debug("Attaching ex phy%02d to wide port 
>> %016llx\n",
>> -                         i, SAS_ADDR(ex->ex_phy[i].attached_sas_addr));
>> -            }
>> -        }
>> -    }
> 
> This change looks ok.
> 
>> -
>> +    if (!child)
>> +        pr_debug("Ex %016llx phy%02d failed to discover\n",
>> +             SAS_ADDR(dev->sas_addr), phy_id);
> 
> nit:
> /s/Ex/ex/

OK.

> 
> In case of "second fanout expander...", before this, we don't attempt to 
> discover, and just disable the PHY. In that case, is the log proper?
> 

In that case the log is not proper. I think we can directly return in 
the case of "second fanout expander..."? Actually nothing to do after 
the phy is disabled.

> And, if indeed proper, it would seem to merit a higher log level than 
> debug, maybe notice is better.
>
Yes, notice should be better.

> 
>>      return res;
>>  }
>>
>>
> 
> 
> 
> .
>