[3/7] net/ethernet: Add parse_protocol header_ops support
diff mbox series

Message ID 20190114131841.1932-4-maximmi@mellanox.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • AF_PACKET transport_offset fix
Related show

Commit Message

Maxim Mikityanskiy Jan. 14, 2019, 1:19 p.m. UTC
The previous commit introduced parse_protocol callback which should
extract the protocol number from the L2 header. Make all Ethernet
devices support it.

Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
---
 include/linux/etherdevice.h |  1 +
 net/ethernet/eth.c          | 13 +++++++++++++
 2 files changed, 14 insertions(+)

Comments

Willem de Bruijn Jan. 23, 2019, 2:14 p.m. UTC | #1
On Mon, Jan 14, 2019 at 8:21 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> The previous commit introduced parse_protocol callback which should
> extract the protocol number from the L2 header. Make all Ethernet
> devices support it.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---
>  include/linux/etherdevice.h |  1 +
>  net/ethernet/eth.c          | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 2c0af7b00715..e2f3b21cd72a 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh, struct hh_cache *hh,
>                      __be16 type);
>  void eth_header_cache_update(struct hh_cache *hh, const struct net_device *dev,
>                              const unsigned char *haddr);
> +__be16 eth_header_parse_protocol(const struct sk_buff *skb);

Does not need to be exposed in the header file or exported.
Maxim Mikityanskiy Jan. 24, 2019, 9:47 a.m. UTC | #2
> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 23 January, 2019 16:15
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>
> Subject: Re: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support
> 
> On Mon, Jan 14, 2019 at 8:21 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > The previous commit introduced parse_protocol callback which should
> > extract the protocol number from the L2 header. Make all Ethernet
> > devices support it.
> >
> > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> > ---
> >  include/linux/etherdevice.h |  1 +
> >  net/ethernet/eth.c          | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > index 2c0af7b00715..e2f3b21cd72a 100644
> > --- a/include/linux/etherdevice.h
> > +++ b/include/linux/etherdevice.h
> > @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh,
> struct hh_cache *hh,
> >                      __be16 type);
> >  void eth_header_cache_update(struct hh_cache *hh, const struct net_device
> *dev,
> >                              const unsigned char *haddr);
> > +__be16 eth_header_parse_protocol(const struct sk_buff *skb);
> 
> Does not need to be exposed in the header file or exported.

Are you sure? All the other Ethernet header_ops callbacks are exported
and declared in the header. I'm not sure about the reason why it is done
in such a way, but my guess is that it will be useful if some driver
decides to replace one callback in header_ops but to use the default
ones for the rest of callbacks. If the default callbacks were not
accessible externally, it wouldn't be possible. So, I'm following the
existing pattern here, and there are reasons for that. What do you think
about it?
Willem de Bruijn Jan. 24, 2019, 2:21 p.m. UTC | #3
On Thu, Jan 24, 2019 at 4:48 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Sent: 23 January, 2019 16:15
> > To: Maxim Mikityanskiy <maximmi@mellanox.com>
> > Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> > <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> > <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> > netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> > <tariqt@mellanox.com>
> > Subject: Re: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support
> >
> > On Mon, Jan 14, 2019 at 8:21 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> > wrote:
> > >
> > > The previous commit introduced parse_protocol callback which should
> > > extract the protocol number from the L2 header. Make all Ethernet
> > > devices support it.
> > >
> > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> > > ---
> > >  include/linux/etherdevice.h |  1 +
> > >  net/ethernet/eth.c          | 13 +++++++++++++
> > >  2 files changed, 14 insertions(+)
> > >
> > > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > > index 2c0af7b00715..e2f3b21cd72a 100644
> > > --- a/include/linux/etherdevice.h
> > > +++ b/include/linux/etherdevice.h
> > > @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh,
> > struct hh_cache *hh,
> > >                      __be16 type);
> > >  void eth_header_cache_update(struct hh_cache *hh, const struct net_device
> > *dev,
> > >                              const unsigned char *haddr);
> > > +__be16 eth_header_parse_protocol(const struct sk_buff *skb);
> >
> > Does not need to be exposed in the header file or exported.
>
> Are you sure? All the other Ethernet header_ops callbacks are exported
> and declared in the header. I'm not sure about the reason why it is done
> in such a way, but my guess is that it will be useful if some driver
> decides to replace one callback in header_ops but to use the default
> ones for the rest of callbacks.

I don't exactly follow this. But I think that many are exported
because Ethernet is so common that of these are also called directly
instead of through header_ops. Looking at other header_ops
implementations, or other such callback structs, shows many examples
where the members are static local functions.

