On 2/25/22 18:54, John Garry wrote:
> On 25/02/2022 05:56, Damien Le Moal wrote:
>> Sparse throws a warning about context imbalance ("different lock
>> contexts for basic block") in sas_form_port() as it gets confused with
>> the fact that a port is locked within one of the 2 search loop and
>> unlocked afterward outside of the search loops once the phy is added to
>> the port. Since this code is not easy to follow, improve it by factoring
>> out the code adding the phy to the port once the port is locked into the
>> helper function __sas_form_port(). This helper can then be called
>> directly within the port search loops, avoiding confusion and clearing
>> the sparse warning.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/scsi/libsas/sas_port.c | 76 ++++++++++++++++++++--------------
>> 1 file changed, 45 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c
>> index 67b429dcf1ff..90bd1666f888 100644
>> --- a/drivers/scsi/libsas/sas_port.c
>> +++ b/drivers/scsi/libsas/sas_port.c
>> @@ -67,6 +67,39 @@ static void sas_resume_port(struct asd_sas_phy *phy)
>> sas_discover_event(port, DISCE_RESUME);
>> }
>>
>> +static struct domain_device *__sas_form_port(struct asd_sas_port *port,
>> + struct asd_sas_phy *phy,
>> + bool wideport)
>
> How about a more obvious name, like "sas_form_port_add_phy()"?
>
> And returning a domain_device is strange. We don't assign/evaluate that
> in this function. Instead I think that you may just use port->port_dev
> outside later, right?
OK. Will send V2. Thanks for the review.
>
>> +{
>> + struct domain_device *port_dev = port->port_dev;
>> +
>> + list_add_tail(&phy->port_phy_el, &port->phy_list);
>> + sas_phy_set_target(phy, port_dev);
>> + phy->port = port;
>> + port->num_phys++;
>> + port->phy_mask |= (1U << phy->id);
>
> nit: no need for ()
>
>> +
>> + if (wideport)
>> + pr_debug("phy%d matched wide port%d\n", phy->id,
>> + port->id);
>> + else
>> + memcpy(port->sas_addr, phy->sas_addr, SAS_ADDR_SIZE);
>> +
>> + if (*(u64 *)port->attached_sas_addr == 0) {
>> + port->class = phy->class;
>> + memcpy(port->attached_sas_addr, phy->attached_sas_addr,
>> + SAS_ADDR_SIZE);
>> + port->iproto = phy->iproto;
>> + port->tproto = phy->tproto;
>> + port->oob_mode = phy->oob_mode;
>> + port->linkrate = phy->linkrate;
>> + } else {
>> + port->linkrate = max(port->linkrate, phy->linkrate);
>> + }
>> +
>> + return port_dev;
>>
> Thanks,
> John
--
Damien Le Moal
Western Digital Research
