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

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

[Jason Yan]

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

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 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 | 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;
+	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

[PATCH 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 048a931d856a..c0841652f0e0 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1284,11 +1284,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

[PATCH 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 | 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)
 {
 	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;
-- 
2.31.1

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

[Damien Le Moal]

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),

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

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;

-- 
Damien Le Moal
Western Digital Research

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

[Jason Yan]

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

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

[Jason Yan]

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;
[...]

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

[Jason Yan]

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.

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

[John Garry]

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

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

[Jason Yan]

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