Re: [PATCH v4 2/2] can: loongson_canfd: Add RXDMA support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The CPU reads ahead of the DMA write pointer, leading to frame corruption and ring buffer desynchronization.
- [High] A DMA status error causes an infinite NAPI polling loop, leading to a soft lockup.
- [High] Failure of `loongson_canfd_rxdma_init` leads to a double free of the DMA buffer and completely breaks the RX path.
- [Medium] Kconfig dependency strictly requires the Loongson DMA controller, breaking the driver for polling mode users.
- [Medium] The network device is leaked if `dma_request_chan()` returns `-EPROBE_DEFER`.
- [Low] Build failure due to undeclared variable 'cf'.
- [Low] Improper internal access of `dma_chan->cookie` field violates the DMA engine API.

Pre-existing issues:
- [High] An SKB allocation failure in `loongson_canfd_rx` leaves payload data in the buffer, permanently desynchronizing the RX stream.
--

--- Patch [2]: [PATCH v4 2/2] can: loongson_canfd: Add RXDMA support ---
commit 1ade4f6818d2a045c4fbf2074c44add29566cfc4
Author: Binbin Zhou <zhoubinbin@loongson.cn>
Subject: can: loongson_canfd: Add RXDMA support

This commit adds optional DMA engine support for the receive path in the
Loongson CAN-FD driver. It configures a cyclic DMA transfer to continuously
fill a ring buffer, falling back to polling if DMA is unavailable.

> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -191,7 +191,7 @@ config CAN_KVASER_PCIEFD
>  
>  config CAN_LOONGSON_CANFD
>  	tristate "Loongson CAN-FD controller"
> -	depends on HAS_IOMEM
> +	depends on HAS_IOMEM && (LOONGSON2_APB_CMC_DMA || COMPILE_TEST)

[Severity: Medium]
The commit message notes that DMA is optional and the driver will fall back
to poll mode. Does modifying the Kconfig to strictly depend on
LOONGSON2_APB_CMC_DMA prevent users from building the driver on platforms
where they only want polling mode?

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c
[ ... ]
> +static bool loongson_canfd_get_rxbuf_empty_in_dma(struct loongson_canfd_priv *priv)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +
> +	status = dmaengine_tx_status(priv->rx_ch, priv->rx_ch->cookie, &state);

[Severity: Low]
Is it safe to directly access the internal cookie field of the dma_chan
structure? The DMA engine API typically requires clients to track the cookie
returned by dmaengine_submit() rather than dereferencing the internal
framework structures.

> +
> +	return priv->last_res == (state.residue / DMA_SLAVE_BUSWIDTH_4_BYTES) &&
> +	       (status == DMA_IN_PROGRESS);
> +}

[Severity: High]
Could this empty check cause the CPU to read ahead of the DMA write pointer?

In loongson_canfd_rx_napi(), the buffer is considered non-empty as soon as
the DMA engine transfers a single word (since last_res will no longer equal
the residue calculation). 

When NAPI calls loongson_canfd_rx(), it unconditionally reads multiple words
for the header and payload. Since the DMA engine might have only written one
word so far, does this consume stale data and corrupt the stream?

[Severity: High]
What happens if the DMA engine status is not DMA_IN_PROGRESS due to a
hardware error or a halted transfer? 

This function will return false, indicating the buffer is not empty. The NAPI
loop in loongson_canfd_rx_napi() will continuously poll loongson_canfd_rx(). 

Because the empty check evaluates to false, it skips napi_complete_done()
and the networking stack instantly reschedules NAPI. Does this lead to an
infinite loop and a CPU soft lockup?

[ ... ]
> +static int loongson_canfd_rxdma_init(struct net_device *ndev)
> +{
[ ... ]
> +	/* Push current dma transaction in the pending queue */
> +	ret = dma_submit_error(dmaengine_submit(desc));
> +	if (ret) {
> +		dmaengine_terminate_sync(priv->rx_ch);
> +		goto err_config;
> +	}
> +
> +	/* Issue pending DMA requests */
> +	dma_async_issue_pending(priv->rx_ch);
> +
> +	return 0;
> +
> +err_config:
> +	loongson_canfd_rxdma_free(priv, dev);
> +	return ret;
> +}

[Severity: High]
If dmaengine_slave_config or dmaengine_prep_dma_cyclic fails, this calls
loongson_canfd_rxdma_free() but leaves priv->rx_ch pointing to the freed
channel. 

Since the probe function ignores the error to fall back to poll mode, will
loongson_canfd_chip_start() mistakenly enable the DMADI interrupt instead
of RBNEI because priv->rx_ch is not NULL? 

Furthermore, when the driver is removed, loongson_canfd_rxdma_remove() checks
priv->rx_ch and calls loongson_canfd_rxdma_free() again. Does this result in
a fatal double-free?

[ ... ]
> @@ -1028,7 +1171,7 @@ static int loongson_canfd_rx(struct net_device *ndev)
[ ... ]
>  	if (is_can_fd) {
>  		skb = alloc_canfd_skb(ndev, &cfd);
>  	} else {
>  		skb = alloc_can_skb(ndev, &ccf);
>  		cfd = (struct canfd_frame *)ccf;
>  	}
>  
>  	if (unlikely(!skb))
>  		return 0;

[Severity: High]
This is a pre-existing issue, but if alloc_canfd_skb() or alloc_can_skb()
fails in loongson_canfd_rx(), the function returns 0 immediately. 

It has already consumed the meta0 and meta1 headers but skips consuming the
payload words. Since these unread words remain in the hardware FIFO or DMA
buffer, will the next RX interrupt misinterpret them as the headers of the
next frame, permanently desynchronizing the stream?

[ ... ]
>  	/* Data Length Code */
>  	dlc = FIELD_GET(REG_FRAME_META1_DLC, meta1);
>  	if (is_can_fd)
>  		cfd->len = can_fd_dlc2len(dlc);
>  	else
>  		can_frame_set_cc_len(ccf, dlc, priv->can.ctrlmode);
>  
>  	/* Data */
>  	for (unsigned int i = 0; i < cfd->len; i += LOONGSON_CANFD_DW_BYTE)
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, (u32 *)(cfd->data + i));
> +		*(u32 *)(cf->data + i) = priv->get_rx_data(priv);

[Severity: Low]
Does this cause a build failure? The variable cf is not declared in
loongson_canfd_rx() (the available pointers are cfd and ccf). Writing to
cf->data seems to be a typo that prevents the driver from compiling.

[ ... ]
> @@ -1635,6 +1779,24 @@ static int loongson_canfd_probe(struct platform_device *pdev)
[ ... ]
>  	ndev->netdev_ops = &loongson_canfd_netdev_ops;
>  	ndev->ethtool_ops = &loongson_canfd_ethtool_ops;
>  	SET_NETDEV_DEV(ndev, dev);
>  
> +	priv->get_rx_data = loongson_canfd_get_rxdata_in_poll;
> +	priv->get_rxbuf_empty = loongson_canfd_get_rxbuf_empty_in_poll;
> +
> +	priv->rx_ch = dma_request_chan(dev, "rx");
> +	if (PTR_ERR(priv->rx_ch) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

[Severity: Medium]
If dma_request_chan() returns -EPROBE_DEFER, this immediately returns the
error code without jumping to the error handling path. 

Since alloc_candev() was successfully called earlier in the probe function,
does returning directly here permanently leak the net_device and priv
structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783391759.git.zhoubinbin@loongson.cn?part=2