| Message ID | 20220206140705.10705-1-sensor1010@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq | expand |
Hi Lihze, On 2/6/22 15:07, lizhe wrote: > Memory allocated with this function is automatically > freed on driver detach > > Signed-off-by: lizhe <sensor1010@163.com> You must use your real name (first-name + last-name) as author, as well as in the Signed-off-by line, see: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin > --- > drivers/pci/hotplug/pciehp_hpc.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c > index 1c1ebf3dad43..aca59c6fdcbc 100644 > --- a/drivers/pci/hotplug/pciehp_hpc.c > +++ b/drivers/pci/hotplug/pciehp_hpc.c > @@ -66,7 +66,7 @@ static inline int pciehp_request_irq(struct controller *ctrl) > } > > /* Installs the interrupt handler */ > - retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist, > + retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist, > IRQF_SHARED, "pciehp", ctrl); > if (retval) > ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n", > @@ -78,8 +78,6 @@ static inline void pciehp_free_irq(struct controller *ctrl) > { > if (pciehp_poll_mode) > kthread_stop(ctrl->poll_thread); > - else > - free_irq(ctrl->pcie->irq, ctrl); > } > > static int pcie_poll_cmd(struct controller *ctrl, int timeout) > You cannot just go and change a single allocation into a devm managed resource, esp. not a request_irq call. Changing this into a devm_allocation means that the irq now will not be free-ed until after pciehp_remove() has completed and that function calls: pciehp_release_ctrl(ctrl); which releases the memory the ctrl pointer points to and that same memory / pointer is used by pciehp_isr. So after your patch it is possible for the IRQ to trigger after pciehp_release_ctrl(ctrl) has free-ed the memory (and before the devm framework calls free_irq() disabling the IRQ), causing pciehp_isr to reference free-ed memory, leading to a crash. Since this patch introduces a bug we cannot accept it, nack. Regards, Hans
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c index 1c1ebf3dad43..aca59c6fdcbc 100644 --- a/drivers/pci/hotplug/pciehp_hpc.c +++ b/drivers/pci/hotplug/pciehp_hpc.c @@ -66,7 +66,7 @@ static inline int pciehp_request_irq(struct controller *ctrl) } /* Installs the interrupt handler */ - retval = request_threaded_irq(irq, pciehp_isr, pciehp_ist, + retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist, IRQF_SHARED, "pciehp", ctrl); if (retval) ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n", @@ -78,8 +78,6 @@ static inline void pciehp_free_irq(struct controller *ctrl) { if (pciehp_poll_mode) kthread_stop(ctrl->poll_thread); - else - free_irq(ctrl->pcie->irq, ctrl); } static int pcie_poll_cmd(struct controller *ctrl, int timeout)
Memory allocated with this function is automatically freed on driver detach Signed-off-by: lizhe <sensor1010@163.com> --- drivers/pci/hotplug/pciehp_hpc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)