* [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
@ 2018-07-05 9:37 Colin King
2018-07-05 9:52 ` Dan Carpenter
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Colin King @ 2018-07-05 9:37 UTC (permalink / raw)
To: David S . Miller, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Variables adv and lpa are being assigned but are never used hence they
are redundant and can be removed.
Cleans up clang warnings:
warning: variable 'lpa' set but not used [-Wunused-but-set-variable]
warning: variable 'adv' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/ethernet/sun/niu.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index 88c12474a0c3..2d6b62c6d9ab 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -1225,17 +1225,13 @@ static int link_status_1g_rgmii(struct niu *np, int *link_up_p)
bmsr = err;
if (bmsr & BMSR_LSTATUS) {
- u16 adv, lpa;
-
err = mii_read(np, np->phy_addr, MII_ADVERTISE);
if (err < 0)
goto out;
- adv = err;
err = mii_read(np, np->phy_addr, MII_LPA);
if (err < 0)
goto out;
- lpa = err;
err = mii_read(np, np->phy_addr, MII_ESTATUS);
if (err < 0)
--
2.17.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 9:37 [PATCH] net: ethernet: sun: remove redundant variables adv and lpa Colin King
@ 2018-07-05 9:52 ` Dan Carpenter
2018-07-05 9:54 ` Colin Ian King
2018-07-05 10:33 ` David Miller
2018-07-05 13:54 ` Andrew Lunn
2 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2018-07-05 9:52 UTC (permalink / raw)
To: Colin King; +Cc: David S . Miller, netdev, kernel-janitors, linux-kernel
On Thu, Jul 05, 2018 at 10:37:32AM +0100, Colin King wrote:
> diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
> index 88c12474a0c3..2d6b62c6d9ab 100644
> --- a/drivers/net/ethernet/sun/niu.c
> +++ b/drivers/net/ethernet/sun/niu.c
> @@ -1225,17 +1225,13 @@ static int link_status_1g_rgmii(struct niu *np, int *link_up_p)
>
> bmsr = err;
> if (bmsr & BMSR_LSTATUS) {
> - u16 adv, lpa;
> -
> err = mii_read(np, np->phy_addr, MII_ADVERTISE);
> if (err < 0)
> goto out;
> - adv = err;
>
> err = mii_read(np, np->phy_addr, MII_LPA);
> if (err < 0)
> goto out;
> - lpa = err;
I'm fairly sure we could get rid of the mii_read() calls as well.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 9:52 ` Dan Carpenter
@ 2018-07-05 9:54 ` Colin Ian King
2018-07-05 10:01 ` Dan Carpenter
0 siblings, 1 reply; 8+ messages in thread
From: Colin Ian King @ 2018-07-05 9:54 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David S . Miller, netdev, kernel-janitors, linux-kernel
On 05/07/18 10:52, Dan Carpenter wrote:
> On Thu, Jul 05, 2018 at 10:37:32AM +0100, Colin King wrote:
>> diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
>> index 88c12474a0c3..2d6b62c6d9ab 100644
>> --- a/drivers/net/ethernet/sun/niu.c
>> +++ b/drivers/net/ethernet/sun/niu.c
>> @@ -1225,17 +1225,13 @@ static int link_status_1g_rgmii(struct niu *np, int *link_up_p)
>>
>> bmsr = err;
>> if (bmsr & BMSR_LSTATUS) {
>> - u16 adv, lpa;
>> -
>> err = mii_read(np, np->phy_addr, MII_ADVERTISE);
>> if (err < 0)
>> goto out;
>> - adv = err;
>>
>> err = mii_read(np, np->phy_addr, MII_LPA);
>> if (err < 0)
>> goto out;
>> - lpa = err;
>
> I'm fairly sure we could get rid of the mii_read() calls as well.
I'm always concerned that removing the reads of H/W registers can affect
the behavior, so I left those in.
Colin
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 9:54 ` Colin Ian King
@ 2018-07-05 10:01 ` Dan Carpenter
0 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2018-07-05 10:01 UTC (permalink / raw)
To: Colin Ian King; +Cc: David S . Miller, netdev, kernel-janitors, linux-kernel
On Thu, Jul 05, 2018 at 10:54:51AM +0100, Colin Ian King wrote:
> On 05/07/18 10:52, Dan Carpenter wrote:
> > On Thu, Jul 05, 2018 at 10:37:32AM +0100, Colin King wrote:
> >> diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
> >> index 88c12474a0c3..2d6b62c6d9ab 100644
> >> --- a/drivers/net/ethernet/sun/niu.c
> >> +++ b/drivers/net/ethernet/sun/niu.c
> >> @@ -1225,17 +1225,13 @@ static int link_status_1g_rgmii(struct niu *np, int *link_up_p)
> >>
> >> bmsr = err;
> >> if (bmsr & BMSR_LSTATUS) {
> >> - u16 adv, lpa;
> >> -
> >> err = mii_read(np, np->phy_addr, MII_ADVERTISE);
> >> if (err < 0)
> >> goto out;
> >> - adv = err;
> >>
> >> err = mii_read(np, np->phy_addr, MII_LPA);
> >> if (err < 0)
> >> goto out;
> >> - lpa = err;
> >
> > I'm fairly sure we could get rid of the mii_read() calls as well.
>
> I'm always concerned that removing the reads of H/W registers can affect
> the behavior, so I left those in.
Yeah... That's true sometimes. Hence my "fairly sure" equivocation. I
looked to see if any of the original devs are around and it's been a
while since anyone worked on this driver.
When we remove these warnings it means that there is no chance that
someone will look at the code again and remove the unneeded mii_read()
calls. I'm 90% sure the reads are unnecessary.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 9:37 [PATCH] net: ethernet: sun: remove redundant variables adv and lpa Colin King
2018-07-05 9:52 ` Dan Carpenter
@ 2018-07-05 10:33 ` David Miller
2018-07-05 10:50 ` Colin Ian King
2018-07-05 13:54 ` Andrew Lunn
2 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2018-07-05 10:33 UTC (permalink / raw)
To: colin.king; +Cc: netdev, kernel-janitors, linux-kernel
From: Colin King <colin.king@canonical.com>
Date: Thu, 5 Jul 2018 10:37:32 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> Variables adv and lpa are being assigned but are never used hence they
> are redundant and can be removed.
>
> Cleans up clang warnings:
> warning: variable 'lpa' set but not used [-Wunused-but-set-variable]
> warning: variable 'adv' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
I think you can safely remove the register reads too, so please
do so.
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 10:33 ` David Miller
@ 2018-07-05 10:50 ` Colin Ian King
2018-07-05 10:52 ` David Miller
0 siblings, 1 reply; 8+ messages in thread
From: Colin Ian King @ 2018-07-05 10:50 UTC (permalink / raw)
To: David Miller; +Cc: netdev, kernel-janitors, linux-kernel
On 05/07/18 11:33, David Miller wrote:
> From: Colin King <colin.king@canonical.com>
> Date: Thu, 5 Jul 2018 10:37:32 +0100
>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Variables adv and lpa are being assigned but are never used hence they
>> are redundant and can be removed.
>>
>> Cleans up clang warnings:
>> warning: variable 'lpa' set but not used [-Wunused-but-set-variable]
>> warning: variable 'adv' set but not used [-Wunused-but-set-variable]
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> I think you can safely remove the register reads too, so please
> do so.
Just to clarify, just the MII_ADVERTISE and MII_LPA, or both these AND
also MII_ESTATUS too?
>
> Thanks.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 10:50 ` Colin Ian King
@ 2018-07-05 10:52 ` David Miller
0 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2018-07-05 10:52 UTC (permalink / raw)
To: colin.king; +Cc: netdev, kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Date: Thu, 5 Jul 2018 11:50:01 +0100
> Just to clarify, just the MII_ADVERTISE and MII_LPA, or both these AND
> also MII_ESTATUS too?
I think you can safely remove all three.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] net: ethernet: sun: remove redundant variables adv and lpa
2018-07-05 9:37 [PATCH] net: ethernet: sun: remove redundant variables adv and lpa Colin King
2018-07-05 9:52 ` Dan Carpenter
2018-07-05 10:33 ` David Miller
@ 2018-07-05 13:54 ` Andrew Lunn
2 siblings, 0 replies; 8+ messages in thread
From: Andrew Lunn @ 2018-07-05 13:54 UTC (permalink / raw)
To: Colin King; +Cc: David S . Miller, netdev, kernel-janitors, linux-kernel
On Thu, Jul 05, 2018 at 10:37:32AM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Variables adv and lpa are being assigned but are never used hence they
> are redundant and can be removed.
>
> Cleans up clang warnings:
> warning: variable 'lpa' set but not used [-Wunused-but-set-variable]
> warning: variable 'adv' set but not used [-Wunused-but-set-variable]
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/ethernet/sun/niu.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
> index 88c12474a0c3..2d6b62c6d9ab 100644
> --- a/drivers/net/ethernet/sun/niu.c
> +++ b/drivers/net/ethernet/sun/niu.c
> @@ -1225,17 +1225,13 @@ static int link_status_1g_rgmii(struct niu *np, int *link_up_p)
>
> bmsr = err;
> if (bmsr & BMSR_LSTATUS) {
> - u16 adv, lpa;
> -
> err = mii_read(np, np->phy_addr, MII_ADVERTISE);
> if (err < 0)
> goto out;
> - adv = err;
>
> err = mii_read(np, np->phy_addr, MII_LPA);
> if (err < 0)
> goto out;
> - lpa = err;
>
> err = mii_read(np, np->phy_addr, MII_ESTATUS);
> if (err < 0)
What should really happen is something like:
common_adv = lpa & adv;
lp->active_speed = SPEED_10;
lp->active_duplex = DUPLEX_HALF;
if (common_adv_gb & (LPA_1000FULL | LPA_1000HALF)) {
lp->active_speed = SPEED_1000;
if (common_adv_gb & LPA_1000FULL)
lp->active_duplex = DUPLEX_FULL;
} else if (common_adv & (LPA_100FULL | LPA_100HALF)) {
lp->active_speed = SPEED_100;
if (common_adv & LPA_100FULL)
lp->active_duplex = DUPLEX_FULL;
} else
if (common_adv & LPA_10FULL)
lp->active_duplex = DUPLEX_FULL;
i.e. making use of the results of the autoneg to determine the link
speed and duplex, rather than just assuming it is 1G.
If i turn this into a real patch, does anybody have the hardware to
test it?
Andrew
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-07-05 13:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-05 9:37 [PATCH] net: ethernet: sun: remove redundant variables adv and lpa Colin King
2018-07-05 9:52 ` Dan Carpenter
2018-07-05 9:54 ` Colin Ian King
2018-07-05 10:01 ` Dan Carpenter
2018-07-05 10:33 ` David Miller
2018-07-05 10:50 ` Colin Ian King
2018-07-05 10:52 ` David Miller
2018-07-05 13:54 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).