All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] gianfar: Fix possible overrun and simplify interrupt name field creation
@ 2012-03-17 19:05 Joe Perches
  2012-03-22  2:34 ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2012-03-17 19:05 UTC (permalink / raw)
  To: netdev, linux-kernel; +Cc: Sandeep Gopalpet

Space allocated for int_name_<foo> is unsufficient for
maximal device name, expand it.

Code to create int_name_<foo> is obscure, simplify it
by using sprintf.

Found by looking for unnecessary \ line continuations.
Uncompiled, untested.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/net/ethernet/freescale/gianfar.c |   39 +++++------------------------
 drivers/net/ethernet/freescale/gianfar.h |    2 +-
 2 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index adb0ae4..770b8cf 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -971,7 +971,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	struct gfar_private *priv = NULL;
 	struct gfar __iomem *regs = NULL;
 	int err = 0, i, grp_idx = 0;
-	int len_devname;
 	u32 rstat = 0, tstat = 0, rqueue = 0, tqueue = 0;
 	u32 isrg = 0;
 	u32 __iomem *baddr;
@@ -1172,40 +1171,16 @@ static int gfar_probe(struct platform_device *ofdev)
 		priv->device_flags & FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
 
 	/* fill out IRQ number and name fields */
-	len_devname = strlen(dev->name);
 	for (i = 0; i < priv->num_grps; i++) {
-		strncpy(&priv->gfargrp[i].int_name_tx[0], dev->name,
-				len_devname);
 		if (priv->device_flags & FSL_GIANFAR_DEV_HAS_MULTI_INTR) {
-			strncpy(&priv->gfargrp[i].int_name_tx[len_devname],
-				"_g", sizeof("_g"));
-			priv->gfargrp[i].int_name_tx[
-				strlen(priv->gfargrp[i].int_name_tx)] = i+48;
-			strncpy(&priv->gfargrp[i].int_name_tx[strlen(
-				priv->gfargrp[i].int_name_tx)],
-				"_tx", sizeof("_tx") + 1);
-
-			strncpy(&priv->gfargrp[i].int_name_rx[0], dev->name,
-					len_devname);
-			strncpy(&priv->gfargrp[i].int_name_rx[len_devname],
-					"_g", sizeof("_g"));
-			priv->gfargrp[i].int_name_rx[
-				strlen(priv->gfargrp[i].int_name_rx)] = i+48;
-			strncpy(&priv->gfargrp[i].int_name_rx[strlen(
-				priv->gfargrp[i].int_name_rx)],
-				"_rx", sizeof("_rx") + 1);
-
-			strncpy(&priv->gfargrp[i].int_name_er[0], dev->name,
-					len_devname);
-			strncpy(&priv->gfargrp[i].int_name_er[len_devname],
-				"_g", sizeof("_g"));
-			priv->gfargrp[i].int_name_er[strlen(
-					priv->gfargrp[i].int_name_er)] = i+48;
-			strncpy(&priv->gfargrp[i].int_name_er[strlen(\
-				priv->gfargrp[i].int_name_er)],
-				"_er", sizeof("_er") + 1);
+			sprintf(priv->gfargrp[i].int_name_tx, "%s%s%c%s",
+				dev->name, "_g", '0' + i, "_tx");
+			sprintf(priv->gfargrp[i].int_name_rx, "%s%s%c%s",
+				dev->name, "_g", '0' + i, "_rx");
+			sprintf(priv->gfargrp[i].int_name_er, "%s%s%c%s",
+				dev->name, "_g", '0' + i, "_er");
 		} else
-			priv->gfargrp[i].int_name_tx[len_devname] = '\0';
+			strcpy(priv->gfargrp[i].int_name_tx, dev->name);
 	}
 
 	/* Initialize the filer table */
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 4fe0f34..e537d81 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -520,7 +520,7 @@ extern const char gfar_driver_version[];
 #define RXFCB_PERR_MASK		0x000c
 #define RXFCB_PERR_BADL3	0x0008
 
-#define GFAR_INT_NAME_MAX	IFNAMSIZ + 4
+#define GFAR_INT_NAME_MAX	(IFNAMSIZ + 6)	/* '_g#_xx' */
 
 struct txbd8
 {
-- 
1.7.8.111.gad25c.dirty


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] gianfar: Fix possible overrun and simplify interrupt name field creation
  2012-03-17 19:05 [PATCH] gianfar: Fix possible overrun and simplify interrupt name field creation Joe Perches
@ 2012-03-22  2:34 ` David Miller
  2012-03-23  3:34   ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2012-03-22  2:34 UTC (permalink / raw)
  To: joe; +Cc: netdev, linux-kernel, sandeep.kumar

From: Joe Perches <joe@perches.com>
Date: Sat, 17 Mar 2012 12:05:38 -0700

> Space allocated for int_name_<foo> is unsufficient for
> maximal device name, expand it.
> 
> Code to create int_name_<foo> is obscure, simplify it
> by using sprintf.
> 
> Found by looking for unnecessary \ line continuations.
> Uncompiled, untested.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Can a gianfar expert please review and test this patch?

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] gianfar: Fix possible overrun and simplify interrupt name field creation
  2012-03-22  2:34 ` David Miller
