Patchwork 8139too: remove unnecessary cast of ioread32()'s return value

login
register
mail settings
Submitter Junchang Wang
Date May 30, 2010, 12:22 p.m.
Message ID <20100530122213.GB1146@host-a-55.ustcsz.edu.cn>
Download mbox | patch
Permalink /patch/53996/
State Accepted
Delegated to: David Miller
Headers show

Comments

Junchang Wang - May 30, 2010, 12:22 p.m.
ioread32() returns a 32-bit integer on all platforms.
There is no need to cast its return value.

Signed-off-by: Junchang Wang <junchangwang@gmail.com>
---
 drivers/net/8139too.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

--
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - May 30, 2010, 5:35 p.m.
On 05/30/2010 08:22 AM, Junchang Wang wrote:
> ioread32() returns a 32-bit integer on all platforms.
> There is no need to cast its return value.
>
> Signed-off-by: Junchang Wang<junchangwang@gmail.com>
> ---
>   drivers/net/8139too.c |    8 ++++----
>   1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
> index 4ba7293..cc7d462 100644
> --- a/drivers/net/8139too.c
> +++ b/drivers/net/8139too.c
> @@ -662,7 +662,7 @@ static const struct ethtool_ops rtl8139_ethtool_ops;
>   /* read MMIO register */
>   #define RTL_R8(reg)		ioread8 (ioaddr + (reg))
>   #define RTL_R16(reg)		ioread16 (ioaddr + (reg))
> -#define RTL_R32(reg)		((unsigned long) ioread32 (ioaddr + (reg)))
> +#define RTL_R32(reg)		ioread32 (ioaddr + (reg))

Have you verified this matches all architectures definition of readl()?

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 30, 2010, 10:34 p.m.
From: Jeff Garzik <jeff@garzik.org>
Date: Sun, 30 May 2010 13:35:51 -0400

> Have you verified this matches all architectures definition of
> readl()?

Jeff, when you come out of hiding after months if not years
of not reviewing network driver changes, could you provide
some useful commentary instead of some trite stuff like this?

It does match, that's why I told this person to write these patches.
And if you have been following the thread where we discussed this, you
wouldn't feel the need to ask this question about these two patches.

And if it doesn't match, that's an arch bug which should be fixed and
in any event there is only one possibility of a non-match and that is
if the routine returns "unsigned long"

Which, surprise surprise Jeff, retains current behavior!

So there is no risk whatsoever possible from this change.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - May 30, 2010, 11:24 p.m.
On 05/30/2010 06:34 PM, David Miller wrote:
> From: Jeff Garzik<jeff@garzik.org>
> Date: Sun, 30 May 2010 13:35:51 -0400
>
>> Have you verified this matches all architectures definition of
>> readl()?

> And if it doesn't match, that's an arch bug which should be fixed and
> in any event there is only one possibility of a non-match and that is
> if the routine returns "unsigned long"

That was the genesis of the question.  Some arches still use unsigned long.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junchang Wang - May 31, 2010, 12:22 a.m.
On Sun, May 30, 2010 at 01:35:51PM -0400, Jeff Garzik wrote:
>
>Have you verified this matches all architectures definition of readl()?
>

Hi Jeff,

Thanks for your question. Just browsed the kernel. ioread32() returns either 
unsigned int or u32 in all arches. There is no arch that uses unsigned long 
or something else.

Secondly, There's a bug if an arch returns unsigned long. What happen when 
programmers invoke sizeof(ioread32()) on 64-bit platforms?

--Junchang
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 31, 2010, 1:29 a.m.
From: Jeff Garzik <jeff@garzik.org>
Date: Sun, 30 May 2010 19:24:18 -0400

> That was the genesis of the question.  Some arches still use unsigned
> long.

They are 32-bit.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 31, 2010, 1:35 a.m.
From: David Miller <davem@davemloft.net>
Date: Sun, 30 May 2010 18:29:48 -0700 (PDT)

> From: Jeff Garzik <jeff@garzik.org>
> Date: Sun, 30 May 2010 19:24:18 -0400
> 
>> That was the genesis of the question.  Some arches still use unsigned
>> long.
> 
> They are 32-bit.

In fact the only two offenders are h8300 and m32r, which are
both 32-bit.

This is really in the realm of "who cares."
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 31, 2010, 7:33 a.m.
From: Junchang Wang <junchangwang@gmail.com>
Date: Sun, 30 May 2010 20:22:14 +0800

> ioread32() returns a 32-bit integer on all platforms.
> There is no need to cast its return value.
> 
> Signed-off-by: Junchang Wang <junchangwang@gmail.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - June 2, 2010, 5:52 p.m.
On 05/30/2010 09:35 PM, David Miller wrote:
> From: David Miller<davem@davemloft.net>
> Date: Sun, 30 May 2010 18:29:48 -0700 (PDT)
>
>> From: Jeff Garzik<jeff@garzik.org>
>> Date: Sun, 30 May 2010 19:24:18 -0400
>>
>>> That was the genesis of the question.  Some arches still use unsigned
>>> long.
>>
>> They are 32-bit.
>
> In fact the only two offenders are h8300 and m32r, which are
> both 32-bit.

The main interesting one is an ARM sub-arch that supports PCI.


> This is really in the realm of "who cares."

Fair enough.  That answers my question.

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/8139too.c b/drivers/net/8139too.c
index 4ba7293..cc7d462 100644
--- a/drivers/net/8139too.c
+++ b/drivers/net/8139too.c
@@ -662,7 +662,7 @@  static const struct ethtool_ops rtl8139_ethtool_ops;
 /* read MMIO register */
 #define RTL_R8(reg)		ioread8 (ioaddr + (reg))
 #define RTL_R16(reg)		ioread16 (ioaddr + (reg))
-#define RTL_R32(reg)		((unsigned long) ioread32 (ioaddr + (reg)))
+#define RTL_R32(reg)		ioread32 (ioaddr + (reg))
 
 
 static const u16 rtl8139_intr_mask =
@@ -861,7 +861,7 @@  retry:
 
 	/* if unknown chip, assume array element #0, original RTL-8139 in this case */
 	dev_dbg(&pdev->dev, "unknown chip version, assuming RTL-8139\n");
-	dev_dbg(&pdev->dev, "TxConfig = 0x%lx\n", RTL_R32 (TxConfig));
+	dev_dbg(&pdev->dev, "TxConfig = 0x%x\n", RTL_R32 (TxConfig));
 	tp->chipset = 0;
 
 match:
@@ -1642,7 +1642,7 @@  static void rtl8139_tx_timeout_task (struct work_struct *work)
 	netdev_dbg(dev, "Tx queue start entry %ld  dirty entry %ld\n",
 		   tp->cur_tx, tp->dirty_tx);
 	for (i = 0; i < NUM_TX_DESC; i++)
-		netdev_dbg(dev, "Tx descriptor %d is %08lx%s\n",
+		netdev_dbg(dev, "Tx descriptor %d is %08x%s\n",
 			   i, RTL_R32(TxStatus0 + (i * 4)),
 			   i == tp->dirty_tx % NUM_TX_DESC ?
 			   " (queue head)" : "");
@@ -2486,7 +2486,7 @@  static void __set_rx_mode (struct net_device *dev)
 	int rx_mode;
 	u32 tmp;
 
-	netdev_dbg(dev, "rtl8139_set_rx_mode(%04x) done -- Rx config %08lx\n",
+	netdev_dbg(dev, "rtl8139_set_rx_mode(%04x) done -- Rx config %08x\n",
 		   dev->flags, RTL_R32(RxConfig));
 
 	/* Note: do not reorder, GCC is clever about common statements. */