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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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