Hi Vincent,
On Tue, Jul 26, 2022 at 10:42 AM Vincent MAILHOL
<mailhol.vincent@xxxxxxxxxx> wrote:
>
> Hi Dario,
>
> On Tue. 26 Jul. 2022 at 16:41, Dario Binacchi
> <dario.binacchi@xxxxxxxxxxxxxxxxxxxx> wrote:
> > Hi Vincent,
> >
> > On Mon, Jul 25, 2022 at 3:32 PM Vincent Mailhol
> > <mailhol.vincent@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxx
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com