> If the default callbacks were not
> accessible externally, it wouldn't be possible. So, I'm following the
> existing pattern here, and there are reasons for that. What do you think
> about it?
Maxim Mikityanskiy Jan. 25, 2019, 8:51 a.m. UTC | #4
> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 24 January, 2019 16:22
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>
> Subject: Re: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support
> 
> On Thu, Jan 24, 2019 at 4:48 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > > Sent: 23 January, 2019 16:15
> > > To: Maxim Mikityanskiy <maximmi@mellanox.com>
> > > Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> > > <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> > > <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> > > netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq
> Toukan
> > > <tariqt@mellanox.com>
> > > Subject: Re: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops
> support
> > >
> > > On Mon, Jan 14, 2019 at 8:21 AM Maxim Mikityanskiy
> <maximmi@mellanox.com>
> > > wrote:
> > > >
> > > > The previous commit introduced parse_protocol callback which should
> > > > extract the protocol number from the L2 header. Make all Ethernet
> > > > devices support it.
> > > >
> > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> > > > ---
> > > >  include/linux/etherdevice.h |  1 +
> > > >  net/ethernet/eth.c          | 13 +++++++++++++
> > > >  2 files changed, 14 insertions(+)
> > > >
> > > > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > > > index 2c0af7b00715..e2f3b21cd72a 100644
> > > > --- a/include/linux/etherdevice.h
> > > > +++ b/include/linux/etherdevice.h
> > > > @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh,
> > > struct hh_cache *hh,
> > > >                      __be16 type);
> > > >  void eth_header_cache_update(struct hh_cache *hh, const struct
> net_device
> > > *dev,
> > > >                              const unsigned char *haddr);
> > > > +__be16 eth_header_parse_protocol(const struct sk_buff *skb);
> > >
> > > Does not need to be exposed in the header file or exported.
> >
> > Are you sure? All the other Ethernet header_ops callbacks are exported
> > and declared in the header. I'm not sure about the reason why it is done
> > in such a way, but my guess is that it will be useful if some driver
> > decides to replace one callback in header_ops but to use the default
> > ones for the rest of callbacks.
> 
> I don't exactly follow this. But I think that many are exported
> because Ethernet is so common that of these are also called directly
> instead of through header_ops. Looking at other header_ops
> implementations, or other such callback structs, shows many examples
> where the members are static local functions.

Yes, they are called directly indeed, but not all of them. E.g.,
eth_header_parse is never called directly. On the other hand, look at
drivers/net/macvlan.c:

static const struct header_ops macvlan_hard_header_ops = {
        .create         = macvlan_hard_header,
        .parse          = eth_header_parse,
        .cache          = eth_header_cache,
        .cache_update   = eth_header_cache_update,
};

This is exactly what I am talking about. In order to support it,
eth_header_parse_protocol needs to be exported. BTW, we should consider
adding it to macvlan_hard_header_ops, ipvlan_header_ops and all other
such structures.
Willem de Bruijn Jan. 25, 2019, 1:52 p.m. UTC | #5
> > > > > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > > > > index 2c0af7b00715..e2f3b21cd72a 100644
> > > > > --- a/include/linux/etherdevice.h
> > > > > +++ b/include/linux/etherdevice.h
> > > > > @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh,
> > > > struct hh_cache *hh,
> > > > >                      __be16 type);
> > > > >  void eth_header_cache_update(struct hh_cache *hh, const struct
> > net_device
> > > > *dev,
> > > > >                              const unsigned char *haddr);
> > > > > +__be16 eth_header_parse_protocol(const struct sk_buff *skb);
> > > >
> > > > Does not need to be exposed in the header file or exported.
> > >
> > > Are you sure? All the other Ethernet header_ops callbacks are exported
> > > and declared in the header. I'm not sure about the reason why it is done
> > > in such a way, but my guess is that it will be useful if some driver
> > > decides to replace one callback in header_ops but to use the default
> > > ones for the rest of callbacks.
> >
> > I don't exactly follow this. But I think that many are exported
> > because Ethernet is so common that of these are also called directly
> > instead of through header_ops. Looking at other header_ops
> > implementations, or other such callback structs, shows many examples
> > where the members are static local functions.
>
> Yes, they are called directly indeed, but not all of them. E.g.,
> eth_header_parse is never called directly. On the other hand, look at
> drivers/net/macvlan.c:
>
> static const struct header_ops macvlan_hard_header_ops = {
>         .create         = macvlan_hard_header,
>         .parse          = eth_header_parse,
>         .cache          = eth_header_cache,
>         .cache_update   = eth_header_cache_update,
> };
>
> This is exactly what I am talking about. In order to support it,
> eth_header_parse_protocol needs to be exported. BTW, we should consider
> adding it to macvlan_hard_header_ops, ipvlan_header_ops and all other
> such structures.

Very good point. Okay, export it is then.

Patch
diff mbox series

diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 2c0af7b00715..e2f3b21cd72a 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -44,6 +44,7 @@  int eth_header_cache(const struct neighbour *neigh, struct hh_cache *hh,
 		     __be16 type);
 void eth_header_cache_update(struct hh_cache *hh, const struct net_device *dev,
 			     const unsigned char *haddr);
+__be16 eth_header_parse_protocol(const struct sk_buff *skb);
 int eth_prepare_mac_addr_change(struct net_device *dev, void *p);
 void eth_commit_mac_addr_change(struct net_device *dev, void *p);
 int eth_mac_addr(struct net_device *dev, void *p);
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 4c520110b04f..f7a3d7a171c7 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -264,6 +264,18 @@  void eth_header_cache_update(struct hh_cache *hh,
 }
 EXPORT_SYMBOL(eth_header_cache_update);
 
+/**
+ * eth_header_parser_protocol - extract protocol from L2 header
+ * @skb: packet to extract protocol from
+ */
+__be16 eth_header_parse_protocol(const struct sk_buff *skb)
+{
+	const struct ethhdr *eth = eth_hdr(skb);
+
+	return eth->h_proto;
+}
+EXPORT_SYMBOL(eth_header_parse_protocol);
+
 /**
  * eth_prepare_mac_addr_change - prepare for mac change
  * @dev: network device
@@ -346,6 +358,7 @@  const struct header_ops eth_header_ops ____cacheline_aligned = {
 	.parse		= eth_header_parse,
 	.cache		= eth_header_cache,
 	.cache_update	= eth_header_cache_update,
+	.parse_protocol	= eth_header_parse_protocol,
 };
 
 /**