Hi Manivannan
On Mon, 24 Feb 2025 at 17:24, Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> On Mon, Feb 24, 2025 at 03:38:29PM +0530, Anand Moon wrote:
> > Hi Manivannan
> >
> > On Mon, 24 Feb 2025 at 13:31, Manivannan Sadhasivam
> > <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Feb 20, 2025 at 03:53:31PM +0530, Anand Moon wrote:
> > >
> > > [...]
> > >
> > > > Following the change fix this warning in a kernel memory leak.
> > > > Would you happen to have any comments on these changes?
> > > >
> > > > diff --git a/drivers/pci/controller/plda/pcie-plda-host.c
> > > > b/drivers/pci/controller/plda/pcie-plda-host.c
> > > > index 4153214ca410..5a72a5a33074 100644
> > > > --- a/drivers/pci/controller/plda/pcie-plda-host.c
> > > > +++ b/drivers/pci/controller/plda/pcie-plda-host.c
> > > > @@ -280,11 +280,6 @@ static u32 plda_get_events(struct plda_pcie_rp *port)
> > > > return events;
> > > > }
> > > >
> > > > -static irqreturn_t plda_event_handler(int irq, void *dev_id)
> > > > -{
> > > > - return IRQ_HANDLED;
> > > > -}
> > > > -
> > > > static void plda_handle_event(struct irq_desc *desc)
> > > > {
> > > > struct plda_pcie_rp *port = irq_desc_get_handler_data(desc);
> > > > @@ -454,13 +449,10 @@ int plda_init_interrupts(struct platform_device *pdev,
> > > >
> > > > if (event->request_event_irq)
> > > > ret = event->request_event_irq(port, event_irq, i);
> > > > - else
> > > > - ret = devm_request_irq(dev, event_irq,
> > > > - plda_event_handler,
> > > > - 0, NULL, port);
> > >
> > > This change is not related to the memleak. But I'd like to have it in a separate
> > > patch since this code is absolutely not required, rather pointless.
> > >
> > Yes, remove these changes to fix the memory leak issue I observed.
> >
>
> Sorry, I don't get you. This specific code change of removing 'devm_request_irq'
> is not supposed to fix memleak.
>
> If you are seeing the memleak getting fixed because of it, then something is
> wrong with the irq implementation. You need to figure it out.
Declaring request_event_irq in the host controller facilitates the
creation of a dedicated IRQ event handler.
In its absence, a dummy devm_request_irq was employed, but this
resulted in unhandled IRQs and subsequent memory leaks.
Eliminating the dummy code eliminated the memory leak logs.
>
> > > >
> > > > if (ret) {
> > > > dev_err(dev, "failed to request IRQ %d\n", event_irq);
> > > > + irq_dispose_mapping(event_irq);
> > >
> > > So the issue overall is that the 'devm_request_irq' fails on your platform
> > > because the interrupts are not defined in DT and then the IRQ is not disposed in
> > > the error path?
> > >
> > On microchip PCIe driver utilizes interrupt resources from its
> > "bridge" and "cntrl"
> > components, accessed via ioremap, to handle hardware events.
> > These resources are absent in the StarFive PCIe controller.
> >
> > While the StarFive JH-7110 datasheet [1] indicates support for PME, MSI, and INT
> > messages and specific implementation details for leveraging these interrupts are
> > unavailable.
> >
> > As per StarFive JH-7110 Datasheet PCI support [1]
> > 1 Power Management Event (PME message) ◦
> > 2 MSI (up to 32) and INT message support
> >
> > [1] https://doc-en.rvspace.org/JH7110/PDF/JH7110I_DS.pdf
> >
>
> Fine.
>
> > > In that case, none of the error paths below for_each_set_bit() loop is disposing
> > > the IRQs. Also, calling 'irq_dispose_mapping()' only once is not going to
> > > dispose all mappings that were created before in the loop.
> >
> > I was looking at how the IRQ error path will clean up IRQ in case of failure
> > hence, I added this in case of failure to unmap IRQ events,
> > I will drop this if not required.
>
> Absolutely not. These are fixing the irq leaks. But if it is not related to the
> 'memleak' you observed, then these should be part of a separate patch.
>
Ok will drop this.
> - Mani
>
Thank
-Anand
> --
> மணிவண்ணன் சதாசிவம்
