drivers/pci/controller/plda/pcie-starfive.c | 39 +++++++++++++++++++++ 1 file changed, 39 insertions(+)
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(+)
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.1On 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
--
மணிவண்ணன் சதாசிவம்
Hi Manivannan
On Fri, 14 Feb 2025 at 11:39, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> 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?
>
The PCIe host driver is responsible for monitoring the PLDA PCIe EVENT irq.
However, I have been unable to locate the register map necessary to
implement the
required code updates,
alarm@archriscv:/media/nvme0/mainline/linux-riscv-6.y-devel$ cat
/proc/interrupts
CPU0 CPU1 CPU2 CPU3
10: 101145 76126 79040 80218 RISC-V INTC 5 Edge
riscv-timer
12: 3 0 0 0 SiFive PLIC 111 Edge
17030000.power-controller
13: 20 0 0 0 SiFive PLIC 30 Edge
1600c000.rng
14: 0 0 0 0 SiFive PLIC 1 Edge
ccache_ecc
15: 0 0 0 0 SiFive PLIC 3 Edge
ccache_ecc
16: 0 0 0 0 SiFive PLIC 4 Edge
ccache_ecc
17: 0 0 0 0 SiFive PLIC 2 Edge
ccache_ecc
20: 0 0 0 0 SiFive PLIC 73 Edge
dw_axi_dmac_platform
21: 1694 0 0 0 SiFive PLIC 32 Edge ttyS0
22: 0 0 0 0 SiFive PLIC 35 Edge
10030000.i2c
23: 0 0 0 0 SiFive PLIC 75 Edge
dw-mci
24: 0 0 0 0 SiFive PLIC 37 Edge
10050000.i2c
25: 171 0 0 0 SiFive PLIC 50 Edge
12050000.i2c
26: 0 0 0 0 SiFive PLIC 51 Edge
12060000.i2c
27: 10419 0 0 0 SiFive PLIC 74 Edge
dw-mci
28: 6 0 0 0 SiFive PLIC 25 Edge
13010000.spi
29: 0 0 0 0 SiFive PLIC 38 Edge pl022
31: 0 0 0 0 PLDA PCIe EVENT 16
Edge 940000000.pcie
32: 0 0 0 0 PLDA PCIe EVENT 17
Edge 940000000.pcie
33: 0 0 0 0 PLDA PCIe EVENT 18
Edge 940000000.pcie
34: 0 0 0 0 PLDA PCIe EVENT 20
Edge 940000000.pcie
35: 0 0 0 0 PLDA PCIe EVENT 21
Edge 940000000.pcie
36: 0 0 0 0 PLDA PCIe EVENT 22
Edge 940000000.pcie
39: 0 0 0 0 PLDA PCIe EVENT 26
Edge 940000000.pcie
40: 0 0 0 0 PLDA PCIe EVENT 27
Edge 940000000.pcie
41: 0 0 0 0 17020000.pinctrl 41
Edge 16020000.mmc cd
42: 0 0 0 0 PLDA PCIe EVENT 28
Edge 940000000.pcie
44: 0 0 0 0 PLDA PCIe MSI 0
Edge PCIe PME, aerdrv, PCIe bwctrl
46: 0 0 0 0 PLDA PCIe EVENT 16
Edge 9c0000000.pcie
47: 0 0 0 0 PLDA PCIe EVENT 17
Edge 9c0000000.pcie
48: 0 0 0 0 PLDA PCIe EVENT 18
Edge 9c0000000.pcie
49: 0 0 0 0 PLDA PCIe EVENT 20
Edge 9c0000000.pcie
50: 0 0 0 0 PLDA PCIe EVENT 21
Edge 9c0000000.pcie
51: 0 0 0 0 PLDA PCIe EVENT 22
Edge 9c0000000.pcie
54: 0 0 0 0 PLDA PCIe EVENT 26
Edge 9c0000000.pcie
55: 0 0 0 0 PLDA PCIe EVENT 27
Edge 9c0000000.pcie
56: 0 0 0 0 PLDA PCIe EVENT 28
Edge 9c0000000.pcie
58: 0 0 0 0 PLDA PCIe MSI
134217728 Edge PCIe PME, aerdrv, PCIe bwctrl
Yep, Thanks for your review comment.
The following changes fix the kernel memory leak warning at my end.
$ git diff
diff --git a/drivers/pci/controller/plda/pcie-plda-host.c
b/drivers/pci/controller/plda/pcie-plda-host.c
index 4153214ca410..d4e7c1b49607 100644
--- a/drivers/pci/controller/plda/pcie-plda-host.c
+++ b/drivers/pci/controller/plda/pcie-plda-host.c
@@ -461,6 +461,7 @@ int plda_init_interrupts(struct platform_device *pdev,
if (ret) {
dev_err(dev, "failed to request IRQ %d\n", event_irq);
+ irq_dispose_mapping(event_irq);
return ret;
}
}
> > > > 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.
You are correct.
>
> - Mani
>
Thanks
-Anand
> --
> மணிவண்ணன் சதாசிவம்
Hi Manivannan,
On Thu, 20 Feb 2025 at 13:43, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Manivannan
>
> On Fri, 14 Feb 2025 at 11:39, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > 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?
> >
> The PCIe host driver is responsible for monitoring the PLDA PCIe EVENT irq.
> However, I have been unable to locate the register map necessary to
> implement the
> required code updates,
>
> alarm@archriscv:/media/nvme0/mainline/linux-riscv-6.y-devel$ cat
> /proc/interrupts
> CPU0 CPU1 CPU2 CPU3
> 10: 101145 76126 79040 80218 RISC-V INTC 5 Edge
> riscv-timer
> 12: 3 0 0 0 SiFive PLIC 111 Edge
> 17030000.power-controller
> 13: 20 0 0 0 SiFive PLIC 30 Edge
> 1600c000.rng
> 14: 0 0 0 0 SiFive PLIC 1 Edge
> ccache_ecc
> 15: 0 0 0 0 SiFive PLIC 3 Edge
> ccache_ecc
> 16: 0 0 0 0 SiFive PLIC 4 Edge
> ccache_ecc
> 17: 0 0 0 0 SiFive PLIC 2 Edge
> ccache_ecc
> 20: 0 0 0 0 SiFive PLIC 73 Edge
> dw_axi_dmac_platform
> 21: 1694 0 0 0 SiFive PLIC 32 Edge ttyS0
> 22: 0 0 0 0 SiFive PLIC 35 Edge
> 10030000.i2c
> 23: 0 0 0 0 SiFive PLIC 75 Edge
> dw-mci
> 24: 0 0 0 0 SiFive PLIC 37 Edge
> 10050000.i2c
> 25: 171 0 0 0 SiFive PLIC 50 Edge
> 12050000.i2c
> 26: 0 0 0 0 SiFive PLIC 51 Edge
> 12060000.i2c
> 27: 10419 0 0 0 SiFive PLIC 74 Edge
> dw-mci
> 28: 6 0 0 0 SiFive PLIC 25 Edge
> 13010000.spi
> 29: 0 0 0 0 SiFive PLIC 38 Edge pl022
> 31: 0 0 0 0 PLDA PCIe EVENT 16
> Edge 940000000.pcie
> 32: 0 0 0 0 PLDA PCIe EVENT 17
> Edge 940000000.pcie
> 33: 0 0 0 0 PLDA PCIe EVENT 18
> Edge 940000000.pcie
> 34: 0 0 0 0 PLDA PCIe EVENT 20
> Edge 940000000.pcie
> 35: 0 0 0 0 PLDA PCIe EVENT 21
> Edge 940000000.pcie
> 36: 0 0 0 0 PLDA PCIe EVENT 22
> Edge 940000000.pcie
> 39: 0 0 0 0 PLDA PCIe EVENT 26
> Edge 940000000.pcie
> 40: 0 0 0 0 PLDA PCIe EVENT 27
> Edge 940000000.pcie
> 41: 0 0 0 0 17020000.pinctrl 41
> Edge 16020000.mmc cd
> 42: 0 0 0 0 PLDA PCIe EVENT 28
> Edge 940000000.pcie
> 44: 0 0 0 0 PLDA PCIe MSI 0
> Edge PCIe PME, aerdrv, PCIe bwctrl
> 46: 0 0 0 0 PLDA PCIe EVENT 16
> Edge 9c0000000.pcie
> 47: 0 0 0 0 PLDA PCIe EVENT 17
> Edge 9c0000000.pcie
> 48: 0 0 0 0 PLDA PCIe EVENT 18
> Edge 9c0000000.pcie
> 49: 0 0 0 0 PLDA PCIe EVENT 20
> Edge 9c0000000.pcie
> 50: 0 0 0 0 PLDA PCIe EVENT 21
> Edge 9c0000000.pcie
> 51: 0 0 0 0 PLDA PCIe EVENT 22
> Edge 9c0000000.pcie
> 54: 0 0 0 0 PLDA PCIe EVENT 26
> Edge 9c0000000.pcie
> 55: 0 0 0 0 PLDA PCIe EVENT 27
> Edge 9c0000000.pcie
> 56: 0 0 0 0 PLDA PCIe EVENT 28
> Edge 9c0000000.pcie
> 58: 0 0 0 0 PLDA PCIe MSI
> 134217728 Edge PCIe PME, aerdrv, PCIe bwctrl
>
> Yep, Thanks for your review comment.
>
> The following changes fix the kernel memory leak warning at my end.
>
> $ git diff
> diff --git a/drivers/pci/controller/plda/pcie-plda-host.c
> b/drivers/pci/controller/plda/pcie-plda-host.c
> index 4153214ca410..d4e7c1b49607 100644
> --- a/drivers/pci/controller/plda/pcie-plda-host.c
> +++ b/drivers/pci/controller/plda/pcie-plda-host.c
> @@ -461,6 +461,7 @@ int plda_init_interrupts(struct platform_device *pdev,
>
> if (ret) {
> dev_err(dev, "failed to request IRQ %d\n", event_irq);
> + irq_dispose_mapping(event_irq);
> return ret;
> }
> }
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);
if (ret) {
dev_err(dev, "failed to request IRQ %d\n", event_irq);
+ irq_dispose_mapping(event_irq);
return ret;
}
}
> > > > > 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.
> You are correct.
> >
> > - Mani
> >
> Thanks
> -Anand
Thanks
-Anand
> > --
> > மணிவண்ணன் சதாசிவம்
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.
>
> 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?
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.
- Mani
--
மணிவண்ணன் சதாசிவம்
Hi Manivannan
On Mon, 24 Feb 2025 at 13:31, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> 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.
> >
> > 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
> 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.
>
> - Mani
>
Thanks
-Anand
> --
> மணிவண்ணன் சதாசிவம்
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@linaro.org> 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.
> > >
> > > 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.
- Mani
--
மணிவண்ணன் சதாசிவம்
Hi Manivannan
On Mon, 24 Feb 2025 at 17:24, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> 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@linaro.org> 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
> --
> மணிவண்ணன் சதாசிவம்
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@linaro.org> 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@linaro.org> 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
--
மணிவண்ணன் சதாசிவம்
Hi Manivannan,
On Mon, 24 Feb 2025 at 20:12, Manivannan Sadhasivam <mani@kernel.org> 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@linaro.org> 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@linaro.org> 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
> > 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.
I have tested on the StarFive JH7110 VisionFive2 RISC-V board.
$ 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
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
Thanks
-Anand
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@kernel.org> 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@linaro.org> 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@linaro.org> 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
--
மணிவண்ணன் சதாசிவம்
… > 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
© 2016 - 2026 Red Hat, Inc.