| Message ID | 20230401081526.1655279-4-yanaijie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: libsas: remove empty branches and code simplification | expand |
On 4/1/23 17:15, Jason Yan wrote: > Factor out a new helper sas_check_phy_topology() to simplify > sas_check_parent_topology(). And centralize the calling of > sas_print_parent_topology_bug(). > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > --- > drivers/scsi/libsas/sas_expander.c | 95 +++++++++++++++++------------- > 1 file changed, 55 insertions(+), 40 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c > index c0841652f0e0..bffcccdbda6b 100644 > --- a/drivers/scsi/libsas/sas_expander.c > +++ b/drivers/scsi/libsas/sas_expander.c > @@ -1238,11 +1238,59 @@ static int sas_check_eeds(struct domain_device *child, > return res; > } > > -/* Here we spill over 80 columns. It is intentional. > - */ > -static int sas_check_parent_topology(struct domain_device *child) > + > +static int sas_check_phy_topology(struct domain_device *child, struct ex_phy *parent_phy) Long line. Break after the first argument. > { > struct expander_device *child_ex = &child->ex_dev; > + struct ex_phy *child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id]; > + struct expander_device *parent_ex = &child->parent->ex_dev; > + bool print_topology_bug = false; > + int res = 0; > + > + switch (child->parent->dev_type) { > + case SAS_EDGE_EXPANDER_DEVICE: > + if (child->dev_type == SAS_FANOUT_EXPANDER_DEVICE) { > + if (parent_phy->routing_attr != SUBTRACTIVE_ROUTING || > + child_phy->routing_attr != TABLE_ROUTING) { > + res = -ENODEV; > + print_topology_bug = true; > + } > + } else if (parent_phy->routing_attr == SUBTRACTIVE_ROUTING) { > + if (child_phy->routing_attr == SUBTRACTIVE_ROUTING) { > + res = sas_check_eeds(child, parent_phy, child_phy); > + } The "else if" below should not be on a different line. > + else if (child_phy->routing_attr != TABLE_ROUTING) { > + res = -ENODEV; > + print_topology_bug = true; > + } > + } else if (parent_phy->routing_attr == TABLE_ROUTING) { > + if (child_phy->routing_attr != SUBTRACTIVE_ROUTING && > + (child_phy->routing_attr != TABLE_ROUTING || > + !child_ex->t2t_supp || !parent_ex->t2t_supp)) { > + res = -ENODEV; > + print_topology_bug = true; > + } > + } > + break; > + case SAS_FANOUT_EXPANDER_DEVICE: > + if (parent_phy->routing_attr != TABLE_ROUTING || > + child_phy->routing_attr != SUBTRACTIVE_ROUTING) { > + res = -ENODEV; > + print_topology_bug = true; > + } > + break; > + default: > + break; > + } > + > + if (print_topology_bug) > + sas_print_parent_topology_bug(child, parent_phy, child_phy); > + > + return res; > +} > + > +static int sas_check_parent_topology(struct domain_device *child) > +{ > struct expander_device *parent_ex; > int i; > int res = 0; > @@ -1257,7 +1305,7 @@ static int sas_check_parent_topology(struct domain_device *child) > > for (i = 0; i < parent_ex->num_phys; i++) { > struct ex_phy *parent_phy = &parent_ex->ex_phy[i]; > - struct ex_phy *child_phy; > + int ret; > > if (parent_phy->phy_state == PHY_VACANT || > parent_phy->phy_state == PHY_NOT_PRESENT) > @@ -1266,42 +1314,9 @@ static int sas_check_parent_topology(struct domain_device *child) > if (!sas_phy_match_dev_addr(child, parent_phy)) > continue; > > - child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id]; > - > - switch (child->parent->dev_type) { > - case SAS_EDGE_EXPANDER_DEVICE: > - if (child->dev_type == SAS_FANOUT_EXPANDER_DEVICE) { > - if (parent_phy->routing_attr != SUBTRACTIVE_ROUTING || > - child_phy->routing_attr != TABLE_ROUTING) { > - sas_print_parent_topology_bug(child, parent_phy, child_phy); > - res = -ENODEV; > - } > - } else if (parent_phy->routing_attr == SUBTRACTIVE_ROUTING) { > - if (child_phy->routing_attr == SUBTRACTIVE_ROUTING) { > - res = sas_check_eeds(child, parent_phy, child_phy); > - } else if (child_phy->routing_attr != TABLE_ROUTING) { > - sas_print_parent_topology_bug(child, parent_phy, child_phy); > - res = -ENODEV; > - } > - } else if (parent_phy->routing_attr == TABLE_ROUTING) { > - if (child_phy->routing_attr != SUBTRACTIVE_ROUTING && > - (child_phy->routing_attr != TABLE_ROUTING || > - !child_ex->t2t_supp || !parent_ex->t2t_supp)) { > - sas_print_parent_topology_bug(child, parent_phy, child_phy); > - res = -ENODEV; > - } > - } > - break; > - case SAS_FANOUT_EXPANDER_DEVICE: > - if (parent_phy->routing_attr != TABLE_ROUTING || > - child_phy->routing_attr != SUBTRACTIVE_ROUTING) { > - sas_print_parent_topology_bug(child, parent_phy, child_phy); > - res = -ENODEV; > - } > - break; > - default: > - break; > - } > + ret = sas_check_phy_topology(child, parent_phy); > + if (ret) > + res = ret; > } > > return res;
On 2023/4/2 13:00, Damien Le Moal wrote: > On 4/1/23 17:15, Jason Yan wrote: >> Factor out a new helper sas_check_phy_topology() to simplify >> sas_check_parent_topology(). And centralize the calling of >> sas_print_parent_topology_bug(). >> >> Signed-off-by: Jason Yan <yanaijie@huawei.com> >> --- >> drivers/scsi/libsas/sas_expander.c | 95 +++++++++++++++++------------- >> 1 file changed, 55 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c >> index c0841652f0e0..bffcccdbda6b 100644 >> --- a/drivers/scsi/libsas/sas_expander.c >> +++ b/drivers/scsi/libsas/sas_expander.c >> @@ -1238,11 +1238,59 @@ static int sas_check_eeds(struct domain_device *child, >> return res; >> } >> >> -/* Here we spill over 80 columns. It is intentional. >> - */ >> -static int sas_check_parent_topology(struct domain_device *child) >> + >> +static int sas_check_phy_topology(struct domain_device *child, struct ex_phy *parent_phy) > > Long line. Break after the first argument. The max line warning in checkpatch has been changed to 100[1]. But 80 is still preferred. So you are right, I will fix it. [1] bdc48fa11e46 ("checkpatch/coding-style: deprecate 80-column warning") > >> { >> struct expander_device *child_ex = &child->ex_dev; >> + struct ex_phy *child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id]; >> + struct expander_device *parent_ex = &child->parent->ex_dev; >> + bool print_topology_bug = false; >> + int res = 0; >> + >> + switch (child->parent->dev_type) { >> + case SAS_EDGE_EXPANDER_DEVICE: >> + if (child->dev_type == SAS_FANOUT_EXPANDER_DEVICE) { >> + if (parent_phy->routing_attr != SUBTRACTIVE_ROUTING || >> + child_phy->routing_attr != TABLE_ROUTING) { >> + res = -ENODEV; >> + print_topology_bug = true; >> + } >> + } else if (parent_phy->routing_attr == SUBTRACTIVE_ROUTING) { >> + if (child_phy->routing_attr == SUBTRACTIVE_ROUTING) { >> + res = sas_check_eeds(child, parent_phy, child_phy); >> + } > > The "else if" below should not be on a different line. Good catch. Will fix. Thanks, Jason > >> + else if (child_phy->routing_attr != TABLE_ROUTING) { >> + res = -ENODEV; >> + print_topology_bug = true; [...]
On 2023/4/2 13:00, Damien Le Moal wrote: > On 4/1/23 17:15, Jason Yan wrote: >> Factor out a new helper sas_check_phy_topology() to simplify >> sas_check_parent_topology(). And centralize the calling of >> sas_print_parent_topology_bug(). >> >> Signed-off-by: Jason Yan <yanaijie@huawei.com> >> --- >> drivers/scsi/libsas/sas_expander.c | 95 +++++++++++++++++------------- >> 1 file changed, 55 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c >> index c0841652f0e0..bffcccdbda6b 100644 >> --- a/drivers/scsi/libsas/sas_expander.c >> +++ b/drivers/scsi/libsas/sas_expander.c >> @@ -1238,11 +1238,59 @@ static int sas_check_eeds(struct domain_device *child, >> return res; >> } >> >> -/* Here we spill over 80 columns. It is intentional. >> - */ >> -static int sas_check_parent_topology(struct domain_device *child) >> + >> +static int sas_check_phy_topology(struct domain_device *child, struct ex_phy *parent_phy) > > Long line. Break after the first argument. And also some lines exceed 80 columns, I wonder if you want me to break them or something else to shorten them too. > >> { >> struct expander_device *child_ex = &child->ex_dev; >> + struct ex_phy *child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id]; This line is slightly longer than 80, but I prefer to keep them as is. >> + struct expander_device *parent_ex = &child->parent->ex_dev; >> + bool print_topology_bug = false; >> + int res = 0; >> + >> + switch (child->parent->dev_type) { >> + case SAS_EDGE_EXPANDER_DEVICE: >> + if (child->dev_type == SAS_FANOUT_EXPANDER_DEVICE) { >> + if (parent_phy->routing_attr != SUBTRACTIVE_ROUTING || >> + child_phy->routing_attr != TABLE_ROUTING) { >> + res = -ENODEV; >> + print_topology_bug = true; >> + } >> + } else if (parent_phy->routing_attr == SUBTRACTIVE_ROUTING) { >> + if (child_phy->routing_attr == SUBTRACTIVE_ROUTING) { >> + res = sas_check_eeds(child, parent_phy, child_phy); And this line too. If someone insist to keep it in 80 columns, it may be written like: + res = sas_check_eeds(child, parent_phy, + child_phy); Which I do not like either.
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c index c0841652f0e0..bffcccdbda6b 100644 --- a/drivers/scsi/libsas/sas_expander.c +++ b/drivers/scsi/libsas/sas_expander.c @@ -1238,11 +1238,59 @@ static int sas_check_eeds(struct domain_device *child, return res; } -/* Here we spill over 80 columns. It is intentional. - */ -static int sas_check_parent_topology(struct domain_device *child) + +static int sas_check_phy_topology(struct domain_device *child, struct ex_phy *parent_phy) { struct expander_device *child_ex = &child->ex_dev; + struct ex_phy *child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id]; + struct expander_device *parent_ex = &child->parent->ex_dev; + bool print_topology_bug = false; + int res = 0; + + switch (child->parent->dev_type) { + case SAS_EDGE_EXPANDER_DEVICE: + if (child->dev_type == SAS_FANOUT_EXPANDER_DEVICE) { + if (parent_phy->routing_attr != SUBTRACTIVE_ROUTING || + child_phy->routing_attr != TABLE_ROUTING) { + res = -ENODEV; + print_topology_bug = true; + } + } else if (parent_phy->routing_attr == SUBTRACTIVE_ROUTING) { + if (child_phy->routing_attr == SUBTRACTIVE_ROUTING) { + res = sas_check_eeds(child, parent_phy, child_phy); + } + else if (child_phy->routing_attr != TABLE_ROUTING) { + res = -ENODEV; + print_topology_bug = true; + } + } else if (parent_phy->routing_attr == TABLE_ROUTING) { + if (child_phy->routing_attr != SUBTRACTIVE_ROUTING && + (child_phy->routing_attr != TABLE_ROUTING || + !child_ex->t2t_supp || !parent_ex->t2t_supp)) { + res = -ENODEV; + print_topology_bug = true; + } + } + break; + case SAS_FANOUT_EXPANDER_DEVICE: + if (parent_phy->routing_attr != TABLE_ROUTING || + child_phy->routing_attr != SUBTRACTIVE_ROUTING) { + res = -ENODEV; + print_topology_bug = true; + } + break; + default: + break; + } + + if (print_topology_bug) + sas_print_parent_topology_bug(child, parent_phy, child_phy); + + return res; +} + +static int sas_check_parent_topology(struct domain_device *child) +{ struct expander_device *parent_ex; int i; int res = 0; @@ -1257,7 +1305,7 @@ static int sas_check_parent_topology(struct domain_device *child) for (i = 0; i < parent_ex->num_phys; i++) { struct ex_phy *parent_phy = &parent_ex->ex_phy[i]; - struct ex_phy *child_phy; + int ret; if (parent_phy->phy_state == PHY_VACANT || parent_phy->phy_state == PHY_NOT_PRESENT) @@ -1266,42 +1314,9 @@ static int sas_check_parent_topology(struct domain_device *child) if (!sas_phy_match_dev_addr(child, parent_phy)) continue; - child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id]; - - switch (child->parent->dev_type) { - case SAS_EDGE_EXPANDER_DEVICE: - if (child->dev_type == SAS_FANOUT_EXPANDER_DEVICE) { - if (parent_phy->routing_attr != SUBTRACTIVE_ROUTING || - child_phy->routing_attr != TABLE_ROUTING) { - sas_print_parent_topology_bug(child, parent_phy, child_phy); - res = -ENODEV; - } - } else if (parent_phy->routing_attr == SUBTRACTIVE_ROUTING) { - if (child_phy->routing_attr == SUBTRACTIVE_ROUTING) { - res = sas_check_eeds(child, parent_phy, child_phy); - } else if (child_phy->routing_attr != TABLE_ROUTING) { - sas_print_parent_topology_bug(child, parent_phy, child_phy); - res = -ENODEV; - } - } else if (parent_phy->routing_attr == TABLE_ROUTING) { - if (child_phy->routing_attr != SUBTRACTIVE_ROUTING && - (child_phy->routing_attr != TABLE_ROUTING || - !child_ex->t2t_supp || !parent_ex->t2t_supp)) { - sas_print_parent_topology_bug(child, parent_phy, child_phy); - res = -ENODEV; - } - } - break; - case SAS_FANOUT_EXPANDER_DEVICE: - if (parent_phy->routing_attr != TABLE_ROUTING || - child_phy->routing_attr != SUBTRACTIVE_ROUTING) { - sas_print_parent_topology_bug(child, parent_phy, child_phy); - res = -ENODEV; - } - break; - default: - break; - } + ret = sas_check_phy_topology(child, parent_phy); + if (ret) + res = ret; } return res;
Factor out a new helper sas_check_phy_topology() to simplify sas_check_parent_topology(). And centralize the calling of sas_print_parent_topology_bug(). Signed-off-by: Jason Yan <yanaijie@huawei.com> --- drivers/scsi/libsas/sas_expander.c | 95 +++++++++++++++++------------- 1 file changed, 55 insertions(+), 40 deletions(-)