* Re: [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq
2022-02-06 14:07 [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq lizhe
@ 2022-02-06 14:34 ` Hans de Goede
2022-02-06 16:14 ` kernel test robot
2022-02-06 18:37 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2022-02-06 14:34 UTC (permalink / raw)
To: lizhe, bhelgaas, lukas, ameynarkhede03, naveennaidu479
Cc: linux-pci, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq
2022-02-06 14:07 [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq lizhe
2022-02-06 14:34 ` Hans de Goede
@ 2022-02-06 16:14 ` kernel test robot
2022-02-06 18:37 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-02-06 16:14 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7505 bytes --]
Hi lizhe,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on helgaas-pci/next]
[also build test WARNING on v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/lizhe/PCI-pciehp-Replace-request_threaded_irq-with-devm_request_threaded_irq/20220206-222442
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220207/202202070030.sdA7lHCM-lkp(a)intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/8c1aa010219ad629dc74363ec0392a9e868ca536
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review lizhe/PCI-pciehp-Replace-request_threaded_irq-with-devm_request_threaded_irq/20220206-222442
git checkout 8c1aa010219ad629dc74363ec0392a9e868ca536
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/pci/hotplug/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/pci/hotplug/pciehp_hpc.c: In function 'pciehp_request_irq':
>> drivers/pci/hotplug/pciehp_hpc.c:69:44: warning: passing argument 1 of 'devm_request_threaded_irq' makes pointer from integer without a cast [-Wint-conversion]
69 | retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
| ^~~
| |
| int
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:205:42: note: expected 'struct device *' but argument is of type 'int'
205 | devm_request_threaded_irq(struct device *dev, unsigned int irq,
| ~~~~~~~~~~~~~~~^~~
>> drivers/pci/hotplug/pciehp_hpc.c:69:49: warning: passing argument 2 of 'devm_request_threaded_irq' makes integer from pointer without a cast [-Wint-conversion]
69 | retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
| ^~~~~~~~~~
| |
| irqreturn_t (*)(int, void *) {aka enum irqreturn (*)(int, void *)}
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:205:60: note: expected 'unsigned int' but argument is of type 'irqreturn_t (*)(int, void *)' {aka 'enum irqreturn (*)(int, void *)'}
205 | devm_request_threaded_irq(struct device *dev, unsigned int irq,
| ~~~~~~~~~~~~~^~~
>> include/linux/interrupt.h:71:33: warning: passing argument 4 of 'devm_request_threaded_irq' makes pointer from integer without a cast [-Wint-conversion]
71 | #define IRQF_SHARED 0x00000080
| ^~~~~~~~~~
| |
| int
drivers/pci/hotplug/pciehp_hpc.c:70:39: note: in expansion of macro 'IRQF_SHARED'
70 | IRQF_SHARED, "pciehp", ctrl);
| ^~~~~~~~~~~
include/linux/interrupt.h:206:64: note: expected 'irq_handler_t' {aka 'enum irqreturn (*)(int, void *)'} but argument is of type 'int'
206 | irq_handler_t handler, irq_handler_t thread_fn,
| ~~~~~~~~~~~~~~^~~~~~~~~
drivers/pci/hotplug/pciehp_hpc.c:70:52: warning: passing argument 5 of 'devm_request_threaded_irq' makes integer from pointer without a cast [-Wint-conversion]
70 | IRQF_SHARED, "pciehp", ctrl);
| ^~~~~~~~
| |
| char *
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:207:41: note: expected 'long unsigned int' but argument is of type 'char *'
207 | unsigned long irqflags, const char *devname,
| ~~~~~~~~~~~~~~^~~~~~~~
drivers/pci/hotplug/pciehp_hpc.c:70:62: error: passing argument 6 of 'devm_request_threaded_irq' from incompatible pointer type [-Werror=incompatible-pointer-types]
70 | IRQF_SHARED, "pciehp", ctrl);
| ^~~~
| |
| struct controller *
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:207:63: note: expected 'const char *' but argument is of type 'struct controller *'
207 | unsigned long irqflags, const char *devname,
| ~~~~~~~~~~~~^~~~~~~
drivers/pci/hotplug/pciehp_hpc.c:69:18: error: too few arguments to function 'devm_request_threaded_irq'
69 | retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:205:1: note: declared here
205 | devm_request_threaded_irq(struct device *dev, unsigned int irq,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/devm_request_threaded_irq +69 drivers/pci/hotplug/pciehp_hpc.c
56
57 static inline int pciehp_request_irq(struct controller *ctrl)
58 {
59 int retval, irq = ctrl->pcie->irq;
60
61 if (pciehp_poll_mode) {
62 ctrl->poll_thread = kthread_run(&pciehp_poll, ctrl,
63 "pciehp_poll-%s",
64 slot_name(ctrl));
65 return PTR_ERR_OR_ZERO(ctrl->poll_thread);
66 }
67
68 /* Installs the interrupt handler */
> 69 retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
70 IRQF_SHARED, "pciehp", ctrl);
71 if (retval)
72 ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n",
73 irq);
74 return retval;
75 }
76
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq
2022-02-06 14:07 [PATCH] PCI:pciehp: Replace request_threaded_irq with devm_request_threaded_irq lizhe
2022-02-06 14:34 ` Hans de Goede
2022-02-06 16:14 ` kernel test robot
@ 2022-02-06 18:37 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2022-02-06 18:37 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 8314 bytes --]
Hi lizhe,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.17-rc2 next-20220204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/lizhe/PCI-pciehp-Replace-request_threaded_irq-with-devm_request_threaded_irq/20220206-222442
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220207/202202070253.VcJImRNn-lkp(a)intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/8c1aa010219ad629dc74363ec0392a9e868ca536
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review lizhe/PCI-pciehp-Replace-request_threaded_irq-with-devm_request_threaded_irq/20220206-222442
git checkout 8c1aa010219ad629dc74363ec0392a9e868ca536
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/pci/hotplug/pciehp_hpc.c: In function 'pciehp_request_irq':
drivers/pci/hotplug/pciehp_hpc.c:69:44: warning: passing argument 1 of 'devm_request_threaded_irq' makes pointer from integer without a cast [-Wint-conversion]
69 | retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
| ^~~
| |
| int
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:205:42: note: expected 'struct device *' but argument is of type 'int'
205 | devm_request_threaded_irq(struct device *dev, unsigned int irq,
| ~~~~~~~~~~~~~~~^~~
drivers/pci/hotplug/pciehp_hpc.c:69:49: warning: passing argument 2 of 'devm_request_threaded_irq' makes integer from pointer without a cast [-Wint-conversion]
69 | retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
| ^~~~~~~~~~
| |
| irqreturn_t (*)(int, void *) {aka enum irqreturn (*)(int, void *)}
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:205:60: note: expected 'unsigned int' but argument is of type 'irqreturn_t (*)(int, void *)' {aka 'enum irqreturn (*)(int, void *)'}
205 | devm_request_threaded_irq(struct device *dev, unsigned int irq,
| ~~~~~~~~~~~~~^~~
include/linux/interrupt.h:71:33: warning: passing argument 4 of 'devm_request_threaded_irq' makes pointer from integer without a cast [-Wint-conversion]
71 | #define IRQF_SHARED 0x00000080
| ^~~~~~~~~~
| |
| int
drivers/pci/hotplug/pciehp_hpc.c:70:39: note: in expansion of macro 'IRQF_SHARED'
70 | IRQF_SHARED, "pciehp", ctrl);
| ^~~~~~~~~~~
include/linux/interrupt.h:206:64: note: expected 'irq_handler_t' {aka 'enum irqreturn (*)(int, void *)'} but argument is of type 'int'
206 | irq_handler_t handler, irq_handler_t thread_fn,
| ~~~~~~~~~~~~~~^~~~~~~~~
drivers/pci/hotplug/pciehp_hpc.c:70:52: warning: passing argument 5 of 'devm_request_threaded_irq' makes integer from pointer without a cast [-Wint-conversion]
70 | IRQF_SHARED, "pciehp", ctrl);
| ^~~~~~~~
| |
| char *
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:207:41: note: expected 'long unsigned int' but argument is of type 'char *'
207 | unsigned long irqflags, const char *devname,
| ~~~~~~~~~~~~~~^~~~~~~~
>> drivers/pci/hotplug/pciehp_hpc.c:70:62: error: passing argument 6 of 'devm_request_threaded_irq' from incompatible pointer type [-Werror=incompatible-pointer-types]
70 | IRQF_SHARED, "pciehp", ctrl);
| ^~~~
| |
| struct controller *
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:207:63: note: expected 'const char *' but argument is of type 'struct controller *'
207 | unsigned long irqflags, const char *devname,
| ~~~~~~~~~~~~^~~~~~~
>> drivers/pci/hotplug/pciehp_hpc.c:69:18: error: too few arguments to function 'devm_request_threaded_irq'
69 | retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/pci.h:38,
from drivers/pci/hotplug/pciehp_hpc.c:22:
include/linux/interrupt.h:205:1: note: declared here
205 | devm_request_threaded_irq(struct device *dev, unsigned int irq,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/devm_request_threaded_irq +70 drivers/pci/hotplug/pciehp_hpc.c
^1da177e4c3f41 Linus Torvalds 2005-04-16 56
2aeeef11999590 Kenji Kaneshige 2008-04-25 57 static inline int pciehp_request_irq(struct controller *ctrl)
2aeeef11999590 Kenji Kaneshige 2008-04-25 58 {
f7a10e32a1a7ae Kenji Kaneshige 2008-08-22 59 int retval, irq = ctrl->pcie->irq;
2aeeef11999590 Kenji Kaneshige 2008-04-25 60
2aeeef11999590 Kenji Kaneshige 2008-04-25 61 if (pciehp_poll_mode) {
ec07a4473072ff Lukas Wunner 2018-07-19 62 ctrl->poll_thread = kthread_run(&pciehp_poll, ctrl,
ec07a4473072ff Lukas Wunner 2018-07-19 63 "pciehp_poll-%s",
5790a9c78e78aa Lukas Wunner 2018-09-18 64 slot_name(ctrl));
ec07a4473072ff Lukas Wunner 2018-07-19 65 return PTR_ERR_OR_ZERO(ctrl->poll_thread);
2aeeef11999590 Kenji Kaneshige 2008-04-25 66 }
2aeeef11999590 Kenji Kaneshige 2008-04-25 67
2aeeef11999590 Kenji Kaneshige 2008-04-25 68 /* Installs the interrupt handler */
8c1aa010219ad6 lizhe 2022-02-06 @69 retval = devm_request_threaded_irq(irq, pciehp_isr, pciehp_ist,
e07ca82a5fde88 Bjorn Helgaas 2019-05-08 @70 IRQF_SHARED, "pciehp", ctrl);
2aeeef11999590 Kenji Kaneshige 2008-04-25 71 if (retval)
7f2feec140f1f1 Taku Izumi 2008-09-05 72 ctrl_err(ctrl, "Cannot get irq %d for the hotplug controller\n",
7f2feec140f1f1 Taku Izumi 2008-09-05 73 irq);
2aeeef11999590 Kenji Kaneshige 2008-04-25 74 return retval;
2aeeef11999590 Kenji Kaneshige 2008-04-25 75 }
2aeeef11999590 Kenji Kaneshige 2008-04-25 76
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 4+ messages in thread