| Message ID | 20211018183930.8448-1-s.shtylyov@omp.ru (mailing list archive) |
|---|---|
| Headers | show |
| Series | Explicitly deny IRQ0 in the USB host drivers | expand |
On Mon, Oct 18, 2021 at 09:39:08PM +0300, Sergey Shtylyov wrote: > Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. > The affected drivers use platform_get_irq() which can return IRQ0 (considered > invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() > called at the end of the probe methods. I think that the solution to this issue > is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this > patch set to get the things going... > > Sergey Shtylyov (22): For patches 1 - 18 (EHCI and OHCI): Acked-by: Alan Stern <stern@rowland.harvard.edu> > usb: host: ehci-exynos: deny IRQ0 > usb: host: ehci-mv: deny IRQ0 > usb: host: ehci-npcm7xx: deny IRQ0 > usb: host: ehci-omap: deny IRQ0 > usb: host: ehci-platform: deny IRQ0 > usb: host: ehci-spear: deny IRQ0 > usb: host: ehci-st: deny IRQ0 > usb: host: ohci-at91: deny IRQ0 > usb: host: ohci-da8xx: deny IRQ0 > usb: host: ohci-exynos: deny IRQ0 > usb: host: ohci-at91: deny IRQ0 > usb: host: ohci-omap: deny IRQ0 > usb: host: ohci-platform: deny IRQ0 > usb: host: ohci-pxa27x: deny IRQ0 > usb: host: ohci-sm501: deny IRQ0 > usb: host: ohci-spear: deny IRQ0 > usb: host: ohci-st: deny IRQ0 > usb: host: ohci-tmio: deny IRQ0 > usb: host: xhci-histb: deny IRQ0 > usb: host: xhci-mtk: deny IRQ0 > usb: host: xhci-plat: deny IRQ0 > usb: host: xhci-tegra: deny IRQ0 > > drivers/usb/host/ehci-exynos.c | 4 ++++ > drivers/usb/host/ehci-mv.c | 4 ++++ > drivers/usb/host/ehci-npcm7xx.c | 4 ++++ > drivers/usb/host/ehci-omap.c | 2 ++ > drivers/usb/host/ehci-platform.c | 2 ++ > drivers/usb/host/ehci-spear.c | 4 ++++ > drivers/usb/host/ehci-st.c | 2 ++ > drivers/usb/host/ohci-at91.c | 2 ++ > drivers/usb/host/ohci-da8xx.c | 4 ++++ > drivers/usb/host/ohci-exynos.c | 4 ++++ > drivers/usb/host/ohci-nxp.c | 4 ++++ > drivers/usb/host/ohci-omap.c | 4 ++++ > drivers/usb/host/ohci-platform.c | 2 ++ > drivers/usb/host/ohci-pxa27x.c | 2 ++ > drivers/usb/host/ohci-sm501.c | 4 ++++ > drivers/usb/host/ohci-spear.c | 4 ++++ > drivers/usb/host/ohci-st.c | 2 ++ > drivers/usb/host/ohci-tmio.c | 2 ++ > drivers/usb/host/xhci-histb.c | 2 ++ > drivers/usb/host/xhci-mtk.c | 4 +++- > drivers/usb/host/xhci-plat.c | 2 ++ > drivers/usb/host/xhci-tegra.c | 2 ++ > 22 files changed, 65 insertions(+), 1 deletion(-) > > -- > 2.26.3
On Mon, Oct 18, 2021 at 09:39:08PM +0300, Sergey Shtylyov wrote: > Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. > The affected drivers use platform_get_irq() which can return IRQ0 (considered > invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() > called at the end of the probe methods. I think that the solution to this issue > is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this > patch set to get the things going... Why not fix the root of the problem for your platform that is failing to assign a valid irq for the device? Are you going to make this change to all callers of this function in the kernel tree? thanks, greg k-h
On Tue, Oct 19, 2021 at 07:41:34AM +0200, Greg Kroah-Hartman wrote: > On Mon, Oct 18, 2021 at 09:39:08PM +0300, Sergey Shtylyov wrote: > > Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. > > The affected drivers use platform_get_irq() which can return IRQ0 (considered > > invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() > > called at the end of the probe methods. I think that the solution to this issue > > is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this > > patch set to get the things going... > > Why not fix the root of the problem for your platform that is failing to > assign a valid irq for the device? > > Are you going to make this change to all callers of this function in the > kernel tree? Also, you should have gotten a huge WARNING in your kernel log if this happens to let you know that something bad is going on. Is this patch series going to really change any of that? What is the root problem here that you are trying to paper over with this patchset? thanks, greg k-h
Hello! On 10/19/21 8:41 AM, Greg Kroah-Hartman wrote: >> Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. >> The affected drivers use platform_get_irq() which can return IRQ0 (considered >> invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() >> called at the end of the probe methods. I think that the solution to this issue >> is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this >> patch set to get the things going... > > Why not fix the root of the problem for your platform that is failing to > assign a valid irq for the device? I'm just auditing the existing code, not developing on a new platform. (I also don't share Linus' opinion on IRQ0, TBH.) > Are you going to make this change to all callers of this function in the > kernel tree? No, only to those drivers that reinterpret IRQ0 as something other, e.g. polling (only libata so far). No change needed to the subsystem that call request_irq() and its ilk w/o filtering out IRQ0. > thanks, > > greg k-h MBR, Sergey
On 10/19/21 10:31 AM, Greg Kroah-Hartman wrote: [...] >>> Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. >>> The affected drivers use platform_get_irq() which can return IRQ0 (considered >>> invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() >>> called at the end of the probe methods. I think that the solution to this issue >>> is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this >>> patch set to get the things going... >> >> Why not fix the root of the problem for your platform that is failing to >> assign a valid irq for the device? >> >> Are you going to make this change to all callers of this function in the >> kernel tree? > > Also, you should have gotten a huge WARNING in your kernel log if this > happens to let you know that something bad is going on. That's the relatively recent addition, yet it doesn't override IRQ0 to s/th like -EINVAL. > Is this patch > series going to really change any of that? How? It doesn't touch drivers/base/platform.c... > > What is the root problem here that you are trying to paper over with > this patchset? As I said, it would be preferrable to either deny IRQ0 in usb_add_hcd() or just don't try to filter it out. The real problem is that usb_add_hcd() does add a non-functioning HCD without the necessary IRQ handling (it only hooks an IRQ when it's non-zero). > thanks, > > greg k-h MBR, Sergey
On Tue, Oct 19, 2021 at 09:28:08PM +0300, Sergey Shtylyov wrote: > On 10/19/21 10:31 AM, Greg Kroah-Hartman wrote: > > [...] > >>> Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. > >>> The affected drivers use platform_get_irq() which can return IRQ0 (considered > >>> invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() > >>> called at the end of the probe methods. I think that the solution to this issue > >>> is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this > >>> patch set to get the things going... > >> > >> Why not fix the root of the problem for your platform that is failing to > >> assign a valid irq for the device? > >> > >> Are you going to make this change to all callers of this function in the > >> kernel tree? > > > > Also, you should have gotten a huge WARNING in your kernel log if this > > happens to let you know that something bad is going on. > > That's the relatively recent addition, yet it doesn't override IRQ0 to s/th > like -EINVAL. > > > Is this patch > > series going to really change any of that? > > How? It doesn't touch drivers/base/platform.c... > > > > > What is the root problem here that you are trying to paper over with > > this patchset? > > As I said, it would be preferrable to either deny IRQ0 in usb_add_hcd() or > just don't try to filter it out. The real problem is that usb_add_hcd() does > add a non-functioning HCD without the necessary IRQ handling (it only hooks > an IRQ when it's non-zero). This is because some HCDs don't use interrupts (e.g., dummy-hcd). Alan Stern
On 10/19/21 9:35 PM, Alan Stern wrote: [...] >>>>> Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. >>>>> The affected drivers use platform_get_irq() which can return IRQ0 (considered >>>>> invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() >>>>> called at the end of the probe methods. I think that the solution to this issue >>>>> is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this >>>>> patch set to get the things going... >>>> >>>> Why not fix the root of the problem for your platform that is failing to >>>> assign a valid irq for the device? >>>> >>>> Are you going to make this change to all callers of this function in the >>>> kernel tree? >>> >>> Also, you should have gotten a huge WARNING in your kernel log if this >>> happens to let you know that something bad is going on. >> >> That's the relatively recent addition, yet it doesn't override IRQ0 to s/th >> like -EINVAL. >> >>> Is this patch >>> series going to really change any of that? >> >> How? It doesn't touch drivers/base/platform.c... >> >>> >>> What is the root problem here that you are trying to paper over with >>> this patchset? >> >> As I said, it would be preferrable to either deny IRQ0 in usb_add_hcd() or >> just don't try to filter it out. The real problem is that usb_add_hcd() does >> add a non-functioning HCD without the necessary IRQ handling (it only hooks >> an IRQ when it's non-zero). > > This is because some HCDs don't use interrupts (e.g., dummy-hcd). Ah, that was the missing piece of a puzzle, thanks! This series doesn't have an alternative then (other than ignoring :-))... > Alan Stern MBR, Sergey
On 10/19/21 9:46 PM, Sergey Shtylyov wrote: [...] >>>>>> Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. >>>>>> The affected drivers use platform_get_irq() which can return IRQ0 (considered >>>>>> invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() >>>>>> called at the end of the probe methods. I think that the solution to this issue >>>>>> is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this >>>>>> patch set to get the things going... >>>>> >>>>> Why not fix the root of the problem for your platform that is failing to >>>>> assign a valid irq for the device? >>>>> >>>>> Are you going to make this change to all callers of this function in the >>>>> kernel tree? >>>> >>>> Also, you should have gotten a huge WARNING in your kernel log if this >>>> happens to let you know that something bad is going on. >>> >>> That's the relatively recent addition, yet it doesn't override IRQ0 to s/th >>> like -EINVAL. >>> >>>> Is this patch >>>> series going to really change any of that? >>> >>> How? It doesn't touch drivers/base/platform.c... >>> >>>> >>>> What is the root problem here that you are trying to paper over with >>>> this patchset? >>> >>> As I said, it would be preferrable to either deny IRQ0 in usb_add_hcd() or >>> just don't try to filter it out. The real problem is that usb_add_hcd() does >>> add a non-functioning HCD without the necessary IRQ handling (it only hooks >>> an IRQ when it's non-zero). >> >> This is because some HCDs don't use interrupts (e.g., dummy-hcd). > > Ah, that was the missing piece of a puzzle, thanks! And some drivers prefer to manage the IRQs themselves too. > This series doesn't have an alternative then (other than ignoring :-))... Or overriding IRQ0 to an error code in platform_get_irq(), finally, of/c... MBR, Sergey
On Mon, Oct 18, 2021 at 09:39:08PM +0300, Sergey Shtylyov wrote: > Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. > The affected drivers use platform_get_irq() which can return IRQ0 (considered > invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() > called at the end of the probe methods. I think that the solution to this issue > is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this > patch set to get the things going... > > Sergey Shtylyov (22): > usb: host: ehci-exynos: deny IRQ0 > usb: host: ehci-mv: deny IRQ0 > usb: host: ehci-npcm7xx: deny IRQ0 > usb: host: ehci-omap: deny IRQ0 > usb: host: ehci-platform: deny IRQ0 > usb: host: ehci-spear: deny IRQ0 > usb: host: ehci-st: deny IRQ0 > usb: host: ohci-at91: deny IRQ0 > usb: host: ohci-da8xx: deny IRQ0 > usb: host: ohci-exynos: deny IRQ0 > usb: host: ohci-at91: deny IRQ0 > usb: host: ohci-omap: deny IRQ0 > usb: host: ohci-platform: deny IRQ0 > usb: host: ohci-pxa27x: deny IRQ0 > usb: host: ohci-sm501: deny IRQ0 > usb: host: ohci-spear: deny IRQ0 > usb: host: ohci-st: deny IRQ0 > usb: host: ohci-tmio: deny IRQ0 > usb: host: xhci-histb: deny IRQ0 > usb: host: xhci-mtk: deny IRQ0 > usb: host: xhci-plat: deny IRQ0 > usb: host: xhci-tegra: deny IRQ0 > > drivers/usb/host/ehci-exynos.c | 4 ++++ > drivers/usb/host/ehci-mv.c | 4 ++++ > drivers/usb/host/ehci-npcm7xx.c | 4 ++++ > drivers/usb/host/ehci-omap.c | 2 ++ > drivers/usb/host/ehci-platform.c | 2 ++ > drivers/usb/host/ehci-spear.c | 4 ++++ > drivers/usb/host/ehci-st.c | 2 ++ > drivers/usb/host/ohci-at91.c | 2 ++ > drivers/usb/host/ohci-da8xx.c | 4 ++++ > drivers/usb/host/ohci-exynos.c | 4 ++++ > drivers/usb/host/ohci-nxp.c | 4 ++++ > drivers/usb/host/ohci-omap.c | 4 ++++ > drivers/usb/host/ohci-platform.c | 2 ++ > drivers/usb/host/ohci-pxa27x.c | 2 ++ > drivers/usb/host/ohci-sm501.c | 4 ++++ > drivers/usb/host/ohci-spear.c | 4 ++++ > drivers/usb/host/ohci-st.c | 2 ++ > drivers/usb/host/ohci-tmio.c | 2 ++ > drivers/usb/host/xhci-histb.c | 2 ++ > drivers/usb/host/xhci-mtk.c | 4 +++- > drivers/usb/host/xhci-plat.c | 2 ++ > drivers/usb/host/xhci-tegra.c | 2 ++ > 22 files changed, 65 insertions(+), 1 deletion(-) Can you update and send a v2 for this series, with Alan's acks added to the proper commits and fix up the other things that people have found? thanks, greg k-h
On 10/20/21 7:16 PM, Greg Kroah-Hartman wrote: [...] >> Here are 22 patches against the 'usb-next' branch of Greg KH's 'usb.git' repo. >> The affected drivers use platform_get_irq() which can return IRQ0 (considered >> invalid, according to Linus) that means broken HCD when passed to usb_add_hcd() >> called at the end of the probe methods. I think that the solution to this issue >> is either explicitly deny or accept IRQ0 in usb_add_hcd()... /but/ here's this >> patch set to get the things going... [...] > Can you update and send a v2 for this series, with Alan's acks added to > the proper commits and fix up the other things that people have found? OK, I'll try. Posting the patches with git for the 1st time, so somewhat afraid to ruin something (which I've already done). :-) > thanks, > > greg k-h MBR, Sergey