@ 2012-03-23  3:34   ` David Miller
  2012-03-23 23:37     ` Paul Gortmaker
  0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2012-03-23  3:34 UTC (permalink / raw)
  To: joe; +Cc: netdev, linux-kernel, sandeep.kumar

From: David Miller <davem@davemloft.net>
Date: Wed, 21 Mar 2012 22:34:57 -0400 (EDT)

> From: Joe Perches <joe@perches.com>
> Date: Sat, 17 Mar 2012 12:05:38 -0700
> 
>> Space allocated for int_name_<foo> is unsufficient for
>> maximal device name, expand it.
>> 
>> Code to create int_name_<foo> is obscure, simplify it
>> by using sprintf.
>> 
>> Found by looking for unnecessary \ line continuations.
>> Uncompiled, untested.
>> 
>> Signed-off-by: Joe Perches <joe@perches.com>
> 
> Can a gianfar expert please review and test this patch?

If you gianfar guys don't show a sign of life in the next 24 hours I'm
just going to apply Joe's patch as-is.  I refuse to just let it rot in
patchwork like this.

And then you don't get to complain if his change breaks your driver
because I asked multiple times for testing and/or review.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] gianfar: Fix possible overrun and simplify interrupt name field creation
  2012-03-23  3:34   ` David Miller
@ 2012-03-23 23:37     ` Paul Gortmaker
  2012-03-25 17:10       ` [PATCH v2] " Paul Gortmaker
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Gortmaker @ 2012-03-23 23:37 UTC (permalink / raw)
  To: David Miller; +Cc: joe, netdev, linux-kernel, sandeep.kumar

On Thu, Mar 22, 2012 at 11:34 PM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 21 Mar 2012 22:34:57 -0400 (EDT)
>
>> From: Joe Perches <joe@perches.com>
>> Date: Sat, 17 Mar 2012 12:05:38 -0700
>>
>>> Space allocated for int_name_<foo> is unsufficient for
>>> maximal device name, expand it.
>>>
>>> Code to create int_name_<foo> is obscure, simplify it
>>> by using sprintf.
>>>
>>> Found by looking for unnecessary \ line continuations.
>>> Uncompiled, untested.
>>>
>>> Signed-off-by: Joe Perches <joe@perches.com>
>>
>> Can a gianfar expert please review and test this patch?
>
> If you gianfar guys don't show a sign of life in the next 24 hours I'm
> just going to apply Joe's patch as-is.  I refuse to just let it rot in
> patchwork like this.
>
> And then you don't get to complain if his change breaks your driver
> because I asked multiple times for testing and/or review.

I don't claim to be a giant fart expert, but the sbc8641D has 4 of
the gianfar ports, and so I can use that hardware to look into
testing this on a real board and check that a cat /proc/interrupts
doesn't do something evil this weekend.  The patch looks sane in
that it culls lines of open coded junk...  FWIW.

Paul.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] gianfar: Fix possible overrun and simplify interrupt name field creation
  2012-03-23 23:37     ` Paul Gortmaker
@ 2012-03-25 17:10       ` Paul Gortmaker
  2012-03-25 17:29         ` David Miller
  0 siblings, 1 reply; 6+ messages in thread
From: Paul Gortmaker @ 2012-03-25 17:10 UTC (permalink / raw)
  To: davem; +Cc: netdev, linux-kernel, afleming, Joe Perches, Paul Gortmaker

From: Joe Perches <joe@perches.com>

Space allocated for int_name_<foo> is insufficient for
maximal device name, expand it.

Code to create int_name_<foo> is obscure, simplify it
by using sprintf.

Found by looking for unnecessary \ line continuations.

Signed-off-by: Joe Perches <joe@perches.com>
Tested-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---

[v2: delete "uncompiled/untested" and fix typo in commit log]

The IRQ names appear unchanged after applying this patch:

root@SBC8641D:/root> for i in `seq 3` ; do ifconfig eth$i 192.168.0.$i ; done
root@SBC8641D:/root> cat /proc/interrupts
           CPU0       CPU1
 16:          0          0   MPIC      Level     phy_interrupt, phy_interrupt, phy_interrupt, phy_interrupt
 29:       6884          0   MPIC      Level     eth0_g0_tx
 30:          0      11578   MPIC      Level     eth0_g0_rx
 31:          0          0   MPIC      Level     eth2_g0_tx
 32:          0          0   MPIC      Level     eth2_g0_rx
 33:          0          0   MPIC      Level     eth2_g0_er
 34:          0          0   MPIC      Level     eth0_g0_er
 35:          0          0   MPIC      Level     eth1_g0_tx
 36:          0          0   MPIC      Level     eth1_g0_rx
 37:          0          0   MPIC      Level     eth3_g0_tx
 38:          0          0   MPIC      Level     eth3_g0_rx
 39:          0          0   MPIC      Level     eth3_g0_er
 40:          0          0   MPIC      Level     eth1_g0_er

