| Message ID | 027ab4c5-57e8-10b8-816a-17c783f82323@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] net: add helper eth_set_protocol | expand |
On Tue, 6 Oct 2020 22:10:00 +0200 Heiner Kallweit wrote: > In all cases I've seen eth_type_trans() is used as in the new helper. > Biggest benefit is improved readability when replacing statements like > the following: > desc->skb->protocol = eth_type_trans(desc->skb, priv->dev); > > Coccinelle tells me that using the new helper tree-wide would touch > 313 files. Therefore I'd like to check for feedback before bothering > 100+ maintainers. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> FWIW in case you're planning to start sending conversion patches.. I'm not 100% sold on this. Maybe it's because I'm used to the call as it is. I don't feel like eth_set_protocol() expresses what eth_type_trans() does well enough. Besides there's a whole bunch of *_type_trans calls for different (old) L2s, are we going to leave those as they are? > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h > index 2e5debc03..c7f89b1bf 100644 > --- a/include/linux/etherdevice.h > +++ b/include/linux/etherdevice.h > @@ -64,6 +64,11 @@ static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) = > { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 }; > #define eth_stp_addr eth_reserved_addr_base > > +static inline void eth_set_protocol(struct sk_buff *skb, struct net_device *dev) > +{ > + skb->protocol = eth_type_trans(skb, dev); > +} > + > /** > * is_link_local_ether_addr - Determine if given Ethernet address is link-local > * @addr: Pointer to a six-byte array containing the Ethernet address
On 12.10.2020 00:13, Jakub Kicinski wrote: > On Tue, 6 Oct 2020 22:10:00 +0200 Heiner Kallweit wrote: >> In all cases I've seen eth_type_trans() is used as in the new helper. >> Biggest benefit is improved readability when replacing statements like >> the following: >> desc->skb->protocol = eth_type_trans(desc->skb, priv->dev); >> >> Coccinelle tells me that using the new helper tree-wide would touch >> 313 files. Therefore I'd like to check for feedback before bothering >> 100+ maintainers. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > FWIW in case you're planning to start sending conversion patches.. > > I'm not 100% sold on this. Maybe it's because I'm used to the call > as it is. I don't feel like eth_set_protocol() expresses what > eth_type_trans() does well enough. Besides there's a whole bunch of > *_type_trans calls for different (old) L2s, are we going to leave those > as they are? > Didn't look at the other *_type_trans functions yet. Seems like at least eth_type_trans() has old functionality that I didn't expect. Based on name and usage of the function I was under the assumption that it treats the skb as const. But it does: skb->dev = dev; skb_reset_mac_header(skb); Such (intentional) side effects are not too nice. But as this function is used in hundreds of places, I think a refactoring would need significant effort. For now I tend to leave the situation as it is. >> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h >> index 2e5debc03..c7f89b1bf 100644 >> --- a/include/linux/etherdevice.h >> +++ b/include/linux/etherdevice.h >> @@ -64,6 +64,11 @@ static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) = >> { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 }; >> #define eth_stp_addr eth_reserved_addr_base >> >> +static inline void eth_set_protocol(struct sk_buff *skb, struct net_device *dev) >> +{ >> + skb->protocol = eth_type_trans(skb, dev); >> +} >> + >> /** >> * is_link_local_ether_addr - Determine if given Ethernet address is link-local >> * @addr: Pointer to a six-byte array containing the Ethernet address >
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h index 2e5debc03..c7f89b1bf 100644 --- a/include/linux/etherdevice.h +++ b/include/linux/etherdevice.h @@ -64,6 +64,11 @@ static const u8 eth_reserved_addr_base[ETH_ALEN] __aligned(2) = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x00 }; #define eth_stp_addr eth_reserved_addr_base +static inline void eth_set_protocol(struct sk_buff *skb, struct net_device *dev) +{ + skb->protocol = eth_type_trans(skb, dev); +} + /** * is_link_local_ether_addr - Determine if given Ethernet address is link-local * @addr: Pointer to a six-byte array containing the Ethernet address
In all cases I've seen eth_type_trans() is used as in the new helper. Biggest benefit is improved readability when replacing statements like the following: desc->skb->protocol = eth_type_trans(desc->skb, priv->dev); Coccinelle tells me that using the new helper tree-wide would touch 313 files. Therefore I'd like to check for feedback before bothering 100+ maintainers. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/linux/etherdevice.h | 5 +++++ 1 file changed, 5 insertions(+)