lkml.org 
[lkml]   [2021]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [RFC PATCH net v2] net: introduce CAN specific pointer in the struct net_device
From
Date
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

\
 
 \ /
  Last update: 2021-02-12 14:47    [W:0.026 / U:3.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site