Re: [PATCH] net: wan: fsl_ucc_hdlc: release resources on remove

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Teardown ordering defect in the new code: ucc_hdlc_remove()
  calls unregister_hdlc_device(priv->ndev) as its first action.
- [Low] Coding style in the new code: the added
  unregister_hdlc_device(priv->ndev) is inserted directly after the…

Pre-existing issues:
- [Medium] Probe error-unwind asymmetry: the labels reached after
  uhdlc_init() has already succeeded ('free_dev:' at…
- [Low] ucc_hdlc_probe() creates a virq mapping with
  'ut_info->uf_info.irq = irq_of_parse_and_map(np, 0);'…

--

net: wan: fsl_ucc_hdlc: release resources on remove

The patch makes ucc_hdlc_remove() unregister the HDLC netdev, free the
netdev after the hardware teardown, and free the separately allocated
struct ucc_tdm before freeing the private data.  A few questions about the
ordering and about the surrounding teardown paths below.

> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 809f21fb93f56..ffc8b989e33a7 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -1254,6 +1254,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
>  static void ucc_hdlc_remove(struct platform_device *pdev)
>  {
>  	struct ucc_hdlc_private *priv = dev_get_drvdata(&pdev->dev);
> +	unregister_hdlc_device(priv->ndev);

[Severity: High]
Can uhdlc_close() be skipped here when the interface is administratively
up at unbind time?

unregister_hdlc_device() in drivers/net/wan/hdlc.c detaches the protocol
before unregistering the netdevice:

void unregister_hdlc_device(struct net_device *dev)
{
	rtnl_lock();
	detach_hdlc_protocol(dev);
	unregister_netdevice(dev);
	rtnl_unlock();
}

detach_hdlc_protocol() ends with hdlc_setup_dev(), which assigns
dev->flags directly:

	dev->flags		 = IFF_POINTOPOINT | IFF_NOARP;

That clears IFF_UP.  netif_close_many() in net/core/dev.c then drops the
device from the close list:

	list_for_each_entry_safe(dev, tmp, head, close_list)
		if (!(dev->flags & IFF_UP))
			list_del_init(&dev->close_list);

so ndo_stop (uhdlc_close()) is not called during unregistration.  Since
uhdlc_close() is the only place that does:

	napi_disable(&priv->napi);
	...
	free_irq(priv->ut_info->uf_info.irq, priv);

does that leave ucc_hdlc_irq_handler() still installed on the virq, with
dev_id == priv, while the code below frees the DMA rings, the netdev and
priv?  The handler starts with:

	struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)dev_id;
	struct net_device *dev = priv->ndev;

Would it be safer to close the device through the networking core (while
IFF_UP and hdlc->proto are still intact) before calling
unregister_hdlc_device()?  A related side effect is that the
request_irq()/free_irq() pair stays unbalanced, so a re-bind would get
-EBUSY from request_irq() in uhdlc_open() on the same non-shared virq.

Reproducer would be: attach a protocol via SIOCWANDEV, ip link set hdlc0
up, then unbind the platform device via sysfs.

[Severity: Low]
This isn't a bug, but the new call sits directly after the declaration of
priv with no blank line between the declaration and the first statement,
which checkpatch.pl flags as "Missing a blank line after declarations".

>  
>  	uhdlc_memclean(priv);
>  
> @@ -1266,6 +1267,8 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
>  		iounmap(priv->utdm->siram);
>  		priv->utdm->siram = NULL;
>  	}
> +	free_netdev(priv->ndev);
> +	kfree(priv->utdm);
>  	kfree(priv);

[Severity: Medium]
This isn't a problem introduced by this patch, but while the two teardown
sites are being brought into agreement: the probe error labels reached
after uhdlc_init() has already succeeded never call uhdlc_memclean():

free_dev:
	free_netdev(dev);
undo_uhdlc_init:
	if (utdm)
		iounmap(utdm->siram);
unmap_si_regs:
	if (utdm)
		iounmap(utdm->si_regs);
free_utdm:
	if (uhdlc_priv->tsa)
		kfree(utdm);
free_uhdlc_priv:
	kfree(uhdlc_priv);

If alloc_hdlcdev() returns NULL or register_hdlc_device() fails, does the
kfree(uhdlc_priv) drop the only handles to what uhdlc_init() allocated,
i.e. the two coherent BD rings, the coherent RX/TX data buffer, the QE
MURAM parameter area plus the riptr/tiptr MURAM allocations, the
ucc_fast_private from ucc_fast_init(), and the rx_skbuff/tx_skbuff
arrays?

The commit message says:

    The probe failure paths release these resources, but the remove path
    does not.

Is that accurate only for the netdev and the ucc_tdm struct?  The label
name undo_uhdlc_init suggests it undoes uhdlc_init(), while the code
under it does not call uhdlc_memclean().

[Severity: Low]
This is a pre-existing asymmetry rather than something added here, but
ucc_hdlc_probe() creates the virq mapping with:

	ut_info->uf_info.irq = irq_of_parse_and_map(np, 0);

and neither the probe error labels nor ucc_hdlc_remove() call
irq_dispose_mapping().  Should the mapping acquired at probe time be
disposed in the remove path being completed here, given that
request_irq()/free_irq() are balanced by uhdlc_open()/uhdlc_close() but
the mapping itself is not?

>  
>  	dev_info(&pdev->dev, "UCC based hdlc module removed\n");

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916065136.2926089-1-lgs201920130244%40gmail.com