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 <[email protected]>
---
.../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;
+}
+
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
>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 <[email protected]>
>---
> .../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 <[email protected]>
> ---
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/[email protected]/
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 <[email protected]>
> Sent: 2023??11??22?? 18:50
> To: Wei Fang <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; Claudiu
> Manoil <[email protected]>; Vladimir Oltean
> <[email protected]>; [email protected]
> Cc: [email protected]
> 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 <[email protected]>
> >---
> > .../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 <[email protected]>
> Sent: 2023??11??22?? 19:01
> To: Wei Fang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Claudiu Manoil <[email protected]>;
> [email protected]; [email protected]
> 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 <[email protected]>
> > ---
>
> 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
> [email protected]/
>
[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.