Re: [PATCH v1 2/5] dmaengine: ioatdma: use common channel sysfs attribute creation

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch leaves behind an unused `struct kobject kobj` and `IOAT_KOBJ_INIT_FAIL` state flag in `struct ioatdma_chan`.
- [Low] Unconditional reference to `dma_chan_sysfs_ops` and `dma_chan_kobject_add` causes build breakage if `CONFIG_DEBUG_FS` is disabled.

Pre-existing issues:
- [High] Missing `dma_async_device_unregister()` on error paths in `ioat3_dma_probe()` causes Use-After-Free and resource leaks.
- [High] Missing `release` callback in `kobj_type` violates kobject lifetime rules and causes Use-After-Free.
--

--- Patch [2]: [PATCH v1 2/5] dmaengine: ioatdma: use common channel sysfs attribute creation ---
commit 88321b4bdaabe072b5dfb4d9c84f04832a5bcc78
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: ioatdma: use common channel sysfs attribute creation

This patch replaces the manual creation of sysfs attributes for each channel
by utilizing the common dma_chan_kobject_add() facility.

> --- a/drivers/dma/ioat/dma.h
> +++ b/drivers/dma/ioat/dma.h
[ ... ]
> @@ -393,8 +393,6 @@ void ioat_issue_pending(struct dma_chan *chan);
>  /* IOAT Init functions */
>  bool is_bwd_ioat(struct pci_dev *pdev);
>  struct dca_provider *ioat_dca_init(struct pci_dev *pdev, void __iomem *iobase);
> -void ioat_kobject_add(struct ioatdma_device *ioat_dma, const struct kobj_type *type);
> -void ioat_kobject_del(struct ioatdma_device *ioat_dma);

[Severity: Low]
Is it possible to remove the kobj and IOAT_KOBJ_INIT_FAIL fields from struct
ioatdma_chan as well? Since dma_chan_kobject_add() uses the kobject embedded
in struct dma_chan, these fields appear to be unused now.

> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
[ ... ]
> @@ -1174,7 +1172,7 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
>  	if (err)
>  		goto err_disable_interrupts;
>  
> -	ioat_kobject_add(ioat_dma, &ioat_ktype);
> +	dma_chan_kobject_add(&ioat_dma->dma_dev, &ioat_ktype, "quickdata");

[Severity: Low]
Will this cause a linker error if the kernel is compiled with CONFIG_DEBUG_FS
disabled? It looks like dma_chan_kobject_add() might depend on debugfs.

[Severity: High]
This is a pre-existing issue, but does this error path properly clean up
device registration?

If an error occurs during PCI capability configuration later in
ioat3_dma_probe() after dma_async_device_register() succeeds:

    err = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &val16);
    if (err) {
        err = pcibios_err_to_errno(err);
        goto err_disable_interrupts;
    }

It jumps to err_disable_interrupts and returns an error without calling
dma_async_device_unregister(). Also, with this patch, do the newly added
kobjects need to be cleaned up on this path?

> --- a/drivers/dma/ioat/sysfs.c
> +++ b/drivers/dma/ioat/sysfs.c
[ ... ]
>  const struct kobj_type ioat_ktype = {
> -	.sysfs_ops = &ioat_sysfs_ops,
> +	.sysfs_ops = &dma_chan_sysfs_ops,

[Severity: Low]
Will referencing dma_chan_sysfs_ops here also cause a build failure when
CONFIG_DEBUG_FS is disabled?

[Severity: High]
This is a pre-existing issue, but does ioat_ktype need a .release callback?

Without it, if sysfs holds a reference to the kobject during driver unload,
could it access freed memory when the kobject core later tries to release it?

>  	.default_groups = ioat_groups,
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707162045.23910-1-logang@deltatee.com?part=2