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

[Jason Yan <yanaijie@huawei.com>]

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.