| Message ID | 20230401081526.1655279-2-yanaijie@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: libsas: remove empty branches and code simplification | expand |
On 4/1/23 17:15, Jason Yan wrote: > In sas_check_eeds() there is an empty branch. We can reverse the > test expression and then remove the empty branch. Also the the test > expression is a little bit complex so it deserves an individual > function. And make the continuing prototype lines indented after > the opening parenthesis to follow the standard coding style. > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > --- > drivers/scsi/libsas/sas_expander.c | 38 ++++++++++++++---------------- > 1 file changed, 18 insertions(+), 20 deletions(-) > > 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. > + 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. > +} > + > static int sas_check_eeds(struct domain_device *child, > - struct ex_phy *parent_phy, > - struct ex_phy *child_phy) > + struct ex_phy *parent_phy, > + struct ex_phy *child_phy) > { > int res = 0; > struct domain_device *parent = child->parent; > + struct sas_discovery *disc = &parent->port->disc; > > - if (SAS_ADDR(parent->port->disc.fanout_sas_addr) != 0) { > + if (SAS_ADDR(disc->fanout_sas_addr) != 0) { > res = -ENODEV; > pr_warn("edge ex %016llx phy S:%02d <--> edge ex %016llx phy S:%02d, while there is a fanout ex %016llx\n", > SAS_ADDR(parent->sas_addr), > parent_phy->phy_id, > SAS_ADDR(child->sas_addr), > child_phy->phy_id, > - SAS_ADDR(parent->port->disc.fanout_sas_addr)); > - } else if (SAS_ADDR(parent->port->disc.eeds_a) == 0) { > - memcpy(parent->port->disc.eeds_a, parent->sas_addr, > - SAS_ADDR_SIZE); > - memcpy(parent->port->disc.eeds_b, child->sas_addr, > - SAS_ADDR_SIZE); > - } else if (((SAS_ADDR(parent->port->disc.eeds_a) == > - SAS_ADDR(parent->sas_addr)) || > - (SAS_ADDR(parent->port->disc.eeds_a) == > - SAS_ADDR(child->sas_addr))) > - && > - ((SAS_ADDR(parent->port->disc.eeds_b) == > - SAS_ADDR(parent->sas_addr)) || > - (SAS_ADDR(parent->port->disc.eeds_b) == > - SAS_ADDR(child->sas_addr)))) > - ; > - else { > + SAS_ADDR(disc->fanout_sas_addr)); > + } else if (SAS_ADDR(disc->eeds_a) == 0) { > + memcpy(disc->eeds_a, parent->sas_addr, SAS_ADDR_SIZE); > + memcpy(disc->eeds_b, child->sas_addr, SAS_ADDR_SIZE); > + } else if (!sas_eeds_valid(parent, child)) { > res = -ENODEV; > pr_warn("edge ex %016llx phy%02d <--> edge ex %016llx phy%02d link forms a third EEDS!\n", > SAS_ADDR(parent->sas_addr),
Hi Damien, On 2023/4/2 12:58, Damien Le Moal wrote: > On 4/1/23 17:15, Jason Yan wrote: >> In sas_check_eeds() there is an empty branch. We can reverse the >> test expression and then remove the empty branch. Also the the test >> expression is a little bit complex so it deserves an individual >> function. And make the continuing prototype lines indented after >> the opening parenthesis to follow the standard coding style. >> >> Signed-off-by: Jason Yan <yanaijie@huawei.com> >> --- >> drivers/scsi/libsas/sas_expander.c | 38 ++++++++++++++---------------- >> 1 file changed, 18 insertions(+), 20 deletions(-) >> >> 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. Thanks, Jason
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. Thanks, John
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 > .
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; + 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)))); +} + static int sas_check_eeds(struct domain_device *child, - struct ex_phy *parent_phy, - struct ex_phy *child_phy) + struct ex_phy *parent_phy, + struct ex_phy *child_phy) { int res = 0; struct domain_device *parent = child->parent; + struct sas_discovery *disc = &parent->port->disc; - if (SAS_ADDR(parent->port->disc.fanout_sas_addr) != 0) { + if (SAS_ADDR(disc->fanout_sas_addr) != 0) { res = -ENODEV; pr_warn("edge ex %016llx phy S:%02d <--> edge ex %016llx phy S:%02d, while there is a fanout ex %016llx\n", SAS_ADDR(parent->sas_addr), parent_phy->phy_id, SAS_ADDR(child->sas_addr), child_phy->phy_id, - SAS_ADDR(parent->port->disc.fanout_sas_addr)); - } else if (SAS_ADDR(parent->port->disc.eeds_a) == 0) { - memcpy(parent->port->disc.eeds_a, parent->sas_addr, - SAS_ADDR_SIZE); - memcpy(parent->port->disc.eeds_b, child->sas_addr, - SAS_ADDR_SIZE); - } else if (((SAS_ADDR(parent->port->disc.eeds_a) == - SAS_ADDR(parent->sas_addr)) || - (SAS_ADDR(parent->port->disc.eeds_a) == - SAS_ADDR(child->sas_addr))) - && - ((SAS_ADDR(parent->port->disc.eeds_b) == - SAS_ADDR(parent->sas_addr)) || - (SAS_ADDR(parent->port->disc.eeds_b) == - SAS_ADDR(child->sas_addr)))) - ; - else { + SAS_ADDR(disc->fanout_sas_addr)); + } else if (SAS_ADDR(disc->eeds_a) == 0) { + memcpy(disc->eeds_a, parent->sas_addr, SAS_ADDR_SIZE); + memcpy(disc->eeds_b, child->sas_addr, SAS_ADDR_SIZE); + } else if (!sas_eeds_valid(parent, child)) { res = -ENODEV; pr_warn("edge ex %016llx phy%02d <--> edge ex %016llx phy%02d link forms a third EEDS!\n", SAS_ADDR(parent->sas_addr),
In sas_check_eeds() there is an empty branch. We can reverse the test expression and then remove the empty branch. Also the the test expression is a little bit complex so it deserves an individual function. And make the continuing prototype lines indented after the opening parenthesis to follow the standard coding style. Signed-off-by: Jason Yan <yanaijie@huawei.com> --- drivers/scsi/libsas/sas_expander.c | 38 ++++++++++++++---------------- 1 file changed, 18 insertions(+), 20 deletions(-)