Re: [PATCH net-next v3 5/7] net: devmem: add ring parameter filtering

[Brett Creeley <bcreeley@xxxxxxx>]

On 10/3/2024 9:06 AM, Taehee Yoo wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> If driver doesn't support ring parameter or tcp-data-split configuration
> is not sufficient, the devmem should not be set up.
> Before setup the devmem, tcp-data-split should be ON and
> tcp-data-split-thresh value should be 0.
>
> Signed-off-by: Taehee Yoo <ap420073@xxxxxxxxx>
> ---
>
> v3:
>   - Patch added.
>
>   net/core/devmem.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 11b91c12ee11..a9e9b15028e0 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -8,6 +8,8 @@
>    */
>
>   #include <linux/dma-buf.h>
> +#include <linux/ethtool.h>
> +#include <linux/ethtool_netlink.h>
>   #include <linux/genalloc.h>
>   #include <linux/mm.h>
>   #include <linux/netdevice.h>
> @@ -131,6 +133,8 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>                                      struct net_devmem_dmabuf_binding *binding,
>                                      struct netlink_ext_ack *extack)
>   {
> +       struct kernel_ethtool_ringparam kernel_ringparam = {};
> +       struct ethtool_ringparam ringparam = {};
>          struct netdev_rx_queue *rxq;
>          u32 xa_idx;
>          int err;
> @@ -146,6 +150,20 @@ int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
>                  return -EEXIST;
>          }
>
> +       if (!dev->ethtool_ops->get_ringparam) {
> +               NL_SET_ERR_MSG(extack, "can't get ringparam");
> +               return -EINVAL;
> +       }

Is EINVAL the correct return value here? I think it makes more sense as 
EOPNOTSUPP.

> +
> +       dev->ethtool_ops->get_ringparam(dev, &ringparam,
> +                                       &kernel_ringparam, extack);
> +       if (kernel_ringparam.tcp_data_split != ETHTOOL_TCP_DATA_SPLIT_ENABLED ||
> +           kernel_ringparam.tcp_data_split_thresh) {
> +               NL_SET_ERR_MSG(extack,
> +                              "tcp-header-data-split is disabled or threshold is not zero");
> +               return -EINVAL;
> +       }
> +
Maybe just my personal opinion, but IMHO these checks should be separate 
so the error message can be more concise/clear.

Also, a small nit, but I think both of these checks should be before 
getting the rxq via __netif_get_rx_queue().


Thanks,

Brett
>   #ifdef CONFIG_XDP_SOCKETS
>          if (rxq->pool) {
>                  NL_SET_ERR_MSG(extack, "designated queue already in use by AF_XDP");
> --
> 2.34.1