Re: [PATCH net v5 3/4] netdevsim: support devmem RX and TX bindings

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Not a bug (baseline: false positive).

--

netdevsim: support devmem RX and TX bindings

This patch sets a 64-bit DMA mask on the netdevsim bus device and turns on
NETMEM_TX_DMA on the netdevsim net_device. It also sets up the page pools
so they can be backed by unreadable netmem, which lets the devmem bind-rx
and bind-tx netlink operations be tested against netdevsim. pp_hold is
refused on memory-provider backed pools, and unreadable or net_iov skbs
are dropped in nsim_start_xmit().

> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index eebc02ccc4a92..0372a9bd63bbb 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -133,6 +133,10 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	int dr;
>  
>  	rcu_read_lock();
> +	if (!skb_frags_readable(skb) ||
> +	    (skb_shinfo(skb)->nr_frags &&
> +	     skb_frag_is_net_iov(&skb_shinfo(skb)->frags[0])))
> +		goto out_drop_any;
>  	if (!nsim_ipsec_tx(ns, skb))
>  		goto out_drop_any;
>  

[ ... ]

> @@ -1059,6 +1078,8 @@ static int nsim_init_netdevsim(struct netdevsim *ns)
>  	ns->netdev->netdev_ops = &nsim_netdev_ops;
>  	ns->netdev->stat_ops = &nsim_stat_ops;
>  	ns->netdev->queue_mgmt_ops = &nsim_queue_mgmt_ops;
> +	if (ns->nsim_bus_dev->dev.dma_mask)
> +		ns->netdev->netmem_tx = NETMEM_TX_DMA;

[Severity: Low]
This isn't a bug, but the docs and the driver don't quite match here.
Documentation/networking/netmem.rst describes the mode as:

    NETMEM_TX_DMA: for physical devices that perform DMA.

nsim_init_netdevsim() sets NETMEM_TX_DMA, but nsim_start_xmit() above
drops every unreadable or net_iov skb.

As I understand it, NETMEM_TX_NO_DMA would not work for this test. That
is because netdev_find_netmem_tx_dev() only resolves a NO_DMA device by
following leased RX queues to a physical NETMEM_TX_DMA device:

net/core/netdev-genl.c:netdev_find_netmem_tx_dev() {
    ...
	if (dev->netmem_tx == NETMEM_TX_DMA)
		return dev;

	if (dev->netmem_tx != NETMEM_TX_NO_DMA)
		return NULL;
    ...
}

Traffic also behaves the same as before. Under NETMEM_TX_NONE,
validate_xmit_unreadable_skb() already freed these skbs. Now the driver
drops them and counts them through dev_dstats_tx_dropped().

Would it help to add a short comment next to this assignment? It could
say that netdevsim advertises NETMEM_TX_DMA only so the bind-tx control
path works, and that devmem TX data is dropped in nsim_start_xmit().

>  	netdev_lockdep_set_classes(ns->netdev);
>  
>  	err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030083539.777877-1-almasrymina%40google.com