[PATCH] gianfar: Fix warnings when built on 64-bit

[PATCH] gianfar: Fix warnings when built on 64-bit

[Scott Wood]

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(-)

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;
-- 
2.1.4

Re: [PATCH] gianfar: Fix warnings when built on 64-bit

[Arnd Bergmann]

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

RE: [PATCH] gianfar: Fix warnings when built on 64-bit

[Manoil 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.
> 
> 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

[...]

RE: [PATCH] gianfar: Fix warnings when built on 64-bit

[Manoil 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
>=20
> On Wednesday 29 July 2015 00:24:37 Scott Wood wrote:
>=20
> > 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.
>=20
> 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=
=3D9061cb023567abf081569d6851b0815dd18437e6

So if applied as it is on top of net.git it will give a headache when net-n=
ext.git
will be merged into net.git (or vice versa).

Since there are no 64-bit systems with gianfar/ eTSEC, I think that this pa=
tch
should target net-next.git (reworked to be applicable on net-next.git) to a=
void
the conflict.   I could do this rework and resend it on top of net-next.git

>=20
> >  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 =3D 0; i < priv->num_tx_queues; i++)
> >  		spin_unlock(&priv->tx_queue[i]->txlock);
> >  }
> > +#endif
> >
>=20
> This seems unrelated and should probably be a separate fix.
>=20

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 ne=
eded).
So this change can be remove from this patch.

Thanks,
Claudiu

[...]

RE: [PATCH] gianfar: Fix warnings when built on 64-bit

[Manoil 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

RE: [PATCH] gianfar: Fix warnings when built on 64-bit

[Manoil 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
>=20
> On Wednesday 29 July 2015 00:24:37 Scott Wood wrote:
>=20
> > 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.
>=20
[...]

> > @@ -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 =3D lower_32_bits(rx_queue-
> >rx_bd_dma_base);
> > +			bdp_dma +=3D (uintptr_t)bdp - (uintptr_t)base;
> > +			gfar_write(rx_queue->rfbptr, bdp_dma);
> > +		}
> >
> >  		/* Update to the next pointer */
> >  		bdp =3D next_bd(bdp, base, rx_queue->rx_ring_size);
>=20
> 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.
>=20

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
>=20
> 	bdp_dma =3D lower_32_bits(rx_queue->rx_bd_dma_base + (base -
> bdp));

I think you mean: (bdp-base)

Claudiu

Re: [PATCH] gianfar: Fix warnings when built on 64-bit

[Scott Wood]

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

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] gianfar: Fix warnings when built on 64-bit

[Scott Wood]

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

Re: [PATCH] gianfar: Fix warnings when built on 64-bit

[Arnd Bergmann]

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