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?
Yours sincerely,
Vincent Mailhol
