diff mbox series

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

Message ID 20230401081526.1655279-2-yanaijie@huawei.com (mailing list archive)
State Superseded
Headers show
Series scsi: libsas: remove empty branches and code simplification | expand

Commit Message

Jason Yan April 1, 2023, 8:15 a.m. UTC 
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(-)

Comments

Damien Le Moal April 2, 2023, 4:58 a.m. UTC | #1 
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),
Jason Yan April 3, 2023, 1:37 a.m. UTC | #2 
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
John Garry April 3, 2023, 8:12 a.m. UTC | #3 
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
Jason Yan April 3, 2023, 9:11 a.m. UTC | #4 
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
> .
diff mbox series

Patch

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