Having them all in group "g0" may seem odd, but it is definitely
the same as it was before the commit.  Tested on mar23 linux-next.


diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index d9428f0..e7bed53 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -968,7 +968,6 @@ static int gfar_probe(struct platform_device *ofdev)
 	struct gfar_private *priv = NULL;
 	struct gfar __iomem *regs = NULL;
 	int err = 0, i, grp_idx = 0;
-	int len_devname;
 	u32 rstat = 0, tstat = 0, rqueue = 0, tqueue = 0;
 	u32 isrg = 0;
 	u32 __iomem *baddr;
@@ -1169,40 +1168,16 @@ static int gfar_probe(struct platform_device *ofdev)
 		priv->device_flags & FSL_GIANFAR_DEV_HAS_MAGIC_PACKET);
 
 	/* fill out IRQ number and name fields */
-	len_devname = strlen(dev->name);
 	for (i = 0; i < priv->num_grps; i++) {
-		strncpy(&priv->gfargrp[i].int_name_tx[0], dev->name,
-				len_devname);
 		if (priv->device_flags & FSL_GIANFAR_DEV_HAS_MULTI_INTR) {
-			strncpy(&priv->gfargrp[i].int_name_tx[len_devname],
-				"_g", sizeof("_g"));
-			priv->gfargrp[i].int_name_tx[
-				strlen(priv->gfargrp[i].int_name_tx)] = i+48;
-			strncpy(&priv->gfargrp[i].int_name_tx[strlen(
-				priv->gfargrp[i].int_name_tx)],
-				"_tx", sizeof("_tx") + 1);
-
-			strncpy(&priv->gfargrp[i].int_name_rx[0], dev->name,
-					len_devname);
-			strncpy(&priv->gfargrp[i].int_name_rx[len_devname],
-					"_g", sizeof("_g"));
-			priv->gfargrp[i].int_name_rx[
-				strlen(priv->gfargrp[i].int_name_rx)] = i+48;
-			strncpy(&priv->gfargrp[i].int_name_rx[strlen(
-				priv->gfargrp[i].int_name_rx)],
-				"_rx", sizeof("_rx") + 1);
-
-			strncpy(&priv->gfargrp[i].int_name_er[0], dev->name,
-					len_devname);
-			strncpy(&priv->gfargrp[i].int_name_er[len_devname],
-				"_g", sizeof("_g"));
-			priv->gfargrp[i].int_name_er[strlen(
-					priv->gfargrp[i].int_name_er)] = i+48;
-			strncpy(&priv->gfargrp[i].int_name_er[strlen(\
-				priv->gfargrp[i].int_name_er)],
-				"_er", sizeof("_er") + 1);
+			sprintf(priv->gfargrp[i].int_name_tx, "%s%s%c%s",
+				dev->name, "_g", '0' + i, "_tx");
+			sprintf(priv->gfargrp[i].int_name_rx, "%s%s%c%s",
+				dev->name, "_g", '0' + i, "_rx");
+			sprintf(priv->gfargrp[i].int_name_er, "%s%s%c%s",
+				dev->name, "_g", '0' + i, "_er");
 		} else
-			priv->gfargrp[i].int_name_tx[len_devname] = '\0';
+			strcpy(priv->gfargrp[i].int_name_tx, dev->name);
 	}
 
 	/* Initialize the filer table */
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index fc2488a..4c9f8d4 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -517,7 +517,7 @@ extern const char gfar_driver_version[];
 #define RXFCB_PERR_MASK		0x000c
 #define RXFCB_PERR_BADL3	0x0008
 
-#define GFAR_INT_NAME_MAX	IFNAMSIZ + 4
+#define GFAR_INT_NAME_MAX	(IFNAMSIZ + 6)	/* '_g#_xx' */
 
 struct txbd8
 {
-- 
1.7.9.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] gianfar: Fix possible overrun and simplify interrupt name field creation
  2012-03-25 17:10       ` [PATCH v2] " Paul Gortmaker
@ 2012-03-25 17:29         ` David Miller
  0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2012-03-25 17:29 UTC (permalink / raw)
  To: paul.gortmaker; +Cc: netdev, linux-kernel, afleming, joe

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Sun, 25 Mar 2012 13:10:07 -0400

> From: Joe Perches <joe@perches.com>
> 
> Space allocated for int_name_<foo> is insufficient for
> maximal device name, expand it.
> 
> Code to create int_name_<foo> is obscure, simplify it
> by using sprintf.
> 
> Found by looking for unnecessary \ line continuations.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> Tested-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Applied.

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-03-25 17:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-17 19:05 [PATCH] gianfar: Fix possible overrun and simplify interrupt name field creation Joe Perches
2012-03-22  2:34 ` David Miller
2012-03-23  3:34   ` David Miller
2012-03-23 23:37     ` Paul Gortmaker
2012-03-25 17:10       ` [PATCH v2] " Paul Gortmaker
2012-03-25 17:29         ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.