Re: [PATCH net-next v15 2/5] net: rnpgbe: Add basic TX packet transmission support

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rnpgbe_probe() now calls dma_set_mask_and_coherent(&pdev->dev,
  DMA_BIT_MASK(56)) and ignores the return value.
- [Low] rnpgbe_add_adapter() calls netif_carrier_off(netdev) just before
  register_netdev().

--

net: rnpgbe: Add basic TX packet transmission support

This commit adds the TX path for the rnpgbe driver: TX descriptor and
buffer management, rnpgbe_xmit_frame_ring(), ring setup and teardown, and
TX completion handling in rnpgbe_clean_tx_irq(). It also keeps the
carrier off at registration, sets 56-bit streaming and coherent DMA masks,
and enables NETIF_F_HIGHDMA when the mask allows it.

> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 6022a1a2ddf1e..265a3fcb07beb 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c

[ ... ]

> @@ -206,6 +254,14 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
>  		goto err_clear_interrupt;
>  	}
>  
> +	netdev->features |= NETIF_F_SG;
> +	netdev->hw_features |= NETIF_F_SG;
> +	if (dma_get_mask(&pdev->dev) > DMA_BIT_MASK(32)) {
> +		netdev->features |= NETIF_F_HIGHDMA;
> +		netdev->hw_features |= NETIF_F_HIGHDMA;
> +	}
> +
> +	netif_carrier_off(netdev);
> +	err = register_netdev(netdev);

[Severity: Low]
In this commit, does anything under drivers/net/ethernet/mucse/ ever call
netif_carrier_on()?

rnpgbe_add_adapter() sets NOCARRIER before register_netdev(). With the
carrier off, dev_activate() returns early:

net/sched/sch_generic.c:dev_activate() {
    ...
	if (!netif_carrier_ok(dev))
		/* Delay activation until next carrier-on event */
		return;
    ...
}

So the normal stack never reaches the new path
rnpgbe_xmit_frame()->rnpgbe_xmit_frame_ring()->rnpgbe_tx_map(), and
rnpgbe_clean_tx_irq() never has completions to handle at this revision.

The commit message says:

    Keep carrier state off during registration so transmit remains gated
    until a valid link state is established.

This patch does not add anything that establishes link state. The later
commit "net: rnpgbe: Add link status handling support" fills the gap:
rnpgbe_link_is_up() calls netif_carrier_on(netdev) from the service task.
Before this patch every frame was dropped in ndo_start_xmit anyway, so
nothing that used to work stops working.

Could the commit message say that the carrier is only turned on by a
later patch in the series?

>  	if (err)
>  		goto err_remove_mbx;

[ ... ]

> @@ -248,12 +304,7 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (err)
>  		return err;
>  
> -	err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(56));
> -	if (err) {
> -		dev_err(&pdev->dev,
> -			"No usable DMA configuration, aborting %d\n", err);
> -		goto err_disable_dev;
> -	}
> +	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(56));

[Severity: Medium]
Should the return value of dma_set_mask_and_coherent() still be checked
here?

Before this patch, rnpgbe_probe() logged "No usable DMA configuration,
aborting" and jumped to err_disable_dev when setting the mask failed. With
this change, a failure is silently ignored:

include/linux/dma-mapping.h:dma_set_mask_and_coherent() {
	int rc = dma_set_mask(dev, mask);
	if (rc == 0)
		dma_set_coherent_mask(dev, mask);
	return rc;
}

If dma_set_mask() fails, neither mask is set. Both stay at the PCI default
and probe goes on to register the netdev. The only visible effect is that
the dma_get_mask() check in rnpgbe_add_adapter() quietly leaves out
NETIF_F_HIGHDMA, with no diagnostic for the user.

The commit message describes this hunk as "Configure streaming and
coherent DMA masks" and does not mention that the error check was
removed. The call is still unchecked at the end of the series.

>  
>  	err = pci_request_mem_regions(pdev, rnpgbe_driver_name);
>  	if (err) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/B438B0A83ACCA2DE%2B20261004065604.224873-1-dong100%40mucse.com