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
