* [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
* 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
* [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 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).