[PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

Subsystems: intel ethernet drivers, networking drivers, the rest

STALE4052d REVIEWED: 1 (0M)

1 review trailer.

10 messages, 6 authors, 2015-06-30 · open the first message on its own page

[PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Maninder Singh <hidden>
Date: 2015-06-30 04:56:09

Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

Signed-off-by: Maninder Singh <redacted>
Reviewed-by: Yogesh Narayan Gaur <redacted>
---
 drivers/net/ethernet/intel/igb/e1000_phy.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index c1bb64d..e399b3c 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c

@@ -37,8 +37,7 @@ static s32  igb_set_master_slave_mode(struct e1000_hw *hw);
 static const u16 e1000_m88_cable_length_table[] = {
 	0, 50, 80, 110, 140, 140, E1000_CABLE_LENGTH_UNDEFINED };
 #define M88E1000_CABLE_LENGTH_TABLE_SIZE \
-	(sizeof(e1000_m88_cable_length_table) / \
-	sizeof(e1000_m88_cable_length_table[0]))
+	ARRAY_SIZE(e1000_m88_cable_length_table)
 
 static const u16 e1000_igp_2_cable_length_table[] = {
 	0, 0, 0, 0, 0, 0, 0, 0, 3, 5, 8, 11, 13, 16, 18, 21,

@@ -50,8 +49,7 @@ static const u16 e1000_igp_2_cable_length_table[] = {
 	83, 89, 95, 100, 105, 109, 113, 116, 119, 122, 124,
 	104, 109, 114, 118, 121, 124};
 #define IGP02E1000_CABLE_LENGTH_TABLE_SIZE \
-	(sizeof(e1000_igp_2_cable_length_table) / \
-	 sizeof(e1000_igp_2_cable_length_table[0]))
+	ARRAY_SIZE(e1000_igp_2_cable_length_table)
 
 /**
  *  igb_check_reset_block - Check if PHY reset is blocked

-- 
1.7.9.5

Re: [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Joe Perches <joe@perches.com>
Date: 2015-06-30 05:12:48

On Tue, 2015-06-30 at 10:25 +0530, Maninder Singh wrote:

Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

I think all of these #defines should be removed instead
as they are all used only once.

quoted hunk

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c

[]

quoted hunk

@@ -37,8 +37,7 @@ static s32  igb_set_master_slave_mode(struct e1000_hw *hw);
 static const u16 e1000_m88_cable_length_table[] = {
 	0, 50, 80, 110, 140, 140, E1000_CABLE_LENGTH_UNDEFINED };
 #define M88E1000_CABLE_LENGTH_TABLE_SIZE \
-	(sizeof(e1000_m88_cable_length_table) / \
-	sizeof(e1000_m88_cable_length_table[0]))
+	ARRAY_SIZE(e1000_m88_cable_length_table)
 
 static const u16 e1000_igp_2_cable_length_table[] = {
 	0, 0, 0, 0, 0, 0, 0, 0, 3, 5, 8, 11, 13, 16, 18, 21,
@@ -50,8 +49,7 @@ static const u16 e1000_igp_2_cable_length_table[] = {
 	83, 89, 95, 100, 105, 109, 113, 116, 119, 122, 124,
 	104, 109, 114, 118, 121, 124};
 #define IGP02E1000_CABLE_LENGTH_TABLE_SIZE \
-	(sizeof(e1000_igp_2_cable_length_table) / \
-	 sizeof(e1000_igp_2_cable_length_table[0]))
+	ARRAY_SIZE(e1000_igp_2_cable_length_table)
 
 /**
  *  igb_check_reset_block - Check if PHY reset is blocked

RE: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Fujinaka, Todd <hidden>
Date: 2015-06-30 14:53:30

I don't see the reason this is needed so I'm going to say NAK.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka@intel.com
(503) 712-4565

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On Behalf Of Maninder Singh
Sent: Monday, June 29, 2015 9:56 PM
To: Kirsher, Jeffrey T; Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: Maninder Singh; pankaj.m@samsung.com
Subject: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

Signed-off-by: Maninder Singh <redacted>
Reviewed-by: Yogesh Narayan Gaur <redacted>
---
 drivers/net/ethernet/intel/igb/e1000_phy.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index c1bb64d..e399b3c 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c

@@ -37,8 +37,7 @@ static s32  igb_set_master_slave_mode(struct e1000_hw *hw);  static const u16 e1000_m88_cable_length_table[] = {
 	0, 50, 80, 110, 140, 140, E1000_CABLE_LENGTH_UNDEFINED };  #define M88E1000_CABLE_LENGTH_TABLE_SIZE \
-	(sizeof(e1000_m88_cable_length_table) / \
-	sizeof(e1000_m88_cable_length_table[0]))
+	ARRAY_SIZE(e1000_m88_cable_length_table)
 
 static const u16 e1000_igp_2_cable_length_table[] = {
 	0, 0, 0, 0, 0, 0, 0, 0, 3, 5, 8, 11, 13, 16, 18, 21, @@ -50,8 +49,7 @@ static const u16 e1000_igp_2_cable_length_table[] = {
 	83, 89, 95, 100, 105, 109, 113, 116, 119, 122, 124,
 	104, 109, 114, 118, 121, 124};
 #define IGP02E1000_CABLE_LENGTH_TABLE_SIZE \
-	(sizeof(e1000_igp_2_cable_length_table) / \
-	 sizeof(e1000_igp_2_cable_length_table[0]))
+	ARRAY_SIZE(e1000_igp_2_cable_length_table)
 
 /**
  *  igb_check_reset_block - Check if PHY reset is blocked
--

1.7.9.5

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Richard Weinberger <hidden>
Date: 2015-06-30 19:01:28

On Tue, Jun 30, 2015 at 4:53 PM, Fujinaka, Todd [off-list ref] wrote:

I don't see the reason this is needed so I'm going to say NAK.

Using generic functions is always better than open coded stuff.
Linux's ARRAY_SIZE also makes sure that the passed variable is actually
an array.

-- 
Thanks,
//richard

RE: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Fujinaka, Todd <hidden>
Date: 2015-06-30 20:16:39

Sorry for the top-posting, but I'm provided with the tools they give me and bottom posting from Outlook just confuses email threads. Plus, this was crossposted all over creation and cc-ed to anyone with an intel address.

I still would say no if I'm allowed, because to guarantee that this change - that I don't think fixes anything - works in all cases, we need to do an incredible amount of regression testing. Every variant of every Intel part that uses this driver (and there are many) should be tested and will end up being used by the community.

Plus, you have no idea the number of obscure bugs I have to deal with as the guy answering customer questions. If this triggers some odd embedded compiler bug, I'm going to have to dig it out. Unless there is an actual bug, I'd like to leave it as it is.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka@intel.com
(503) 712-4565

-----Original Message-----
From: Richard Weinberger [mailto:richard.weinberger@gmail.com] 
Sent: Tuesday, June 30, 2015 12:01 PM
To: Fujinaka, Todd
Cc: Maninder Singh; Kirsher, Jeffrey T; Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; pankaj.m@samsung.com
Subject: Re: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

On Tue, Jun 30, 2015 at 4:53 PM, Fujinaka, Todd [off-list ref] wrote:

I don't see the reason this is needed so I'm going to say NAK.

Using generic functions is always better than open coded stuff.
Linux's ARRAY_SIZE also makes sure that the passed variable is actually an array.

--
Thanks,
//richard

Re: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Richard Weinberger <richard@nod.at>
Date: 2015-06-30 20:24:30

Hi!

Am 30.06.2015 um 22:16 schrieb Fujinaka, Todd:

Sorry for the top-posting, but I'm provided with the tools they give me and bottom posting from Outlook just confuses email threads. Plus, this was crossposted all over creation and cc-ed to anyone with an intel address.

I still would say no if I'm allowed, because to guarantee that this change - that I don't think fixes anything - works in all cases, we need to do an incredible amount of regression testing. Every variant of every Intel part that uses this driver (and there are many) should be tested and will end up being used by the community.

Plus, you have no idea the number of obscure bugs I have to deal with as the guy answering customer questions. If this triggers some odd embedded compiler bug, I'm going to have to dig it out. Unless there is an actual bug, I'd like to leave it as it is.

If you don't dare to touch your driver please update it's maintenance status.
"Supported" is definitely not the case, maybe "Odd fixes" would fit better.

Thanks,
//richard

Re: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Alex Gartrell <hidden>
Date: 2015-06-30 20:38:49

On Tue, Jun 30, 2015 at 1:16 PM, Fujinaka, Todd [off-list ref] wrote:

I still would say no if I'm allowed, because to guarantee that this change - that I don't think fixes anything - works in all cases, we need to do an incredible amount of regression testing. Every variant of every Intel part that uses this driver (and there are many) should be tested and will end up being used by the community.

Validation is really simple: diff old_module.ko new_module.ko

And this is a good defensive measure, as it'll save you when someone
screws up and changes your array to a pointer to an array (you'll get
a build failure instead of 0).

-- 
Alex Gartrell [off-list ref]

Re: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Joe Perches <joe@perches.com>
Date: 2015-06-30 20:52:45

On Tue, 2015-06-30 at 20:16 +0000, Fujinaka, Todd wrote:

Sorry for the top-posting, but I'm provided with the tools they give me
and bottom posting from Outlook just confuses email threads. Plus, this
was crossposted all over creation and cc-ed to anyone with an intel
address.

Not quite.  It was posted to the names listed under the
MAINTAINERS entry.

INTEL ETHERNET DRIVERS
M:	Jeff Kirsher [off-list ref]
R:	Jesse Brandeburg [off-list ref]
R:	Shannon Nelson [off-list ref]
R:	Carolyn Wyborny [off-list ref]
R:	Don Skidmore [off-list ref]
R:	Matthew Vick [off-list ref]
R:	John Ronciak [off-list ref]
R:	Mitch Williams [off-list ref]
L:	intel-wired-lan@lists.osuosl.org

btw: You aren't listed there Todd.  Should you be?

I still would say no if I'm allowed, because to guarantee that this
change - that I don't think fixes anything

Simplicity for the reader is generally a good thing.
Removing the macros altogether is likely better.

 - works in all cases, we
need to do an incredible amount of regression testing.

Compilers should not produce different object code.
Verification of no object changes should be good enough.

Every variant of
every Intel part that uses this driver (and there are many) should be
tested and will end up being used by the community.

Plus, you have no idea the number of obscure bugs I have to deal with
as the guy answering customer questions. If this triggers some odd
embedded compiler bug, I'm going to have to dig it out. Unless there is
an actual bug, I'd like to leave it as it is.

If any compiler miscompiles the ARRAY_SIZE macro, there are bound to
be real issues with using that compiler in a production environment.

RE: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Fujinaka, Todd <hidden>
Date: 2015-06-30 22:26:01

I've submitted a version to intel-wired-lan that addresses the original issue as well as Joe Perches's suggestion to just delete the define.

Todd Fujinaka
Software Application Engineer
Networking Division (ND)
Intel Corporation
todd.fujinaka@intel.com
(503) 712-4565

Re: [Intel-wired-lan] [PATCH 1/1] igb: Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])

From: Joe Perches <joe@perches.com>
Date: 2015-06-30 22:32:53

On Tue, 2015-06-30 at 22:25 +0000, Fujinaka, Todd wrote:

I've submitted a version to intel-wired-lan that addresses the original
issue as well as Joe Perches's suggestion to just delete the define.

Please cc netdev next time too.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help