On Mon, Feb 24, 2025 at 09:37:14PM +0530, Anand Moon wrote:
> Hi Manivannan,
>
> On Mon, 24 Feb 2025 at 20:12, Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
> >
> > 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.
> >
> Yes, but my solution was to work around
>
Which is what I'm trying to avoid....
> > > Eliminating the dummy code eliminated the memory leak logs.
> >
> From the code, we are creating a mapping of the IRQ event
>
> for_each_set_bit(i, &port->events_bitmap, port->num_events) {
> event_irq = irq_create_mapping(port->event_domain, i);
> if (!event_irq) {
> dev_err(dev, "failed to map hwirq %d\n", i);
> return -ENXIO;
> }
>
> 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);
>
> if (ret) {
> dev_err(dev, "failed to request IRQ %d\n", event_irq);
> return ret;
> }
> }
>
> in the microchip PCIe host we are requesting those IRQ events mapping.
>
> static int mc_request_event_irq(struct plda_pcie_rp *plda, int event_irq,
> int event)
> {
> return devm_request_irq(plda->dev, event_irq, mc_event_handler,
> 0, event_cause[event].sym, plda);
> }
>
> static const struct plda_event_ops mc_event_ops = {
> .get_events = mc_get_events,
> };
>
> 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,
> };
>
> > 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.
>
> The StarFive PCIe host lacks the necessary hardware event mapping.
> Consequently, the system attempts to handle dummy events, resulting
> in observed log messages.
>
> The issue is likely due to devm_request_irq being called with a NULL devname,
> preventing proper IRQ mapping.
>
Then please fix the offending devm_request_irq() call. Do not workaround the
issue.
- Mani
--
மணிவண்ணன் சதாசிவம்
