| Message ID | 1438147477-393-1-git-send-email-scottwood@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Scott Wood |
| Headers | show |
On Wednesday 29 July 2015 00:24:37 Scott Wood wrote: > Alternatively, if there's a desire to not mess with this code (I don't > know how to trigger this code path to test it), this driver should be > given dependencies that ensure that it only builds on 32-bit. These are obvious fixes, they should definitely go in. > drivers/net/ethernet/freescale/gianfar.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index ff87502..7c682ac 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -565,6 +565,7 @@ static void gfar_ints_enable(struct gfar_private *priv) > } > } > > +#ifdef CONFIG_PM > static void lock_tx_qs(struct gfar_private *priv) > { > int i; > @@ -580,6 +581,7 @@ static void unlock_tx_qs(struct gfar_private *priv) > for (i = 0; i < priv->num_tx_queues; i++) > spin_unlock(&priv->tx_queue[i]->txlock); > } > +#endif > This seems unrelated and should probably be a separate fix. > @@ -2964,8 +2967,13 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit) > gfar_init_rxbdp(rx_queue, bdp, bufaddr); > > /* Update Last Free RxBD pointer for LFC */ > - if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) > - gfar_write(rx_queue->rfbptr, (u32)bdp); > + if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) { > + u32 bdp_dma; > + > + bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base); > + bdp_dma += (uintptr_t)bdp - (uintptr_t)base; > + gfar_write(rx_queue->rfbptr, bdp_dma); > + } > > /* Update to the next pointer */ > bdp = next_bd(bdp, base, rx_queue->rx_ring_size); You are fixing two problems here: the warning about a size cast, and the fact that the driver is using the wrong pointer. I'd suggest explaining it in the changelog. Note that we normally rely on void pointer arithmetic in the kernel, so I'd write it without the uintptr_t casts as bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base + (base - bdp)); Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Wednesday, July 29, 2015 11:02 AM > To: linuxppc-dev@lists.ozlabs.org; netdev@vger.kernel.org; Manoil Claudiu- > B08782; Wood Scott-B07421 > Subject: Re: [PATCH] gianfar: Fix warnings when built on 64-bit > > On Wednesday 29 July 2015 00:24:37 Scott Wood wrote: > > > Alternatively, if there's a desire to not mess with this code (I don't > > know how to trigger this code path to test it), this driver should be > > given dependencies that ensure that it only builds on 32-bit. > > These are obvious fixes, they should definitely go in. This patch conflicts with the rx s/g patch series: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=9061cb023567abf081569d6851b0815dd18437e6 So if applied as it is on top of net.git it will give a headache when net-next.git will be merged into net.git (or vice versa). Since there are no 64-bit systems with gianfar/ eTSEC, I think that this patch should target net-next.git (reworked to be applicable on net-next.git) to avoid the conflict. I could do this rework and resend it on top of net-next.git > > > drivers/net/ethernet/freescale/gianfar.c | 22 ++++++++++++++++++---- > > 1 file changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/gianfar.c > b/drivers/net/ethernet/freescale/gianfar.c > > index ff87502..7c682ac 100644 > > --- a/drivers/net/ethernet/freescale/gianfar.c > > +++ b/drivers/net/ethernet/freescale/gianfar.c > > @@ -565,6 +565,7 @@ static void gfar_ints_enable(struct gfar_private > *priv) > > } > > } > > > > +#ifdef CONFIG_PM > > static void lock_tx_qs(struct gfar_private *priv) > > { > > int i; > > @@ -580,6 +581,7 @@ static void unlock_tx_qs(struct gfar_private *priv) > > for (i = 0; i < priv->num_tx_queues; i++) > > spin_unlock(&priv->tx_queue[i]->txlock); > > } > > +#endif > > > > This seems unrelated and should probably be a separate fix. > I'm working at a patch set to revive/ cleanup the power management code, and lock_tx_qs() is planned to be removed (it can be shown that it's not needed). So this change can be remove from this patch. Thanks, Claudiu [...]
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Wednesday, July 29, 2015 11:02 AM > To: linuxppc-dev@lists.ozlabs.org; netdev@vger.kernel.org; Manoil Claudiu- > B08782; Wood Scott-B07421 > Subject: Re: [PATCH] gianfar: Fix warnings when built on 64-bit > > On Wednesday 29 July 2015 00:24:37 Scott Wood wrote: > > > Alternatively, if there's a desire to not mess with this code (I don't > > know how to trigger this code path to test it), this driver should be > > given dependencies that ensure that it only builds on 32-bit. > [...] > > @@ -2964,8 +2967,13 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q > *rx_queue, int rx_work_limit) > > gfar_init_rxbdp(rx_queue, bdp, bufaddr); > > > > /* Update Last Free RxBD pointer for LFC */ > > - if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) > > - gfar_write(rx_queue->rfbptr, (u32)bdp); > > + if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) { > > + u32 bdp_dma; > > + > > + bdp_dma = lower_32_bits(rx_queue- > >rx_bd_dma_base); > > + bdp_dma += (uintptr_t)bdp - (uintptr_t)base; > > + gfar_write(rx_queue->rfbptr, bdp_dma); > > + } > > > > /* Update to the next pointer */ > > bdp = next_bd(bdp, base, rx_queue->rx_ring_size); > > You are fixing two problems here: the warning about a size cast, and > the fact that the driver is using the wrong pointer. I'd suggest > explaining it in the changelog. > What would be the wrong pointer here? "base"? "base" is " rx_queue->rx_bd_base". > Note that we normally rely on void pointer arithmetic in the kernel, so > I'd write it without the uintptr_t casts as > > bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base + (base - > bdp)); I think you mean: (bdp-base) Claudiu
On Wed, 2015-07-29 at 10:02 +0200, Arnd Bergmann wrote: > On Wednesday 29 July 2015 00:24:37 Scott Wood wrote: > > +#ifdef CONFIG_PM > > static void lock_tx_qs(struct gfar_private *priv) > > { > > int i; > > @@ -580,6 +581,7 @@ static void unlock_tx_qs(struct gfar_private *priv) > > for (i = 0; i < priv->num_tx_queues; i++) > > spin_unlock(&priv->tx_queue[i]->txlock); > > } > > +#endif > > > > This seems unrelated and should probably be a separate fix. It's related in that it fixes a warning -- the 64-bit build didn't have CONFIG_PM -- though I should have been clearer about that in the changelog. > > @@ -2964,8 +2967,13 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q > > *rx_queue, int rx_work_limit) > > gfar_init_rxbdp(rx_queue, bdp, bufaddr); > > > > /* Update Last Free RxBD pointer for LFC */ > > - if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) > > - gfar_write(rx_queue->rfbptr, (u32)bdp); > > + if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) { > > + u32 bdp_dma; > > + > > + bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base); > > + bdp_dma += (uintptr_t)bdp - (uintptr_t)base; > > + gfar_write(rx_queue->rfbptr, bdp_dma); > > + } > > > > /* Update to the next pointer */ > > bdp = next_bd(bdp, base, rx_queue->rx_ring_size); > > You are fixing two problems here: the warning about a size cast, and > the fact that the driver is using the wrong pointer. I'd suggest > explaining it in the changelog. > > Note that we normally rely on void pointer arithmetic in the kernel, so > I'd write it without the uintptr_t casts as > > bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base + (base - bdp)); But those aren't void pointers, and rx_bd_dma_base isn't a pointer, so you'd get the wrong answer doing that. -Scott
On Wednesday 29 July 2015 11:02:41 Scott Wood wrote: > On Wed, 2015-07-29 at 10:02 +0200, Arnd Bergmann wrote: > > On Wednesday 29 July 2015 00:24:37 Scott Wood wrote: > > > +#ifdef CONFIG_PM > > > static void lock_tx_qs(struct gfar_private *priv) > > > { > > > int i; > > > @@ -580,6 +581,7 @@ static void unlock_tx_qs(struct gfar_private *priv) > > > for (i = 0; i < priv->num_tx_queues; i++) > > > spin_unlock(&priv->tx_queue[i]->txlock); > > > } > > > +#endif > > > > > > > This seems unrelated and should probably be a separate fix. > > It's related in that it fixes a warning -- the 64-bit build didn't have > CONFIG_PM -- though I should have been clearer about that in the changelog. Yes, that's what I meant: you can easily have a 32-bit build without CONFIG_PM of course, and that would have the same problem. > > > > You are fixing two problems here: the warning about a size cast, and > > the fact that the driver is using the wrong pointer. I'd suggest > > explaining it in the changelog. > > > > Note that we normally rely on void pointer arithmetic in the kernel, so > > I'd write it without the uintptr_t casts as > > > > bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base + (base - bdp)); > > But those aren't void pointers, and rx_bd_dma_base isn't a pointer, so you'd > get the wrong answer doing that. Ah, right. Arnd
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index ff87502..7c682ac 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -565,6 +565,7 @@ static void gfar_ints_enable(struct gfar_private *priv) } } +#ifdef CONFIG_PM static void lock_tx_qs(struct gfar_private *priv) { int i; @@ -580,6 +581,7 @@ static void unlock_tx_qs(struct gfar_private *priv) for (i = 0; i < priv->num_tx_queues; i++) spin_unlock(&priv->tx_queue[i]->txlock); } +#endif static int gfar_alloc_tx_queues(struct gfar_private *priv) { @@ -2655,7 +2657,8 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) { struct skb_shared_hwtstamps shhwtstamps; - u64 *ns = (u64*) (((u32)skb->data + 0x10) & ~0x7); + u64 *ns = (u64 *)(((uintptr_t)skb->data + 0x10) & + ~0x7UL); memset(&shhwtstamps, 0, sizeof(shhwtstamps)); shhwtstamps.hwtstamp = ns_to_ktime(*ns); @@ -2964,8 +2967,13 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit) gfar_init_rxbdp(rx_queue, bdp, bufaddr); /* Update Last Free RxBD pointer for LFC */ - if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) - gfar_write(rx_queue->rfbptr, (u32)bdp); + if (unlikely(rx_queue->rfbptr && priv->tx_actual_en)) { + u32 bdp_dma; + + bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base); + bdp_dma += (uintptr_t)bdp - (uintptr_t)base; + gfar_write(rx_queue->rfbptr, bdp_dma); + } /* Update to the next pointer */ bdp = next_bd(bdp, base, rx_queue->rx_ring_size); @@ -3551,6 +3559,8 @@ static noinline void gfar_update_link_state(struct gfar_private *priv) /* Turn last free buffer recording on */ if ((tempval1 & MACCFG1_TX_FLOW) && !tx_flow_oldval) { for (i = 0; i < priv->num_rx_queues; i++) { + u32 bdp_dma; + rx_queue = priv->rx_queue[i]; bdp = rx_queue->cur_rx; /* skip to previous bd */ @@ -3558,8 +3568,12 @@ static noinline void gfar_update_link_state(struct gfar_private *priv) rx_queue->rx_bd_base, rx_queue->rx_ring_size); + bdp_dma = lower_32_bits(rx_queue->rx_bd_dma_base); + bdp_dma += (uintptr_t)bdp - + (uintptr_t)rx_queue->rx_bd_base; + if (rx_queue->rfbptr) - gfar_write(rx_queue->rfbptr, (u32)bdp); + gfar_write(rx_queue->rfbptr, bdp_dma); } priv->tx_actual_en = 1;
As part of defconfig consolidation using fragments, we'd like to be able to have the same drivers enabled on 32-bit and 64-bit. Gianfar happens to only exist on 32-bit systems, and when building the resulting 64-bit kernel warnings were produced. A couple of the warnings are trivial, but the rfbptr code has deeper issues. It uses the virtual address as the DMA address, which again, happens to work in the environments where this driver is currently used, but is not the right thing to do. Signed-off-by: Scott Wood <scottwood@freescale.com> --- Alternatively, if there's a desire to not mess with this code (I don't know how to trigger this code path to test it), this driver should be given dependencies that ensure that it only builds on 32-bit. --- drivers/net/ethernet/freescale/gianfar.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)