On Mon, Feb 24, 2025 at 07:33:37PM +0530, Anand Moon wrote:
> 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.
What do you mean by 'unhandled IRQs'? There is a dummy IRQ handler invoked to
handle these IRQs. Even your starfive_event_handler() that you proposed was
doing the same thing.
> Eliminating the dummy code eliminated the memory leak logs.
Sorry, this is not a valid justification. But as I said before, the change
itself (removing the dummy irq handler and related code) looks good to me as I
see no need for that. But I cannot accept it as a fix for the memleak.
- Mani
--
மணிவண்ணன் சதாசிவம்
