| Message ID | 1341937124.4475.27.camel@jlt3.sipsolutions.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, Jul 10, 2012 at 12:18 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > From: Johannes Berg <johannes.berg@intel.com> > > A lot of code has either the memset or an inefficient copy > from a static array that contains the all-ones broadcast Shouldn't we see all that "lot of code" here in this same commit, now using this new shortcut? If we apply this, we have a new function, but with no users. If you have done the audit, and found the inefficient cases, why isn't it here? I would think it better to just fix those people who have a pointless static array of all-ones to use the memset. If it was a multi line thing to achieve the eth_broadcast_addr() then it might make sense to exist. But as a one line alias, it does seem somewhat pointless to me. Paul. -- > address. Introduce eth_broadcast_addr() to fill an address > with all ones, making the code clearer and allowing us to > get rid of some constant arrays. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > include/linux/etherdevice.h | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h > index 3d406e0..98a27cc 100644 > --- a/include/linux/etherdevice.h > +++ b/include/linux/etherdevice.h > @@ -138,6 +138,17 @@ static inline void random_ether_addr(u8 *addr) > } > > /** > + * eth_broadcast_addr - Assign broadcast address > + * @addr: Pointer to a six-byte array containing the Ethernet address > + * > + * Assign the broadcast address to the given address array. > + */ > +static inline void eth_broadcast_addr(u8 *addr) > +{ > + memset(addr, 0xff, ETH_ALEN); > +} > + > +/** > * eth_hw_addr_random - Generate software assigned random Ethernet and > * set device flag > * @dev: pointer to net_device structure > -- > 1.7.10.4 > > > > -- > 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 -- 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
From: Paul Gortmaker <paul.gortmaker@windriver.com> Date: Tue, 10 Jul 2012 20:09:44 -0400 > On Tue, Jul 10, 2012 at 12:18 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: >> From: Johannes Berg <johannes.berg@intel.com> >> >> A lot of code has either the memset or an inefficient copy >> from a static array that contains the all-ones broadcast > > Shouldn't we see all that "lot of code" here in this same > commit, now using this new shortcut? I disagree and I intend to apply Johannes's patch as-is to net-next. -- 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
From: Johannes Berg <johannes@sipsolutions.net> Date: Tue, 10 Jul 2012 18:18:44 +0200 > From: Johannes Berg <johannes.berg@intel.com> > > A lot of code has either the memset or an inefficient copy > from a static array that contains the all-ones broadcast > address. Introduce eth_broadcast_addr() to fill an address > with all ones, making the code clearer and allowing us to > get rid of some constant arrays. > > Signed-off-by: Johannes Berg <johannes.berg@intel.com> Applied, thanks. -- 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, 2012-07-10 at 17:41 -0700, David Miller wrote: > From: Paul Gortmaker <paul.gortmaker@windriver.com> > Date: Tue, 10 Jul 2012 20:09:44 -0400 > > > On Tue, Jul 10, 2012 at 12:18 PM, Johannes Berg > > <johannes@sipsolutions.net> wrote: > >> From: Johannes Berg <johannes.berg@intel.com> > >> > >> A lot of code has either the memset or an inefficient copy > >> from a static array that contains the all-ones broadcast > > > > Shouldn't we see all that "lot of code" here in this same > > commit, now using this new shortcut? If I grepped properly, there are 42 instances of static arrays for for broadcast ethernet addresses in drivers/net and drivers/staging so it'd save some smallish amount of code by using a combination of is_broadcast_ether_addr and this new func. I think there are 53 instances of the memset(foo, 0xff, 6|ETH_ALEN). > I disagree and I intend to apply Johannes's patch as-is to net-next. Sounds fine to me. For some additional style symmetry, how about a conversion of random_ether_address to eth_random_addr too via o Rename random_ether_addr to eth_random_addr and add a #define random_ether_addr eth_random_addr o sed 's/\brandom_ether_addr\b/eth_random_addr/g' files_that_use_REA o remove the #define after awhile -- 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, 2012-07-10 at 20:09 -0400, Paul Gortmaker wrote: > On Tue, Jul 10, 2012 at 12:18 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: > > From: Johannes Berg <johannes.berg@intel.com> > > > > A lot of code has either the memset or an inefficient copy > > from a static array that contains the all-ones broadcast > > Shouldn't we see all that "lot of code" here in this same > commit, now using this new shortcut? If we apply this, we > have a new function, but with no users. If you have done > the audit, and found the inefficient cases, why isn't it here? I'm planning to fix the wireless uses (at least the ones I'm responsible for), but I'm just going to stick them into my mac80211-next tree after this patch percolates down there, I don't see a need to send around a ton of patches for it. > I would think it better to just fix those people who have a > pointless static array of all-ones to use the memset. If it was a > multi line thing to achieve the eth_broadcast_addr() then it > might make sense to exist. But as a one line alias, it does > seem somewhat pointless to me. At least in my code I'm going to prefer this over the memset for documentation purposes. I'll agree that memset(..., 0xff, ETH_ALEN) is pretty obvious already, but eth_broadcast_addr(...) is even easier to read IMHO. johannes -- 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
net-next commit ad7eee98be ("etherdevice: introduce eth_broadcast_addr")
added a new style API. Rename random_ether_addr to eth_random_addr to
create some API symmetry.
Joe Perches (8):
etherdevice: Rename random_ether_addr to eth_random_addr
ethernet: Use eth_random_addr
net: usb: Use eth_random_addr
wireless: Use eth_random_addr
drivers/net: Use eth_random_addr
s390: Use eth_random_addr
usb: Use eth_random_addr
arch: Use eth_random_addr
arch/blackfin/mach-bf537/boards/stamp.c | 2 +-
arch/c6x/kernel/soc.c | 2 +-
arch/mips/ar7/platform.c | 4 ++--
arch/mips/powertv/powertv_setup.c | 6 +++---
arch/um/drivers/net_kern.c | 2 +-
drivers/net/ethernet/atheros/atl1c/atl1c_hw.c | 2 +-
drivers/net/ethernet/atheros/atlx/atl1.c | 2 +-
drivers/net/ethernet/atheros/atlx/atl2.c | 2 +-
drivers/net/ethernet/ethoc.c | 2 +-
drivers/net/ethernet/intel/igb/igb_main.c | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 2 +-
drivers/net/ethernet/lantiq_etop.c | 2 +-
drivers/net/ethernet/micrel/ks8851.c | 2 +-
drivers/net/ethernet/micrel/ks8851_mll.c | 2 +-
drivers/net/ethernet/smsc/smsc911x.c | 2 +-
drivers/net/ethernet/ti/cpsw.c | 2 +-
drivers/net/ethernet/tile/tilegx.c | 2 +-
drivers/net/ethernet/wiznet/w5100.c | 2 +-
drivers/net/ethernet/wiznet/w5300.c | 2 +-
drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +-
drivers/net/tun.c | 2 +-
drivers/net/usb/smsc75xx.c | 2 +-
drivers/net/usb/smsc95xx.c | 2 +-
drivers/net/usb/usbnet.c | 2 +-
drivers/net/wimax/i2400m/driver.c | 2 +-
drivers/net/wireless/adm8211.c | 2 +-
drivers/net/wireless/p54/eeprom.c | 2 +-
drivers/net/wireless/rt2x00/rt2400pci.c | 2 +-
drivers/net/wireless/rt2x00/rt2500pci.c | 2 +-
drivers/net/wireless/rt2x00/rt2500usb.c | 2 +-
drivers/net/wireless/rt2x00/rt2800lib.c | 2 +-
drivers/net/wireless/rt2x00/rt61pci.c | 2 +-
drivers/net/wireless/rt2x00/rt73usb.c | 2 +-
drivers/net/wireless/rtl818x/rtl8180/dev.c | 2 +-
drivers/net/wireless/rtl818x/rtl8187/dev.c | 2 +-
drivers/s390/net/qeth_l2_main.c | 2 +-
drivers/s390/net/qeth_l3_main.c | 2 +-
drivers/usb/atm/xusbatm.c | 4 ++--
drivers/usb/gadget/u_ether.c | 2 +-
include/linux/etherdevice.h | 14 ++++++++------
40 files changed, 52 insertions(+), 50 deletions(-)
On Thu, Jul 12, 2012 at 10:33:04PM -0700, Joe Perches wrote: > net-next commit ad7eee98be ("etherdevice: introduce eth_broadcast_addr") > added a new style API. Rename random_ether_addr to eth_random_addr to > create some API symmetry. > > Joe Perches (8): > etherdevice: Rename random_ether_addr to eth_random_addr if you're really renaming the function, then this patch alone will break all of the below users. That should all be a single patch, I'm afraid. > ethernet: Use eth_random_addr > net: usb: Use eth_random_addr > wireless: Use eth_random_addr > drivers/net: Use eth_random_addr > s390: Use eth_random_addr > usb: Use eth_random_addr > arch: Use eth_random_addr > > arch/blackfin/mach-bf537/boards/stamp.c | 2 +- > arch/c6x/kernel/soc.c | 2 +- > arch/mips/ar7/platform.c | 4 ++-- > arch/mips/powertv/powertv_setup.c | 6 +++--- > arch/um/drivers/net_kern.c | 2 +- > drivers/net/ethernet/atheros/atl1c/atl1c_hw.c | 2 +- > drivers/net/ethernet/atheros/atlx/atl1.c | 2 +- > drivers/net/ethernet/atheros/atlx/atl2.c | 2 +- > drivers/net/ethernet/ethoc.c | 2 +- > drivers/net/ethernet/intel/igb/igb_main.c | 4 ++-- > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 2 +- > drivers/net/ethernet/lantiq_etop.c | 2 +- > drivers/net/ethernet/micrel/ks8851.c | 2 +- > drivers/net/ethernet/micrel/ks8851_mll.c | 2 +- > drivers/net/ethernet/smsc/smsc911x.c | 2 +- > drivers/net/ethernet/ti/cpsw.c | 2 +- > drivers/net/ethernet/tile/tilegx.c | 2 +- > drivers/net/ethernet/wiznet/w5100.c | 2 +- > drivers/net/ethernet/wiznet/w5300.c | 2 +- > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 2 +- > drivers/net/tun.c | 2 +- > drivers/net/usb/smsc75xx.c | 2 +- > drivers/net/usb/smsc95xx.c | 2 +- > drivers/net/usb/usbnet.c | 2 +- > drivers/net/wimax/i2400m/driver.c | 2 +- > drivers/net/wireless/adm8211.c | 2 +- > drivers/net/wireless/p54/eeprom.c | 2 +- > drivers/net/wireless/rt2x00/rt2400pci.c | 2 +- > drivers/net/wireless/rt2x00/rt2500pci.c | 2 +- > drivers/net/wireless/rt2x00/rt2500usb.c | 2 +- > drivers/net/wireless/rt2x00/rt2800lib.c | 2 +- > drivers/net/wireless/rt2x00/rt61pci.c | 2 +- > drivers/net/wireless/rt2x00/rt73usb.c | 2 +- > drivers/net/wireless/rtl818x/rtl8180/dev.c | 2 +- > drivers/net/wireless/rtl818x/rtl8187/dev.c | 2 +- > drivers/s390/net/qeth_l2_main.c | 2 +- > drivers/s390/net/qeth_l3_main.c | 2 +- > drivers/usb/atm/xusbatm.c | 4 ++-- > drivers/usb/gadget/u_ether.c | 2 +- > include/linux/etherdevice.h | 14 ++++++++------ > 40 files changed, 52 insertions(+), 50 deletions(-) > > -- > 1.7.8.111.gad25c.dirty > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
From: Felipe Balbi <balbi@ti.com> Date: Mon, 16 Jul 2012 13:14:38 +0300 > if you're really renaming the function, then this patch alone will break > all of the below users. That should all be a single patch, I'm afraid. It would help if you actually read his patches before saying what they might or might not do. He provides a macro in the first patch that provides the old name, and this will get removed at the end. -- 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
Hi, On Mon, Jul 16, 2012 at 03:29:01AM -0700, David Miller wrote: > From: Felipe Balbi <balbi@ti.com> > Date: Mon, 16 Jul 2012 13:14:38 +0300 > > > if you're really renaming the function, then this patch alone will break > > all of the below users. That should all be a single patch, I'm afraid. > > It would help if you actually read his patches before saying what they > might or might not do. > > He provides a macro in the first patch that provides the old name, > and this will get removed at the end. that's why I put an "if" there. The subject was misleading and I really couldn't bother going search for the patch on the mail archives. Anyway, if nothing will be broken then for drivers/usb/gadget/: Acked-by: Felipe Balbi <balbi@ti.com>
From: Felipe Balbi <balbi@ti.com> Date: Mon, 16 Jul 2012 14:12:19 +0300 > Acked-by: Felipe Balbi <balbi@ti.com> You need to provide this in a reply to the patch you actually want to ACK, so that the patch tracking system attaches your ACK to the proper patch. Thank you. -- 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
From: Joe Perches <joe@perches.com> Date: Thu, 12 Jul 2012 22:33:04 -0700 > net-next commit ad7eee98be ("etherdevice: introduce eth_broadcast_addr") > added a new style API. Rename random_ether_addr to eth_random_addr to > create some API symmetry. Series applied, thanks Joe. -- 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/include/linux/etherdevice.h b/include/linux/etherdevice.h index 3d406e0..98a27cc 100644 --- a/include/linux/etherdevice.h +++ b/include/linux/etherdevice.h @@ -138,6 +138,17 @@ static inline void random_ether_addr(u8 *addr) } /** + * eth_broadcast_addr - Assign broadcast address + * @addr: Pointer to a six-byte array containing the Ethernet address + * + * Assign the broadcast address to the given address array. + */ +static inline void eth_broadcast_addr(u8 *addr) +{ + memset(addr, 0xff, ETH_ALEN); +} + +/** * eth_hw_addr_random - Generate software assigned random Ethernet and * set device flag * @dev: pointer to net_device structure