* [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers
@ 2016-01-10 21:26 Sergei Shtylyov
2016-01-10 21:27 ` [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init() Sergei Shtylyov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2016-01-10 21:26 UTC (permalink / raw)
To: netdev; +Cc: linux-sh
Hello.
Here's a set of 2 patches against DaveM's 'net.git' repo. While initializing
EMAC the code tries to respect the duplex mode both programmed into ECMR and
stored in its own private data -- this just can't be right.
[1/2] ravb: stop reading ECMR in ravb_emac_init()
[2/2] sh_eth: stop reading ECMR in sh_eth_dev_init()
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init()
2016-01-10 21:26 [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers Sergei Shtylyov
@ 2016-01-10 21:27 ` Sergei Shtylyov
2016-01-11 9:45 ` Geert Uytterhoeven
2016-01-10 21:28 ` [PATCH 2/2] sh_eth: stop reading ECMR in sh_eth_dev_init() Sergei Shtylyov
2016-01-11 22:31 ` [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2016-01-10 21:27 UTC (permalink / raw)
To: netdev; +Cc: linux-sh
The code in ravb_emac_init() twiddling the ECMR bits always looked a bit
strange to me: if one intends to respect 'priv->duplex', why save old value
of the ECMR.DM bit? As all the other bits are zeroed anyway, we don't
really need to read ECMR before writing to it.
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
drivers/net/ethernet/renesas/ravb_main.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
Index: net/drivers/net/ethernet/renesas/ravb_main.c
=================================--- net.orig/drivers/net/ethernet/renesas/ravb_main.c
+++ net/drivers/net/ethernet/renesas/ravb_main.c
@@ -338,16 +338,13 @@ error:
static void ravb_emac_init(struct net_device *ndev)
{
struct ravb_private *priv = netdev_priv(ndev);
- u32 ecmr;
/* Receive frame limit set register */
ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
/* PAUSE prohibition */
- ecmr = ravb_read(ndev, ECMR);
- ecmr &= ECMR_DM;
- ecmr |= ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) | ECMR_TE | ECMR_RE;
- ravb_write(ndev, ecmr, ECMR);
+ ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
+ ECMR_TE | ECMR_RE, ECMR);
ravb_set_rate(ndev);
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] sh_eth: stop reading ECMR in sh_eth_dev_init()
2016-01-10 21:26 [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers Sergei Shtylyov
2016-01-10 21:27 ` [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init() Sergei Shtylyov
@ 2016-01-10 21:28 ` Sergei Shtylyov
2016-01-11 22:31 ` [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers David Miller
2 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2016-01-10 21:28 UTC (permalink / raw)
To: netdev; +Cc: linux-sh
The code in sh_eth_dev_init() twiddling the ECMR bits always looked a bit
strange to me: if one intends to respect 'mdp->duplex', why save old value
of the ECMR.DM bit? As all the other bits are zeroed anyway, we don't really
need to read ECMR before writing to it.
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
drivers/net/ethernet/renesas/sh_eth.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
Index: net/drivers/net/ethernet/renesas/sh_eth.c
=================================--- net.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net/drivers/net/ethernet/renesas/sh_eth.c
@@ -1289,7 +1289,6 @@ static int sh_eth_dev_init(struct net_de
{
int ret = 0;
struct sh_eth_private *mdp = netdev_priv(ndev);
- u32 val;
/* Soft Reset */
ret = sh_eth_reset(ndev);
@@ -1342,10 +1341,8 @@ static int sh_eth_dev_init(struct net_de
}
/* PAUSE Prohibition */
- val = (sh_eth_read(ndev, ECMR) & ECMR_DM) |
- ECMR_ZPF | (mdp->duplex ? ECMR_DM : 0) | ECMR_TE | ECMR_RE;
-
- sh_eth_write(ndev, val, ECMR);
+ sh_eth_write(ndev, ECMR_ZPF | (mdp->duplex ? ECMR_DM : 0) |
+ ECMR_TE | ECMR_RE, ECMR);
if (mdp->cd->set_rate)
mdp->cd->set_rate(ndev);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init()
2016-01-10 21:27 ` [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init() Sergei Shtylyov
@ 2016-01-11 9:45 ` Geert Uytterhoeven
2016-01-11 11:11 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2016-01-11 9:45 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: netdev@vger.kernel.org, Linux-sh list
Hi Sergei,
On Sun, Jan 10, 2016 at 10:27 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> The code in ravb_emac_init() twiddling the ECMR bits always looked a bit
> strange to me: if one intends to respect 'priv->duplex', why save old value
> of the ECMR.DM bit? As all the other bits are zeroed anyway, we don't
> really need to read ECMR before writing to it.
Just an observation, I don't know about correctness: priv->duplex wasn't
respected if ECMR_DM was already set before.
Same comment for the second patch.
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> ---
> drivers/net/ethernet/renesas/ravb_main.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> Index: net/drivers/net/ethernet/renesas/ravb_main.c
> =================================> --- net.orig/drivers/net/ethernet/renesas/ravb_main.c
> +++ net/drivers/net/ethernet/renesas/ravb_main.c
> @@ -338,16 +338,13 @@ error:
> static void ravb_emac_init(struct net_device *ndev)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> - u32 ecmr;
>
> /* Receive frame limit set register */
> ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
>
> /* PAUSE prohibition */
> - ecmr = ravb_read(ndev, ECMR);
> - ecmr &= ECMR_DM;
Perhaps you misread as "ecmr &= ~ECMR_DM"?
> - ecmr |= ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) | ECMR_TE | ECMR_RE;
> - ravb_write(ndev, ecmr, ECMR);
> + ravb_write(ndev, ECMR_ZPF | (priv->duplex ? ECMR_DM : 0) |
> + ECMR_TE | ECMR_RE, ECMR);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init()
2016-01-11 9:45 ` Geert Uytterhoeven
@ 2016-01-11 11:11 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2016-01-11 11:11 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: netdev@vger.kernel.org, Linux-sh list
Hello.
On 1/11/2016 12:45 PM, Geert Uytterhoeven wrote:
> On Sun, Jan 10, 2016 at 10:27 PM, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
>> The code in ravb_emac_init() twiddling the ECMR bits always looked a bit
>> strange to me: if one intends to respect 'priv->duplex', why save old value
>> of the ECMR.DM bit? As all the other bits are zeroed anyway, we don't
>> really need to read ECMR before writing to it.
>
> Just an observation, I don't know about correctness: priv->duplex wasn't
> respected if ECMR_DM was already set before.
>
> Same comment for the second patch.
That's what the patches try to fix. Perhaps the description was too terse?
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>> drivers/net/ethernet/renesas/ravb_main.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> Index: net/drivers/net/ethernet/renesas/ravb_main.c
>> =================================>> --- net.orig/drivers/net/ethernet/renesas/ravb_main.c
>> +++ net/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -338,16 +338,13 @@ error:
>> static void ravb_emac_init(struct net_device *ndev)
>> {
>> struct ravb_private *priv = netdev_priv(ndev);
>> - u32 ecmr;
>>
>> /* Receive frame limit set register */
>> ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR);
>>
>> /* PAUSE prohibition */
>> - ecmr = ravb_read(ndev, ECMR);
>> - ecmr &= ECMR_DM;
>
> Perhaps you misread as "ecmr &= ~ECMR_DM"?
If it was so, there would be no point in patching...
[...]
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers
2016-01-10 21:26 [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers Sergei Shtylyov
2016-01-10 21:27 ` [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init() Sergei Shtylyov
2016-01-10 21:28 ` [PATCH 2/2] sh_eth: stop reading ECMR in sh_eth_dev_init() Sergei Shtylyov
@ 2016-01-11 22:31 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2016-01-11 22:31 UTC (permalink / raw)
To: sergei.shtylyov; +Cc: netdev, linux-sh
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 11 Jan 2016 00:26:07 +0300
> Here's a set of 2 patches against DaveM's 'net.git' repo. While initializing
> EMAC the code tries to respect the duplex mode both programmed into ECMR and
> stored in its own private data -- this just can't be right.
>
> [1/2] ravb: stop reading ECMR in ravb_emac_init()
> [2/2] sh_eth: stop reading ECMR in sh_eth_dev_init()
Series applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-11 22:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-10 21:26 [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers Sergei Shtylyov
2016-01-10 21:27 ` [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init() Sergei Shtylyov
2016-01-11 9:45 ` Geert Uytterhoeven
2016-01-11 11:11 ` Sergei Shtylyov
2016-01-10 21:28 ` [PATCH 2/2] sh_eth: stop reading ECMR in sh_eth_dev_init() Sergei Shtylyov
2016-01-11 22:31 ` [PATCH 0/2] Fix some dubious code in the Renesas Ethernet drivers David Miller
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).