Re: [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds()

[Jason Yan <yanaijie@huawei.com>]

On 2023/4/3 16:12, John Garry wrote:
> On 03/04/2023 02:37, Jason Yan wrote:
>>>>
>>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>>> b/drivers/scsi/libsas/sas_expander.c
>>>> index dc670304f181..048a931d856a 100644
>>>> --- a/drivers/scsi/libsas/sas_expander.c
>>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>>> @@ -1198,37 +1198,35 @@ static void 
>>>> sas_print_parent_topology_bug(struct domain_device *child,
>>>>             sas_route_char(child, child_phy));
>>>>   }
>>>> +static bool sas_eeds_valid(struct domain_device *parent, struct 
>>>> domain_device *child)
>>>> +{
>>>> +    struct sas_discovery *disc = &parent->port->disc;
>>>
>>> Missing blank line after declaration.
>>
>> OK.
>>
>>>
>>>> +    return (((SAS_ADDR(disc->eeds_a) == SAS_ADDR(parent->sas_addr)) ||
>>>> +         (SAS_ADDR(disc->eeds_a) == SAS_ADDR(child->sas_addr))) &&
>>>> +        ((SAS_ADDR(disc->eeds_b) == SAS_ADDR(parent->sas_addr)) ||
>>>> +         (SAS_ADDR(disc->eeds_b) == SAS_ADDR(child->sas_addr))));
>>>
>>> Drop the inner-most and outter-most parenthesis.
>>
>> No problem.
> 
> Personally I think that the flow:
> 
> if (SAS_ADDR(disc->eeds_a) == SAS_ADDR(parent->sas_addr))
>      return true;
> if (...)
>      return true;
> if (...)
>      return true;
> return false;
> 
> ..reads a bit better (than this and the current code). However I don't 
> feel too strongly about it.

If there is only "||", we can do that. But there is a "&&" so it's not 
that simple now.

Thanks,
Jason

> 
> Thanks,
> John
> .