| Message ID | 20250208140110.2389-1-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | [v1] PCI: starfive: Fix kmemleak in StarFive PCIe driver's IRQ handling | expand |
… > This patch addresses a kmemleak reported during StarFive PCIe driver … > This patch introduces an event IRQ handler and the necessary > infrastructure to manage these IRQs, preventing the memory leak. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13#n94 Regards, Markus
Hi Markus, On Sat, 8 Feb 2025 at 22:03, Markus Elfring <Markus.Elfring@web.de> wrote: > > … > > This patch addresses a kmemleak reported during StarFive PCIe driver > … > > This patch introduces an event IRQ handler and the necessary > > infrastructure to manage these IRQs, preventing the memory leak. > > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.13#n94 > Thanks I will update the commit message, one I get more feedback on this code changes. > Regards, > Markus Thanks -Anand
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? > 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? - Mani > Fixes: 647690479660 ("PCI: microchip: Add request_event_irq() callback function") > Cc: Minda Chen <minda.chen@starfivetech.com> > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > drivers/pci/controller/plda/pcie-starfive.c | 39 +++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c > index e73c1b7bc8ef..a57177a8be8a 100644 > --- a/drivers/pci/controller/plda/pcie-starfive.c > +++ b/drivers/pci/controller/plda/pcie-starfive.c > @@ -381,7 +381,46 @@ static const struct plda_pcie_host_ops sf_host_ops = { > .host_deinit = starfive_pcie_host_deinit, > }; > > +/* IRQ handler for PCIe events */ > +static irqreturn_t starfive_event_handler(int irq, void *dev_id) > +{ > + struct plda_pcie_rp *port = dev_id; > + struct irq_data *data; > + > + /* Retrieve IRQ data */ > + data = irq_domain_get_irq_data(port->event_domain, irq); > + > + if (data) { > + dev_err_ratelimited(port->dev, "Event IRQ %ld triggered\n", > + data->hwirq); > + } else { > + dev_err_ratelimited(port->dev, "Invalid event IRQ %d\n", irq); > + } > + > + return IRQ_HANDLED; > +} > + > +/* Request an IRQ for a specific event */ > +static int starfive_request_event_irq(struct plda_pcie_rp *plda, int event_irq, > + int event) > +{ > + int ret; > + > + /* Request the IRQ and associate it with the handler */ > + ret = devm_request_irq(plda->dev, event_irq, starfive_event_handler, > + IRQF_SHARED, "starfivepcie", plda); > + > + if (ret) { > + dev_err(plda->dev, "Failed to request IRQ %d: %d\n", event_irq, > + ret); > + return ret; > + } > + > + return ret; > +} > + > static const struct plda_event stf_pcie_event = { > + .request_event_irq = starfive_request_event_irq, > .intx_event = EVENT_PM_MSI_INT_INTX, > .msi_event = EVENT_PM_MSI_INT_MSI > }; > > base-commit: bb066fe812d6fb3a9d01c073d9f1e2fd5a63403b > -- > 2.48.1 >
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. [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 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. > - Mani > Thanks -Anand > > Fixes: 647690479660 ("PCI: microchip: Add request_event_irq() callback function") > > Cc: Minda Chen <minda.chen@starfivetech.com> > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > drivers/pci/controller/plda/pcie-starfive.c | 39 +++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c > > index e73c1b7bc8ef..a57177a8be8a 100644 > > --- a/drivers/pci/controller/plda/pcie-starfive.c > > +++ b/drivers/pci/controller/plda/pcie-starfive.c > > @@ -381,7 +381,46 @@ static const struct plda_pcie_host_ops sf_host_ops = { > > .host_deinit = starfive_pcie_host_deinit, > > }; > > > > +/* IRQ handler for PCIe events */ > > +static irqreturn_t starfive_event_handler(int irq, void *dev_id) > > +{ > > + struct plda_pcie_rp *port = dev_id; > > + struct irq_data *data; > > + > > + /* Retrieve IRQ data */ > > + data = irq_domain_get_irq_data(port->event_domain, irq); > > + > > + if (data) { > > + dev_err_ratelimited(port->dev, "Event IRQ %ld triggered\n", > > + data->hwirq); > > + } else { > > + dev_err_ratelimited(port->dev, "Invalid event IRQ %d\n", irq); > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +/* Request an IRQ for a specific event */ > > +static int starfive_request_event_irq(struct plda_pcie_rp *plda, int event_irq, > > + int event) > > +{ > > + int ret; > > + > > + /* Request the IRQ and associate it with the handler */ > > + ret = devm_request_irq(plda->dev, event_irq, starfive_event_handler, > > + IRQF_SHARED, "starfivepcie", plda); > > + > > + if (ret) { > > + dev_err(plda->dev, "Failed to request IRQ %d: %d\n", event_irq, > > + ret); > > + return ret; > > + } > > + > > + return ret; > > +} > > + > > static const struct plda_event stf_pcie_event = { > > + .request_event_irq = starfive_request_event_irq, > > .intx_event = EVENT_PM_MSI_INT_INTX, > > .msi_event = EVENT_PM_MSI_INT_MSI > > }; > > > > base-commit: bb066fe812d6fb3a9d01c073d9f1e2fd5a63403b > > -- > > 2.48.1 > > > > -- > மணிவண்ணன் சதாசிவம்
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
diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c index e73c1b7bc8ef..a57177a8be8a 100644 --- a/drivers/pci/controller/plda/pcie-starfive.c +++ b/drivers/pci/controller/plda/pcie-starfive.c @@ -381,7 +381,46 @@ static const struct plda_pcie_host_ops sf_host_ops = { .host_deinit = starfive_pcie_host_deinit, }; +/* IRQ handler for PCIe events */ +static irqreturn_t starfive_event_handler(int irq, void *dev_id) +{ + struct plda_pcie_rp *port = dev_id; + struct irq_data *data; + + /* Retrieve IRQ data */ + data = irq_domain_get_irq_data(port->event_domain, irq); + + if (data) { + dev_err_ratelimited(port->dev, "Event IRQ %ld triggered\n", + data->hwirq); + } else { + dev_err_ratelimited(port->dev, "Invalid event IRQ %d\n", irq); + } + + return IRQ_HANDLED; +} + +/* Request an IRQ for a specific event */ +static int starfive_request_event_irq(struct plda_pcie_rp *plda, int event_irq, + int event) +{ + int ret; + + /* Request the IRQ and associate it with the handler */ + ret = devm_request_irq(plda->dev, event_irq, starfive_event_handler, + IRQF_SHARED, "starfivepcie", plda); + + if (ret) { + dev_err(plda->dev, "Failed to request IRQ %d: %d\n", event_irq, + ret); + return ret; + } + + return ret; +} + static const struct plda_event stf_pcie_event = { + .request_event_irq = starfive_request_event_irq, .intx_event = EVENT_PM_MSI_INT_INTX, .msi_event = EVENT_PM_MSI_INT_MSI };
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. This patch introduces an event IRQ handler and the necessary infrastructure to manage these IRQs, preventing the memory leak. Fixes: 647690479660 ("PCI: microchip: Add request_event_irq() callback function") Cc: Minda Chen <minda.chen@starfivetech.com> Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- drivers/pci/controller/plda/pcie-starfive.c | 39 +++++++++++++++++++++ 1 file changed, 39 insertions(+) base-commit: bb066fe812d6fb3a9d01c073d9f1e2fd5a63403b