On Tue. 26 Jul. 2022 at 18:26, Dario Binacchi
<dario.binacchi@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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.
| $ git grep "void .*_set_ethtool_ops.*;" | wc -l
| 46
| $ git grep "extern const struct ethtool_ops" | wc -l
| 43
I did not know it was a good practice, but you are right, both schemes
are roughly as popular (with yours slightly more popular by a small
margin).
> 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.
My taste is to export the data (to remove a function call), but as the
maintainer, your opinion should prevail here.
And thanks for the explanation.
I will also fix those two drivers:
| $ git grep "void .*_set_ethtool_ops.*;" drivers/net/can/
| drivers/net/can/c_can/c_can.h:void c_can_set_ethtool_ops(struct
net_device *dev);
| drivers/net/can/flexcan/flexcan.h:void
flexcan_set_ethtool_ops(struct net_device *dev);
Yours sincerely,
Vincent Mailhol
