Re: [PATCH v1] PCI: starfive: Fix kmemleak in StarFive PCIe driver's IRQ handling

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Feb 11, 2025 at 01:09:04AM +0530, Anand Moon wrote:
> Hi Manivannan
> 
> On Mon, 10 Feb 2025 at 23:14, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Sat, Feb 08, 2025 at 07:31:08PM +0530, Anand Moon wrote:
> > > kmemleak reported following debug log
> > >
> > > $ sudo cat /sys/kernel/debug/kmemleak
> > > unreferenced object 0xffffffd6c47c2600 (size 128):
> > >   comm "kworker/u16:2", pid 38, jiffies 4294942263
> > >   hex dump (first 32 bytes):
> > >     cc 7c 5a 8d ff ff ff ff 40 b0 47 c8 d6 ff ff ff  .|Z.....@.G.....
> > >     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> > >   backtrace (crc 4f07ff07):
> > >     __create_object+0x2a/0xfc
> > >     kmemleak_alloc+0x38/0x98
> > >     __kmalloc_cache_noprof+0x296/0x444
> > >     request_threaded_irq+0x168/0x284
> > >     devm_request_threaded_irq+0xa8/0x13c
> > >     plda_init_interrupts+0x46e/0x858
> > >     plda_pcie_host_init+0x356/0x468
> > >     starfive_pcie_probe+0x2f6/0x398
> > >     platform_probe+0x106/0x150
> > >     really_probe+0x30e/0x746
> > >     __driver_probe_device+0x11c/0x2c2
> > >     driver_probe_device+0x5e/0x316
> > >     __device_attach_driver+0x296/0x3a4
> > >     bus_for_each_drv+0x1d0/0x260
> > >     __device_attach+0x1fa/0x2d6
> > >     device_initial_probe+0x14/0x28
> > > unreferenced object 0xffffffd6c47c2900 (size 128):
> > >   comm "kworker/u16:2", pid 38, jiffies 4294942281
> > >
> > > This patch addresses a kmemleak reported during StarFive PCIe driver
> > > initialization. The leak was observed with kmemleak reporting
> > > unreferenced objects related to IRQ handling. The backtrace pointed
> > > to the `request_threaded_irq` and related functions within the
> > > `plda_init_interrupts` path, indicating that some allocated memory
> > > related to IRQs was not being properly freed.
> > >
> > > The issue was that while the driver requested IRQs, it wasn't
> > > correctly handling the lifecycle of the associated resources.
> >
> > What resources?
> >
> The Microchip PCIe host driver [1] handles  PCI, SEC, DEBUG, and LOCAL
> hardware events
> through a dedicated framework [2]. This framework allows the core driver [3]
> to monitor and wait for these specific events.
> 

Microchip driver also has its own 'event_ops' and 'event_irq_chip', so defining
'request_event_irq()' callback makes sense to me.

> [1]: https://github.com/torvalds/linux/blob/master/drivers/pci/controller/plda/pcie-microchip-host.c#L90-L292
> [2]: https://github.com/torvalds/linux/blob/master/drivers/pci/controller/plda/pcie-microchip-host.c#L374-L499
> [3]: https://github.com/torvalds/linux/blob/master/drivers/pci/controller/plda/pcie-plda-host.c#L448-L466
> 
> StarFive PCIe driver currently lacks the necessary `request_event_irq`
> implementation
> to integrate with this event-handling mechanism, which prevents the core driver
> from properly responding to these events on StarFive platforms.
> 
> static const struct plda_event mc_event = {
> .  request_event_irq = mc_request_event_irq,
>   .intx_event        = EVENT_LOCAL_PM_MSI_INT_INTX,
>   .msi_event         = EVENT_LOCAL_PM_MSI_INT_MSI,
> };
> 
> This patch implements dummy `request_event_irq` for the StarFive PCIe driver,
> Since the core [3] driver is monitoring for these events
> 

This still doesn't make sense to me. Under what condition you observed the
kmemleak? Since it points to devm_request_irq(), I can understand that the
memory allocated for the IRQ is not freed. But when does it happen?

> > > This patch introduces an event IRQ handler and the necessary
> > > infrastructure to manage these IRQs, preventing the memory leak.
> > >
> >
> > These handles appear pointless to me. What purpose are they serving?
> >
> Yes, you are correct, the core event monitoring framework [3] triggered a
> kernel memory leak. This patch adds a dummy IRQ callback as a
> placeholder for proper event handling, which can be implemented in a
> future patch.
> 

The dummy request_event_irq() callback is not supposed to be needed in the first
place. So clearly, this patch is not fixing the actual memory leak but trying to
cover it up.

- Mani

-- 
மணிவண்ணன் சதாசிவம்