Message ID | 9B4A1B1917080E46B64F07F2989DADD653571921@ORSMSX114.amr.corp.intel.com |
---|---|
State | Awaiting Upstream, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, Jun 30, 2015 at 4:53 PM, Fujinaka, Todd <todd.fujinaka@intel.com> 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.
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 <todd.fujinaka@intel.com> 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
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 -- 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
On Tue, Jun 30, 2015 at 1:16 PM, Fujinaka, Todd <todd.fujinaka@intel.com> 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).
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 <jeffrey.t.kirsher@intel.com> R: Jesse Brandeburg <jesse.brandeburg@intel.com> R: Shannon Nelson <shannon.nelson@intel.com> R: Carolyn Wyborny <carolyn.wyborny@intel.com> R: Don Skidmore <donald.c.skidmore@intel.com> R: Matthew Vick <matthew.vick@intel.com> R: John Ronciak <john.ronciak@intel.com> R: Mitch Williams <mitch.a.williams@intel.com> 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. -- 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
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 -- 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
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. -- 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
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};