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

[Jason Yan <yanaijie@xxxxxxxxxx>]

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
> .