[PATCH net-next] net: enetc: add ethtool::get_channels support

[PATCH net-next] net: enetc: add ethtool::get_channels support

[Wei Fang]

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(+)

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

RE: [EXT] [PATCH net-next] net: enetc: add ethtool::get_channels support

[Suman Ghosh]

>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
>

Re: [PATCH net-next] net: enetc: add ethtool::get_channels support

[Vladimir Oltean]

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.

RE: [EXT] [PATCH net-next] net: enetc: add ethtool::get_channels support

[Wei Fang]

> -----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.

RE: [PATCH net-next] net: enetc: add ethtool::get_channels support

[Wei Fang]

> -----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.

Re: [PATCH net-next] net: enetc: add ethtool::get_channels support

[Vladimir Oltean]

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.