| Message ID | 20231122102540.3766699-1-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: enetc: add ethtool::get_channels support | expand |
>Subject: [EXT] [PATCH net-next] net: enetc: add ethtool::get_channels >support > >External Email > >---------------------------------------------------------------------- >Since ETHTOOL_MSG_RSS_GET netlink message [1] has been applied to >ethtool tree, there is a netlink error when using "ethtool -x eno0" >command to get RSS information from fsl-enetc driver, and the user >cannot get the information, the error logs are as follows: > >root@ls1028ardb:~# ./ethtool -x eno0 >netlink error: Operation not supported > >The rationale is that ethtool will issue a ETHTOOL_MSG_CHANNELS_GET >netlink message to get the number of Rx ring. However, the fsl-enetc >driver doesn't support ethtool::get_channels, so it directly returns - >EOPNOTSUPP error. > >[1]: https://urldefense.proofpoint.com/v2/url?u=https- >3A__git.kernel.org_pub_scm_linux_kernel_git_jkirsher_ethtool.git_commit_ >-3Fid- >3Dffab99c1f3820e21d65686e030dcf2c4fd0a8bd0&d=DwIDAg&c=nKjWec2b6R0mOyPaz7 >xtfQ&r=7si3Xn9Ly-Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=- >6bW3FaCKau7jio6XSUWDZw3IEqdetIwhU_Bgcv87QcnjyMDGD0slJGQYFlbVx7l&s=8vMevY >UEvNkyCjMDO278j67ir4daMquk6QK9wR65NSQ&e= > >Signed-off-by: Wei Fang <wei.fang@nxp.com> >--- > .../net/ethernet/freescale/enetc/enetc_ethtool.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > >diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c >b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c >index e993ed04ab57..5fa1000b9b83 100644 >--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c >+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c >@@ -740,6 +740,17 @@ static int enetc_set_rxfh(struct net_device *ndev, >const u32 *indir, > return err; > } > >+static void enetc_get_channels(struct net_device *ndev, >+ struct ethtool_channels *ch) >+{ >+ struct enetc_ndev_priv *priv = netdev_priv(ndev); >+ >+ ch->max_rx = priv->num_rx_rings; >+ ch->max_tx = priv->num_tx_rings; >+ ch->rx_count = priv->num_rx_rings; >+ ch->tx_count = priv->num_tx_rings; [Suman] max_rx/tx and rx/tx_count can be different right? To me it seems like max_rx/tx should read the max value not the current configured values. >+} >+ > static void enetc_get_ringparam(struct net_device *ndev, > struct ethtool_ringparam *ring, > struct kernel_ethtool_ringparam *kernel_ring, @@ - >1196,6 +1207,7 @@ static const struct ethtool_ops enetc_pf_ethtool_ops = >{ > .get_rxfh_indir_size = enetc_get_rxfh_indir_size, > .get_rxfh = enetc_get_rxfh, > .set_rxfh = enetc_set_rxfh, >+ .get_channels = enetc_get_channels, > .get_ringparam = enetc_get_ringparam, > .get_coalesce = enetc_get_coalesce, > .set_coalesce = enetc_set_coalesce, >@@ -1226,6 +1238,7 @@ static const struct ethtool_ops >enetc_vf_ethtool_ops = { > .get_rxfh_indir_size = enetc_get_rxfh_indir_size, > .get_rxfh = enetc_get_rxfh, > .set_rxfh = enetc_set_rxfh, >+ .get_channels = enetc_get_channels, > .get_ringparam = enetc_get_ringparam, > .get_coalesce = enetc_get_coalesce, > .set_coalesce = enetc_set_coalesce, >-- >2.25.1 >
Hi Wei, On Wed, Nov 22, 2023 at 06:25:40PM +0800, Wei Fang wrote: > Since ETHTOOL_MSG_RSS_GET netlink message [1] has been applied to > ethtool tree, there is a netlink error when using "ethtool -x eno0" > command to get RSS information from fsl-enetc driver, and the user > cannot get the information, the error logs are as follows: > > root@ls1028ardb:~# ./ethtool -x eno0 > netlink error: Operation not supported > > The rationale is that ethtool will issue a ETHTOOL_MSG_CHANNELS_GET > netlink message to get the number of Rx ring. However, the fsl-enetc > driver doesn't support ethtool::get_channels, so it directly returns > -EOPNOTSUPP error. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/ethtool.git/commit/?id=ffab99c1f3820e21d65686e030dcf2c4fd0a8bd0 > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > --- I think we have 2 problems on our hands. 1. enetc is not the only driver that doesn't report ETHTOOL_MSG_CHANNELS_GET. So it is a general issue for Sudheer Mogilappagari's implementation of "ethtool -x" using netlink. The ioctl-based implementation used to look at ETHTOOL_GRXRINGS which was handled in the kernel through ethtool_ops :: get_rxnfc. 2. I used to have a different implementation (and interpretation) of ETHTOOL_MSG_CHANNELS_GET for enetc anyway, which associated channels not with rings, but with interrupt vectors (making the reported channels combined): https://patchwork.kernel.org/project/netdevbpf/patch/20230206100837.451300-6-vladimir.oltean@nxp.com/ I would suggest finding a way for the user space implementation to not assume that ETHTOOL_MSG_CHANNELS_GET is implemented by the driver.
> -----Original Message----- > From: Suman Ghosh <sumang@marvell.com> > Sent: 2023年11月22日 18:50 > To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Claudiu > Manoil <claudiu.manoil@nxp.com>; Vladimir Oltean > <vladimir.oltean@nxp.com>; netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Subject: RE: [EXT] [PATCH net-next] net: enetc: add ethtool::get_channels > support > > >Subject: [EXT] [PATCH net-next] net: enetc: add ethtool::get_channels > >support > > > >External Email > > > >---------------------------------------------------------------------- > >Since ETHTOOL_MSG_RSS_GET netlink message [1] has been applied to > >ethtool tree, there is a netlink error when using "ethtool -x eno0" > >command to get RSS information from fsl-enetc driver, and the user > >cannot get the information, the error logs are as follows: > > > >root@ls1028ardb:~# ./ethtool -x eno0 > >netlink error: Operation not supported > > > >The rationale is that ethtool will issue a ETHTOOL_MSG_CHANNELS_GET > >netlink message to get the number of Rx ring. However, the fsl-enetc > >driver doesn't support ethtool::get_channels, so it directly returns - > >EOPNOTSUPP error. > > > >[1]: > >https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furlde > >fense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-&data=05%7C01%7Cwei.f > ang%40 > >nxp.com%7Ccb29522a88634f39882d08dbeb48c6b0%7C686ea1d3bc2b4c6fa > 92cd99c5c > >301635%7C0%7C0%7C638362470026457333%7CUnknown%7CTWFpbGZsb3 > d8eyJWIjoiMC4 > >wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000% > 7C%7C%7C > >&sdata=j8g%2Filjj0mneiVEbRln1QpxFtBbudmQDfzqBqyfP9%2BY%3D&reserv > ed=0 > >3A__git.kernel.org_pub_scm_linux_kernel_git_jkirsher_ethtool.git_commit > >_ > >-3Fid- > >3Dffab99c1f3820e21d65686e030dcf2c4fd0a8bd0&d=DwIDAg&c=nKjWec2b > 6R0mOyPaz > >7 > >xtfQ&r=7si3Xn9Ly-Se1a655kvEPIYU0nQ9HPeN280sEUv5ROU&m=- > >6bW3FaCKau7jio6XSUWDZw3IEqdetIwhU_Bgcv87QcnjyMDGD0slJGQYFlbVx7 > l&s=8vMev > >Y UEvNkyCjMDO278j67ir4daMquk6QK9wR65NSQ&e= > > > >Signed-off-by: Wei Fang <wei.fang@nxp.com> > >--- > > .../net/ethernet/freescale/enetc/enetc_ethtool.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > >diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > >b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > >index e993ed04ab57..5fa1000b9b83 100644 > >--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > >+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > >@@ -740,6 +740,17 @@ static int enetc_set_rxfh(struct net_device *ndev, > >const u32 *indir, > > return err; > > } > > > >+static void enetc_get_channels(struct net_device *ndev, > >+ struct ethtool_channels *ch) { > >+ struct enetc_ndev_priv *priv = netdev_priv(ndev); > >+ > >+ ch->max_rx = priv->num_rx_rings; > >+ ch->max_tx = priv->num_tx_rings; > >+ ch->rx_count = priv->num_rx_rings; > >+ ch->tx_count = priv->num_tx_rings; > [Suman] max_rx/tx and rx/tx_count can be different right? To me it seems like > max_rx/tx should read the max value not the current configured values. [Wei] Yes, but the current fsl-enetc driver doesn't support to dynamically configure the number of tx/rx rings, so even we have more available rings than priv->num_rx_rings, we can only the use priv->num_rx_rings rings. So I make the max_rx = rx_count.
> -----Original Message----- > From: Vladimir Oltean <vladimir.oltean@nxp.com> > Sent: 2023年11月22日 19:01 > To: Wei Fang <wei.fang@nxp.com> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Claudiu Manoil <claudiu.manoil@nxp.com>; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next] net: enetc: add ethtool::get_channels support > > Hi Wei, > > On Wed, Nov 22, 2023 at 06:25:40PM +0800, Wei Fang wrote: > > Since ETHTOOL_MSG_RSS_GET netlink message [1] has been applied to > > ethtool tree, there is a netlink error when using "ethtool -x eno0" > > command to get RSS information from fsl-enetc driver, and the user > > cannot get the information, the error logs are as follows: > > > > root@ls1028ardb:~# ./ethtool -x eno0 > > netlink error: Operation not supported > > > > The rationale is that ethtool will issue a ETHTOOL_MSG_CHANNELS_GET > > netlink message to get the number of Rx ring. However, the fsl-enetc > > driver doesn't support ethtool::get_channels, so it directly returns > > -EOPNOTSUPP error. > > > > [1]: > > https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/ethtool.git/c > > ommit/?id=ffab99c1f3820e21d65686e030dcf2c4fd0a8bd0 > > > > Signed-off-by: Wei Fang <wei.fang@nxp.com> > > --- > > I think we have 2 problems on our hands. > > 1. enetc is not the only driver that doesn't report > ETHTOOL_MSG_CHANNELS_GET. > So it is a general issue for Sudheer Mogilappagari's implementation > of "ethtool -x" using netlink. The ioctl-based implementation used to > look at ETHTOOL_GRXRINGS which was handled in the kernel through > ethtool_ops :: get_rxnfc. > > 2. I used to have a different implementation (and interpretation) of > ETHTOOL_MSG_CHANNELS_GET for enetc anyway, which associated > channels > not with rings, but with interrupt vectors (making the reported > channels combined): > > https://patchwork.kernel.org/project/netdevbpf/patch/20230206100837.451 > 300-6-vladimir.oltean@nxp.com/ > [Wei] Sorry, I didn't notice you patch before, I think you patch is more better than mine after I read the "channel" interpretation in napi.rst doc. > I would suggest finding a way for the user space implementation to not > assume that ETHTOOL_MSG_CHANNELS_GET is implemented by the driver. [Wei] IMO, the issue you encountered is that libbpf will perform an ETHTOOL_GCHANNELS operation. The issue I encountered is that "ethtool -x" will also perform an ETHTOOL_GCHANNELS operation. Besides, There are other apps that do the same operation as this, so I think it's best for fsl-enetc driver to support querying channels. Because your patch is more reasonable than mine, I think you should submit this patch to upstream separately first.
On Thu, Nov 23, 2023 at 04:09:57AM +0200, Wei Fang wrote: > > I would suggest finding a way for the user space implementation to not > > assume that ETHTOOL_MSG_CHANNELS_GET is implemented by the driver. > > [Wei] IMO, the issue you encountered is that libbpf will perform an > ETHTOOL_GCHANNELS operation. The issue I encountered is that "ethtool -x" > will also perform an ETHTOOL_GCHANNELS operation. Besides, There are other > apps that do the same operation as this, so I think it's best for fsl-enetc driver > to support querying channels. > Because your patch is more reasonable than mine, I think you should submit > this patch to upstream separately first. The crucial piece you're omitting is that for ethtool -x, the "get channels" operation didn't use to be required, making this new requirement a breaking change. The interface between the Linux kernel and applications doesn't do that, which is why it's preferable to bring it up with the ethtool maintainers and the patch author instead of fixing one driver and leaving who knows how many still unfixed (and also the stable kernels unfixed for enetc as well). To be clear, I won't submit the "get_channels" enetc patch for the RFH indirection table to keep working. I might resubmit it for other reasons, like when I return to the AF_XDP zerocopy work.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c index e993ed04ab57..5fa1000b9b83 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c @@ -740,6 +740,17 @@ static int enetc_set_rxfh(struct net_device *ndev, const u32 *indir, return err; } +static void enetc_get_channels(struct net_device *ndev, + struct ethtool_channels *ch) +{ + struct enetc_ndev_priv *priv = netdev_priv(ndev); + + ch->max_rx = priv->num_rx_rings; + ch->max_tx = priv->num_tx_rings; + ch->rx_count = priv->num_rx_rings; + ch->tx_count = priv->num_tx_rings; +} + static void enetc_get_ringparam(struct net_device *ndev, struct ethtool_ringparam *ring, struct kernel_ethtool_ringparam *kernel_ring, @@ -1196,6 +1207,7 @@ static const struct ethtool_ops enetc_pf_ethtool_ops = { .get_rxfh_indir_size = enetc_get_rxfh_indir_size, .get_rxfh = enetc_get_rxfh, .set_rxfh = enetc_set_rxfh, + .get_channels = enetc_get_channels, .get_ringparam = enetc_get_ringparam, .get_coalesce = enetc_get_coalesce, .set_coalesce = enetc_set_coalesce, @@ -1226,6 +1238,7 @@ static const struct ethtool_ops enetc_vf_ethtool_ops = { .get_rxfh_indir_size = enetc_get_rxfh_indir_size, .get_rxfh = enetc_get_rxfh, .set_rxfh = enetc_set_rxfh, + .get_channels = enetc_get_channels, .get_ringparam = enetc_get_ringparam, .get_coalesce = enetc_get_coalesce, .set_coalesce = enetc_set_coalesce,
Since ETHTOOL_MSG_RSS_GET netlink message [1] has been applied to ethtool tree, there is a netlink error when using "ethtool -x eno0" command to get RSS information from fsl-enetc driver, and the user cannot get the information, the error logs are as follows: root@ls1028ardb:~# ./ethtool -x eno0 netlink error: Operation not supported The rationale is that ethtool will issue a ETHTOOL_MSG_CHANNELS_GET netlink message to get the number of Rx ring. However, the fsl-enetc driver doesn't support ethtool::get_channels, so it directly returns -EOPNOTSUPP error. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/ethtool.git/commit/?id=ffab99c1f3820e21d65686e030dcf2c4fd0a8bd0 Signed-off-by: Wei Fang <wei.fang@nxp.com> --- .../net/ethernet/freescale/enetc/enetc_ethtool.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)