* [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds()
2023-04-01 8:15 [PATCH 0/3] scsi: libsas: remove empty branches and code simplification Jason Yan
@ 2023-04-01 8:15 ` Jason Yan
2023-04-02 4:58 ` Damien Le Moal
2023-04-01 8:15 ` [PATCH 2/3] scsi: libsas: Remove an empty branch in sas_check_parent_topology() Jason Yan
2023-04-01 8:15 ` [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology() Jason Yan
2 siblings, 1 reply; 11+ messages in thread
From: Jason Yan @ 2023-04-01 8:15 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds()
2023-04-01 8:15 ` [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds() Jason Yan
@ 2023-04-02 4:58 ` Damien Le Moal
2023-04-03 1:37 ` Jason Yan
0 siblings, 1 reply; 11+ messages in thread
From: Damien Le Moal @ 2023-04-02 4:58 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, john.g.garry
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds()
2023-04-02 4:58 ` Damien Le Moal
@ 2023-04-03 1:37 ` Jason Yan
2023-04-03 8:12 ` John Garry
0 siblings, 1 reply; 11+ messages in thread
From: Jason Yan @ 2023-04-03 1:37 UTC (permalink / raw)
To: Damien Le Moal, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, john.g.garry
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds()
2023-04-03 1:37 ` Jason Yan
@ 2023-04-03 8:12 ` John Garry
2023-04-03 9:11 ` Jason Yan
0 siblings, 1 reply; 11+ messages in thread
From: John Garry @ 2023-04-03 8:12 UTC (permalink / raw)
To: Jason Yan, Damien Le Moal, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds()
2023-04-03 8:12 ` John Garry
@ 2023-04-03 9:11 ` Jason Yan
0 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2023-04-03 9:11 UTC (permalink / raw)
To: John Garry, Damien Le Moal, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang
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
> .
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] scsi: libsas: Remove an empty branch in sas_check_parent_topology()
2023-04-01 8:15 [PATCH 0/3] scsi: libsas: remove empty branches and code simplification Jason Yan
2023-04-01 8:15 ` [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds() Jason Yan
@ 2023-04-01 8:15 ` Jason Yan
2023-04-01 8:15 ` [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology() Jason Yan
2 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2023-04-01 8:15 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology()
2023-04-01 8:15 [PATCH 0/3] scsi: libsas: remove empty branches and code simplification Jason Yan
2023-04-01 8:15 ` [PATCH 1/3] scsi: libsas: Simplify sas_check_eeds() Jason Yan
2023-04-01 8:15 ` [PATCH 2/3] scsi: libsas: Remove an empty branch in sas_check_parent_topology() Jason Yan
@ 2023-04-01 8:15 ` Jason Yan
2023-04-02 5:00 ` Damien Le Moal
2 siblings, 1 reply; 11+ messages in thread
From: Jason Yan @ 2023-04-01 8:15 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, damien.lemoal,
john.g.garry, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology()
2023-04-01 8:15 ` [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology() Jason Yan
@ 2023-04-02 5:00 ` Damien Le Moal
2023-04-03 2:07 ` Jason Yan
2023-04-03 3:13 ` Jason Yan
0 siblings, 2 replies; 11+ messages in thread
From: Damien Le Moal @ 2023-04-02 5:00 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, john.g.garry
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology()
2023-04-02 5:00 ` Damien Le Moal
@ 2023-04-03 2:07 ` Jason Yan
2023-04-03 3:13 ` Jason Yan
1 sibling, 0 replies; 11+ messages in thread
From: Jason Yan @ 2023-04-03 2:07 UTC (permalink / raw)
To: Damien Le Moal, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, john.g.garry
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;
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 3/3] scsi: libsas: Simplify sas_check_parent_topology()
2023-04-02 5:00 ` Damien Le Moal
2023-04-03 2:07 ` Jason Yan
@ 2023-04-03 3:13 ` Jason Yan
1 sibling, 0 replies; 11+ messages in thread
From: Jason Yan @ 2023-04-03 3:13 UTC (permalink / raw)
To: Damien Le Moal, martin.petersen, jejb
Cc: linux-scsi, hare, hch, bvanassche, jinpu.wang, john.g.garry
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.
^ permalink raw reply [flat|nested] 11+ messages in thread