Messages in this thread Patch in this message | | | Subject | Re: [RFC PATCH net v2] net: introduce CAN specific pointer in the struct net_device | From | Oliver Hartkopp <> | Date | Fri, 12 Feb 2021 14:43:07 +0100 |
| |
Hello Oleksij,
nice cleanup - and I like the removal of the notifier in af_can.c :-)
Two questions/hints from my side:
On 12.02.21 13:52, Oleksij Rempel wrote:
> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c > index d9281ae853f8..912401788d93 100644 > --- a/drivers/net/can/dev/dev.c > +++ b/drivers/net/can/dev/dev.c > @@ -239,6 +239,7 @@ void can_setup(struct net_device *dev) > struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max, > unsigned int txqs, unsigned int rxqs) > { > + struct can_ml_priv *can;
This should not be named 'can' but e.g. 'can_ml'.
'can' is already used for the struct netns_can:
$ git grep netns_can include/net/net_namespace.h: struct netns_can can; include/net/netns/can.h:struct netns_can {
which is also used in af_can.c and will create some naming confusion.
Maybe the latter could be renamed to can_ns too (later).
But 'can' alone does not tell what the variable is about IMO.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index bfadf3b82f9c..9a4c6d14098c 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1584,6 +1584,16 @@ enum netdev_priv_flags { > #define IFF_L3MDEV_RX_HANDLER IFF_L3MDEV_RX_HANDLER > #define IFF_LIVE_RENAME_OK IFF_LIVE_RENAME_OK > > +/** > + * enum netdev_ml_priv_type - &struct net_device ml_priv_type > + * > + * This enum specifies the type of the struct net_device::ml_priv pointer. > + */ > +enum netdev_ml_priv_type { > + ML_PRIV_NONE, > + ML_PRIV_CAN, > +}; > + > /** > * struct net_device - The DEVICE structure. > * > @@ -1779,6 +1789,7 @@ enum netdev_priv_flags { > * @nd_net: Network namespace this network device is inside > * > * @ml_priv: Mid-layer private > + @ml_priv_type: Mid-layer private type > * @lstats: Loopback statistics > * @tstats: Tunnel statistics > * @dstats: Dummy statistics > @@ -2100,6 +2111,7 @@ struct net_device { > struct pcpu_sw_netstats __percpu *tstats; > struct pcpu_dstats __percpu *dstats; > }; > + enum netdev_ml_priv_type ml_priv_type;
I wonder if it makes more sense to *remove* ml_priv from this union in include/linux/netdevice.h and just put it behind the union:
/* mid-layer private */ union { void *ml_priv; struct pcpu_lstats __percpu *lstats; struct pcpu_sw_netstats __percpu *tstats; struct pcpu_dstats __percpu *dstats; }; When doing git grep for ml_priv a bunch of users shows up - which all have nothing to do with statistics.
I just looks fishy to combine things into a union that have a completely different purpose - and we might finally run into similar problems like today. Best regards, Oliver
| |