Use ARRAY_SIZE instead fo sizeof(a)/sizeof(a[0])
Signed-off-by: Maninder Singh <[email protected]>
Reviewed-by: Yogesh Narayan Gaur <[email protected]>
---
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
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.
> diff --git 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
Hi Joe,
>> 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.
If we replcase only ARRAY_SIZE then cood will look alike
to drivers/net/ethernet/intel/e1000e/phy.c
And if we have to remove these #defines , then we can also remove it from
drivers/net/ethernet/intel/e1000e/phy.c
Because same operations and same usage is there in both files.
what say?
>> diff --git 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)
>> ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
[email protected]
(503) 712-4565
-----Original Message-----
From: Intel-wired-lan [mailto:[email protected]] 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; [email protected]; [email protected]; [email protected]
Cc: Maninder Singh; [email protected]
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 <[email protected]>
Reviewed-by: Yogesh Narayan Gaur <[email protected]>
---
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
[email protected]
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Tue, Jun 30, 2015 at 4:53 PM, Fujinaka, Todd <[email protected]> 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
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
[email protected]
(503) 712-4565
-----Original Message-----
From: Richard Weinberger [mailto:[email protected]]
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; [email protected]; [email protected]; [email protected]; [email protected]
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 <[email protected]> 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
On Tue, Jun 30, 2015 at 1:16 PM, Fujinaka, Todd <[email protected]> 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 <[email protected]>
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 <[email protected]>
R: Jesse Brandeburg <[email protected]>
R: Shannon Nelson <[email protected]>
R: Carolyn Wyborny <[email protected]>
R: Don Skidmore <[email protected]>
R: Matthew Vick <[email protected]>
R: John Ronciak <[email protected]>
R: Mitch Williams <[email protected]>
L: [email protected]
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.
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
[email protected]
(503) 712-4565
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.