[PATCH v2 0/3] scsi: libsas: remove empty branches and code simplification

[PATCH v2 0/3] scsi: libsas: remove empty branches and code simplification

[Jason Yan]

Three patches to remove two empty branches and a little code
simplification.

v1->v2:
  1. Add blank line after declaration in sas_eeds_valid().
  2. Drop the inner-most and outter-most parenthesis in sas_eeds_valid().
  3. Break the long line in the declaration of sas_check_phy_topology().
  4. Move "else if" to the same line after "}".

Jason Yan (3):
  scsi: libsas: Simplify sas_check_eeds()
  scsi: libsas: Remove an empty branch in sas_check_parent_topology()
  scsi: libsas: Simplify sas_check_parent_topology()

 drivers/scsi/libsas/sas_expander.c | 135 ++++++++++++++++-------------
 1 file changed, 73 insertions(+), 62 deletions(-)

-- 
2.31.1

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

[Jason Yan]

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 | 39 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index dc670304f181..70fd4f439664 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1198,37 +1198,36 @@ 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),
-- 
2.31.1

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

[Christoph Hellwig]

On Thu, Apr 20, 2023 at 10:33:37PM +0800, 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 | 39 +++++++++++++++---------------
>  1 file changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index dc670304f181..70fd4f439664 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1198,37 +1198,36 @@ 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)

Please avoid the overly long line.

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

[Jason Yan]

On 2023/4/20 23:00, Christoph Hellwig wrote:
> On Thu, Apr 20, 2023 at 10:33:37PM +0800, 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 | 39 +++++++++++++++---------------
>>   1 file changed, 19 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
>> index dc670304f181..70fd4f439664 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -1198,37 +1198,36 @@ 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)
> 
> Please avoid the overly long line.

OK.

[PATCH v2 2/3] scsi: libsas: Remove an empty branch in sas_check_parent_topology()

[Jason Yan]

There is an empty "All good" branch in sas_check_parent_topology(). We can
reverse the test statement and remove the empty branch.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 70fd4f439664..bbf73e74530e 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1285,11 +1285,9 @@ static int sas_check_parent_topology(struct domain_device *child)
 					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)) {
-					/* All good */;
-				} else {
+				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;
 				}
-- 
2.31.1

Re: [PATCH v2 2/3] scsi: libsas: Remove an empty branch in sas_check_parent_topology()

[Christoph Hellwig]

On Thu, Apr 20, 2023 at 10:33:38PM +0800, Jason Yan wrote:
> There is an empty "All good" branch in sas_check_parent_topology(). We can
> reverse the test statement and remove the empty branch.

Eww, this code is pretty unreadable (as-is and after the change).
Can you move SAS_EDGE_EXPANDER_DEVICE case into a helper to
make it readabke?  That has the extra upside of just being able to
return the error code instead of assigning it to res.

Re: [PATCH v2 2/3] scsi: libsas: Remove an empty branch in sas_check_parent_topology()

[Jason Yan]

On 2023/4/20 23:02, Christoph Hellwig wrote:
> On Thu, Apr 20, 2023 at 10:33:38PM +0800, Jason Yan wrote:
>> There is an empty "All good" branch in sas_check_parent_topology(). We can
>> reverse the test statement and remove the empty branch.
> 
> Eww, this code is pretty unreadable (as-is and after the change).
> Can you move SAS_EDGE_EXPANDER_DEVICE case into a helper to
> make it readabke?  That has the extra upside of just being able to
> return the error code instead of assigning it to res.

Sure.

[PATCH v2 3/3] scsi: libsas: Simplify sas_check_parent_topology()

[Jason Yan]

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 | 94 +++++++++++++++++-------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index bbf73e74530e..be8171f39de3 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1239,11 +1239,58 @@ 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;
@@ -1258,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)
@@ -1267,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;
-- 
2.31.1