Re: [PATCH v1 10/24] can: tree-wide: implement ethtool_ops::get_drvinfo()

[Dario Binacchi <dario.binacchi@amarulasolutions.com>]

Hi Vincent,

On Tue, Jul 26, 2022 at 10:42 AM Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
>
> Hi Dario,
>
> On Tue. 26 Jul. 2022 at 16:41, Dario Binacchi
> <dario.binacchi@amarulasolutions.com> wrote:
> > Hi Vincent,
> >
> > On Mon, Jul 25, 2022 at 3:32 PM Vincent Mailhol
> > <mailhol.vincent@wanadoo.fr> wrote:
> > >
> > > For all CAN drivers, implement the get_drvinfo() function.
> > >
> > > After this patch, it is now possible to retrieve basic information by
> > > doing:
> > >
> > > | $ ethtool -i canX
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > ---
> > >  drivers/net/can/at91_can.c                       | 12 ++++++++++++
> > >  drivers/net/can/c_can/c_can_main.c               | 12 ++++++++++++
> > >  drivers/net/can/can327.c                         | 11 +++++++++++
> > >  drivers/net/can/cc770/cc770.c                    | 12 ++++++++++++
> > >  drivers/net/can/ctucanfd/ctucanfd_base.c         | 12 ++++++++++++
> > >  drivers/net/can/flexcan/flexcan-core.c           | 12 ++++++++++++
> > >  drivers/net/can/grcan.c                          | 12 ++++++++++++
> > >  drivers/net/can/ifi_canfd/ifi_canfd.c            | 12 ++++++++++++
> > >  drivers/net/can/janz-ican3.c                     | 12 ++++++++++++
> > >  drivers/net/can/kvaser_pciefd.c                  | 12 ++++++++++++
> > >  drivers/net/can/m_can/m_can.c                    | 12 ++++++++++++
> > >  drivers/net/can/mscan/mscan.c                    |  1 +
> > >  drivers/net/can/pch_can.c                        | 12 ++++++++++++
> > >  drivers/net/can/peak_canfd/peak_canfd.c          | 12 ++++++++++++
> > >  drivers/net/can/rcar/rcar_can.c                  | 12 ++++++++++++
> > >  drivers/net/can/rcar/rcar_canfd.c                | 12 ++++++++++++
> > >  drivers/net/can/sja1000/sja1000.c                | 12 ++++++++++++
> > >  drivers/net/can/slcan/slcan-core.c               | 12 ++++++++++++
> > >  drivers/net/can/softing/softing_main.c           | 12 ++++++++++++
> > >  drivers/net/can/spi/hi311x.c                     | 12 ++++++++++++
> > >  drivers/net/can/spi/mcp251x.c                    | 12 ++++++++++++
> > >  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c   | 12 ++++++++++++
> > >  drivers/net/can/sun4i_can.c                      | 12 ++++++++++++
> > >  drivers/net/can/ti_hecc.c                        | 12 ++++++++++++
> > >  drivers/net/can/usb/ems_usb.c                    | 12 ++++++++++++
> > >  drivers/net/can/usb/esd_usb.c                    | 12 ++++++++++++
> > >  drivers/net/can/usb/etas_es58x/es58x_core.c      | 12 ++++++++++++
> > >  drivers/net/can/usb/gs_usb.c                     |  8 ++++++++
> > >  drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 12 ++++++++++++
> > >  drivers/net/can/usb/mcba_usb.c                   | 12 ++++++++++++
> > >  drivers/net/can/usb/peak_usb/pcan_usb_core.c     |  6 ++++++
> > >  drivers/net/can/usb/peak_usb/pcan_usb_core.h     |  2 ++
> > >  drivers/net/can/usb/peak_usb/pcan_usb_fd.c       |  1 +
> > >  drivers/net/can/usb/peak_usb/pcan_usb_pro.c      |  1 +
> > >  drivers/net/can/usb/ucan.c                       | 12 ++++++++++++
> > >  drivers/net/can/usb/usb_8dev.c                   | 12 ++++++++++++
> > >  drivers/net/can/vcan.c                           | 12 ++++++++++++
> > >  drivers/net/can/vxcan.c                          | 12 ++++++++++++
> > >  drivers/net/can/xilinx_can.c                     | 12 ++++++++++++
> > >  39 files changed, 414 insertions(+)
>
> (...)
>
> > > diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c
> > > index d1562f9474c9..1b86001c85f8 100644
> > > --- a/drivers/net/can/slcan/slcan-core.c
> > > +++ b/drivers/net/can/slcan/slcan-core.c
> > > @@ -46,6 +46,7 @@
> > >  #include <linux/string.h>
> > >  #include <linux/tty.h>
> > >  #include <linux/errno.h>
> > > +#include <linux/ethtool.h>
> > >  #include <linux/netdevice.h>
> > >  #include <linux/skbuff.h>
> > >  #include <linux/rtnetlink.h>
> > > @@ -790,6 +791,16 @@ static const struct net_device_ops slc_netdev_ops = {
> > >         .ndo_change_mtu         = slcan_change_mtu,
> > >  };
> > >
> > > +static void slcan_get_drvinfo(struct net_device *netdev,
> > > +                             struct ethtool_drvinfo *drvinfo)
> > > +{
> > > +       strscpy(drvinfo->driver, DRV_NAME, sizeof(drvinfo->driver));
> > > +}
> > > +
> >
> > Please put the function inside the slcan-ethtool.c.
> >
> > > +static const struct ethtool_ops slcan_ethtool_ops = {
> > > +       .get_drvinfo = slcan_get_drvinfo,
> > > +};
> > > +
> >
> > slcan_ethtool_ops is is already defined in slcan-ethtool.c
> >
> > >  /******************************************
> > >   *  Routines looking at TTY side.
> > >   ******************************************/
> > > @@ -869,6 +880,7 @@ static struct slcan *slc_alloc(void)
> > >
> > >         snprintf(dev->name, sizeof(dev->name), DRV_NAME "%d", i);
> > >         dev->netdev_ops = &slc_netdev_ops;
> > > +       dev->ethtool_ops = &slcan_ethtool_ops;
> > >         dev->base_addr  = i;
> > >         slcan_set_ethtool_ops(dev);
> >
> > It already sets dev->ethtool_ops.
>
> Indeed. I did not realise this. I was looking for an assignment on
> dev->ethtool_ops and missed the call to slcan_set_ethtool_ops().
>
> I am not convinced by the slcan_set_ethtool_ops(). It introduces a
> function call for no specific reasons. Instead, I am thinking to just
> export slcan_ethtool_ops like that:
>
>
> diff --git a/drivers/net/can/slcan/slcan-core.c
> b/drivers/net/can/slcan/slcan-core.c
> index d4dbeb849432..c98567c711ae 100644
> --- a/drivers/net/can/slcan/slcan-core.c
> +++ b/drivers/net/can/slcan/slcan-core.c
> @@ -868,8 +868,8 @@ static struct slcan *slc_alloc(void)
>
>         snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
>         dev->netdev_ops = &slc_netdev_ops;
> +       dev->ethtool_ops = &slcan_ethtool_ops;
>         dev->base_addr  = i;
> -       slcan_set_ethtool_ops(dev);
>         sl = netdev_priv(dev);
>
>         /* Initialize channel control data */
> diff --git a/drivers/net/can/slcan/slcan-ethtool.c
> b/drivers/net/can/slcan/slcan-ethtool.c
> index bf0afdc4e49d..328ae1fb065b 100644
> --- a/drivers/net/can/slcan/slcan-ethtool.c
> +++ b/drivers/net/can/slcan/slcan-ethtool.c
> @@ -52,14 +52,9 @@ static int slcan_get_sset_count(struct net_device
> *netdev, int sset)
>         }
>  }
>
> -static const struct ethtool_ops slcan_ethtool_ops = {
> +const struct ethtool_ops slcan_ethtool_ops = {
>         .get_strings = slcan_get_strings,
>         .get_priv_flags = slcan_get_priv_flags,
>         .set_priv_flags = slcan_set_priv_flags,
>         .get_sset_count = slcan_get_sset_count,
>  };
> -
> -void slcan_set_ethtool_ops(struct net_device *netdev)
> -{
> -       netdev->ethtool_ops = &slcan_ethtool_ops;
> -}
> diff --git a/drivers/net/can/slcan/slcan.h b/drivers/net/can/slcan/slcan.h
> index d463c8d99e22..85cedf856db3 100644
> --- a/drivers/net/can/slcan/slcan.h
> +++ b/drivers/net/can/slcan/slcan.h
> @@ -13,6 +13,7 @@
>
>  bool slcan_err_rst_on_open(struct net_device *ndev);
>  int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on);
> -void slcan_set_ethtool_ops(struct net_device *ndev);
> +
> +extern const struct ethtool_ops slcan_ethtool_ops;
>
>  #endif /* _SLCAN_H */
>
>
>
> Does it make sense?
>
>

I have already used this scheme in the c_can driver. I used this
scheme because I saw that it was used a lot
(git grep set_ethtool_ops) in the kernel. By doing so you can define
slcan_ethtool_ops as a static variable
and if possible I prefer to export functions rather than data. But it
can be a matter of taste.

Thanks and regards,
Dario

> Yours sincerely,
> Vincent Mailhol



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com