[PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[Emil Medve]

drivers/net/ucc_geth.c: In function 'ucc_geth_startup':
drivers/net/ucc_geth.c:2614: warning: assignment makes integer from pointer without a cast
drivers/net/ucc_geth.c:2651: warning: assignment makes integer from pointer without a cast

Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
---

This patch is against Jeff's tree: d85714d81cc0408daddb68c10f7fd69eafe7c213 

netdev-2.6> scripts/checkpatch.pl 0001-POWERPC-ucc_geth-Eliminate-compile-warnings.patch 
Your patch has no obvious style problems and is ready for submission.

 drivers/net/ucc_geth.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index bec413b..d427da8 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -2611,7 +2611,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 			if (UCC_GETH_TX_BD_RING_ALIGNMENT > 4)
 				align = UCC_GETH_TX_BD_RING_ALIGNMENT;
 			ugeth->tx_bd_ring_offset[j] =
-				kmalloc((u32) (length + align), GFP_KERNEL);
+				(u32)kmalloc(length + align, GFP_KERNEL);
 
 			if (ugeth->tx_bd_ring_offset[j] != 0)
 				ugeth->p_tx_bd_ring[j] =
@@ -2648,7 +2648,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 			if (UCC_GETH_RX_BD_RING_ALIGNMENT > 4)
 				align = UCC_GETH_RX_BD_RING_ALIGNMENT;
 			ugeth->rx_bd_ring_offset[j] =
-				kmalloc((u32) (length + align), GFP_KERNEL);
+				(u32)kmalloc(length + align, GFP_KERNEL);
 			if (ugeth->rx_bd_ring_offset[j] != 0)
 				ugeth->p_rx_bd_ring[j] =
 					(void*)((ugeth->rx_bd_ring_offset[j] +
-- 
1.5.3.GIT

Re: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[David Miller]

From: Emil Medve <Emilian.Medve@freescale.com>
Date: Thu, 18 Oct 2007 17:15:13 -0500

> drivers/net/ucc_geth.c: In function 'ucc_geth_startup':
> drivers/net/ucc_geth.c:2614: warning: assignment makes integer from pointer without a cast
> drivers/net/ucc_geth.c:2651: warning: assignment makes integer from pointer without a cast
> 
> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>

It only kills the warning on 32-bit systems, the cast is wrong
either way.

>  			ugeth->tx_bd_ring_offset[j] =
> -				kmalloc((u32) (length + align), GFP_KERNEL);
> +				(u32)kmalloc(length + align, GFP_KERNEL);
>  
>  			if (ugeth->tx_bd_ring_offset[j] != 0)
>  				ugeth->p_tx_bd_ring[j] =

Pointers can be up to "unsigned long" in size, therefore that
is the minimal amount of storage you need to store them into
if they are needed in integer form for some reason.

Any cast from pointer to integer like this is a huge red flag.

RE: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[Medve Emilian-EMMEDVE1]

Hello David


> It only kills the warning on 32-bit systems, the cast is wrong
> either way.

I'm not aware of the QE being present on any 64-bit PowerPC. However,
porting the entire driver to a 64-bit platform is an exercise in itself
as many other things would need tweaking the QE hardware itself. But
that's an orthogonal issue. For now I just want to fix that warning.

>=20
> >  			ugeth->tx_bd_ring_offset[j] =3D
> > -				kmalloc((u32) (length + align),=20
> GFP_KERNEL);
> > +				(u32)kmalloc(length + align,=20
> GFP_KERNEL);
> > =20
> >  			if (ugeth->tx_bd_ring_offset[j] !=3D 0)
> >  				ugeth->p_tx_bd_ring[j] =3D
>=20
> Pointers can be up to "unsigned long" in size, therefore that
> is the minimal amount of storage you need to store them into
> if they are needed in integer form for some reason.
>=20
> Any cast from pointer to integer like this is a huge red flag.

Agreed, but again, I'm not trying to fix the entire driver or to port it
to a different architecture.

For the current situation, 32-bit QE, 32-bit PowerPC, do you find the
patch acceptable?


Cheers,
Emil.

Re: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[David Miller]

From: "Medve Emilian-EMMEDVE1" <Emilian.Medve@freescale.com>
Date: Fri, 19 Oct 2007 06:39:12 -0700

> For the current situation, 32-bit QE, 32-bit PowerPC, do you find
> the patch acceptable?

No piece of code in the kernel should live in a vacuum.

In order to improve overall code quality, every piece of
driver code should avoid assuming things about pointer
sizes and things of this nature.

Then the driver can get enabled into the build on every
platform, and therefore nobody will break the build of
this driver again since it will get hit by "allmodconfig"
et al. builds even on platforms other than the one it is
meant for.

This hack fix is not acceptable, really.

RE: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[Medve Emilian-EMMEDVE1]

Hello David,


> No piece of code in the kernel should live in a vacuum.
>=20
> In order to improve overall code quality, every piece of
> driver code should avoid assuming things about pointer
> sizes and things of this nature.

I'm afraid we might be talking about orthogonal issues here. I actively
agree that all code (not only kernel) should be written up to the
coding/quality standards you mention above, but I see a difference
between fixing a warning and making a driver portable (to 64-bit
PowerPCs, to other platforms, etc.). If there is a kernel todo list
somewhere lets add to it the task to make the ucc_geth more portable.

> Then the driver can get enabled into the build on every
> platform, and therefore nobody will break the build of
> this driver again since it will get hit by "allmodconfig"
> et al. builds even on platforms other than the one it is
> meant for.
>=20
> This hack fix is not acceptable, really.

Are you suggesting we leave those warnings there until somebody decides
to fix all the portability issues of this driver? My patch is a small
and insignificant improvement and not the revolution you're asking for,
but is an small improvement today (I dislike warnings) vs. an improbable
big one in the future.

Leo, as author and maintainer of this driver, what's your take on this?


Cheers,
Emil.

RE: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[Li Yang-r58472]

> -----Original Message-----
> From: Medve Emilian-EMMEDVE1=20
> Sent: Monday, October 22, 2007 9:48 PM
> To: David Miller
> Cc: jgarzik@pobox.com; Li Yang-r58472;=20
> netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
> Subject: RE: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings
>=20
> Hello David,
>=20
>=20
> > No piece of code in the kernel should live in a vacuum.
> >=20
> > In order to improve overall code quality, every piece of=20
> driver code=20
> > should avoid assuming things about pointer sizes and things of this=20
> > nature.
>=20
> I'm afraid we might be talking about orthogonal issues here.=20
> I actively agree that all code (not only kernel) should be=20
> written up to the coding/quality standards you mention above,=20
> but I see a difference between fixing a warning and making a=20
> driver portable (to 64-bit PowerPCs, to other platforms,=20
> etc.). If there is a kernel todo list somewhere lets add to=20
> it the task to make the ucc_geth more portable.
>=20
> > Then the driver can get enabled into the build on every=20
> platform, and=20
> > therefore nobody will break the build of this driver again since it=20
> > will get hit by "allmodconfig"
> > et al. builds even on platforms other than the one it is meant for.
> >=20
> > This hack fix is not acceptable, really.
>=20
> Are you suggesting we leave those warnings there until=20
> somebody decides to fix all the portability issues of this=20
> driver? My patch is a small and insignificant improvement and=20
> not the revolution you're asking for, but is an small=20
> improvement today (I dislike warnings) vs. an improbable big=20
> one in the future.

I'd say we can not use our way of doing things while working with the
community.  The community has to consider the kernel as a whole and thus
has its own virtue.  The warning has been there for some time.  It stays
as an indicator that we have something to do to improve the portability.
I will work on a patch to fix this portability issue.

- Leo

Re: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[Scott Wood]

On Mon, Oct 22, 2007 at 06:47:32AM -0700, Medve Emilian-EMMEDVE1 wrote:
> Are you suggesting we leave those warnings there until somebody decides
> to fix all the portability issues of this driver? My patch is a small
> and insignificant improvement and not the revolution you're asking for,
> but is an small improvement today (I dislike warnings) vs. an improbable
> big one in the future.

It is not an improvement, as it moves the driver further away from being
64-bit clean.  A better fix would be to change the definition of
tx/rx_bd_ring_offset to unsigned long (or better yet, a union).

-Scott

Re: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings

[Timur Tabi]

Emil Medve wrote:
> drivers/net/ucc_geth.c: In function 'ucc_geth_startup':
> drivers/net/ucc_geth.c:2614: warning: assignment makes integer from pointer without a cast
> drivers/net/ucc_geth.c:2651: warning: assignment makes integer from pointer without a cast
> 
> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>

Acked-by: Timur Tabi <timur@freescale.com>