| Message ID | 20170715214406.657273405@cogentembedded.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Sorry, forgot to mention that the patch is against Bjorn Helgaas's pci.git repo's master branch.
On Sat, Jul 15, 2017 at 11:43 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > of_irq_get() may return a negative error number as well as 0 on failure, > while the driver only checks for 0, blithely continuing with the call to > irq_set_chained_handler_and_data() -- that function expects *unsigned int* > so should probably do nothing when a large IRQ number resulting from a > conversion of a negative error number is passed to it. The driver then > probes successfully while being only partly functional... > > Check for 'irq <= 0' instead and propagate the negative error number to > the probe method -- that will allow the deferred probing as well... > > Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Looks correct! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > - return -EINVAL; > + return irq ?: -EINVAL; I thought I knew C but I learn something new all the time. I had no clue one can use the ternary operator like this. Linus Walleij
Hello! On 7/17/2017 12:17 AM, Linus Walleij wrote: >> of_irq_get() may return a negative error number as well as 0 on failure, >> while the driver only checks for 0, blithely continuing with the call to >> irq_set_chained_handler_and_data() -- that function expects *unsigned int* >> so should probably do nothing when a large IRQ number resulting from a >> conversion of a negative error number is passed to it. The driver then >> probes successfully while being only partly functional... >> >> Check for 'irq <= 0' instead and propagate the negative error number to >> the probe method -- that will allow the deferred probing as well... >> >> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > Looks correct! > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thanks! >> - return -EINVAL; >> + return irq ?: -EINVAL; > > I thought I knew C but I learn something new all the time. > I had no clue one can use the ternary operator like this. It's not a standard C, it's gcc's trickery. :-) > Linus Walleij MBR, Sergei
On Sun, Jul 16, 2017 at 12:43:10AM +0300, Sergei Shtylyov wrote: > of_irq_get() may return a negative error number as well as 0 on failure, > while the driver only checks for 0, blithely continuing with the call to > irq_set_chained_handler_and_data() -- that function expects *unsigned int* > so should probably do nothing when a large IRQ number resulting from a > conversion of a negative error number is passed to it. The driver then > probes successfully while being only partly functional... > > Check for 'irq <= 0' instead and propagate the negative error number to > the probe method -- that will allow the deferred probing as well... > > Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks! > --- > drivers/pci/host/pci-ftpci100.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: pci/drivers/pci/host/pci-ftpci100.c > =================================================================== > --- pci.orig/drivers/pci/host/pci-ftpci100.c > +++ pci/drivers/pci/host/pci-ftpci100.c > @@ -350,9 +350,9 @@ static int faraday_pci_setup_cascaded_ir > > /* All PCI IRQs cascade off this one */ > irq = of_irq_get(intc, 0); > - if (!irq) { > + if (irq <= 0) { > dev_err(p->dev, "failed to get parent IRQ\n"); > - return -EINVAL; > + return irq ?: -EINVAL; > } > > p->irqdomain = irq_domain_add_linear(intc, 4, >
Hello! On 8/3/2017 12:36 AM, Bjorn Helgaas wrote: >> of_irq_get() may return a negative error number as well as 0 on failure, >> while the driver only checks for 0, blithely continuing with the call to >> irq_set_chained_handler_and_data() -- that function expects *unsigned int* >> so should probably do nothing when a large IRQ number resulting from a >> conversion of a negative error number is passed to it. The driver then >> probes successfully while being only partly functional... >> >> Check for 'irq <= 0' instead and propagate the negative error number to >> the probe method -- that will allow the deferred probing as well... >> >> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks! Thanks! But why only to 4.14? [...] MBR, Sergei
On Thu, Aug 03, 2017 at 12:32:29PM +0300, Sergei Shtylyov wrote: > Hello! > > On 8/3/2017 12:36 AM, Bjorn Helgaas wrote: > > >>of_irq_get() may return a negative error number as well as 0 on failure, > >>while the driver only checks for 0, blithely continuing with the call to > >>irq_set_chained_handler_and_data() -- that function expects *unsigned int* > >>so should probably do nothing when a large IRQ number resulting from a > >>conversion of a negative error number is passed to it. The driver then > >>probes successfully while being only partly functional... > >> > >>Check for 'irq <= 0' instead and propagate the negative error number to > >>the probe method -- that will allow the deferred probing as well... > >> > >>Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") > >>Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > > >Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks! > > Thanks! But why only to 4.14? Standard practice. We're currently in the v4.13 cycle, and the merge window is closed, so the only changes we add for v4.13 are (1) fixes for something we merged during the v4.13 merge window, or (2) critical fixes that can't wait for v4.14. If we want something backported to stable kernels, we can add a tag for that. That might apply in this case? Your patch is a fix for d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver"), which appeared in v4.12, so we might want to tag it for any v4.12 or v4.13 stable kernels. Bjorn
On 08/03/2017 07:15 PM, Bjorn Helgaas wrote: >>>> of_irq_get() may return a negative error number as well as 0 on failure, >>>> while the driver only checks for 0, blithely continuing with the call to >>>> irq_set_chained_handler_and_data() -- that function expects *unsigned int* >>>> so should probably do nothing when a large IRQ number resulting from a >>>> conversion of a negative error number is passed to it. The driver then >>>> probes successfully while being only partly functional... >>>> >>>> Check for 'irq <= 0' instead and propagate the negative error number to >>>> the probe method -- that will allow the deferred probing as well... >>>> >>>> Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> >>> Applied with Linus' reviewed-by to pci/host-faraday for v4.14, thanks! >> >> Thanks! But why only to 4.14? > > Standard practice. We're currently in the v4.13 cycle, and the merge > window is closed, so the only changes we add for v4.13 are (1) fixes > for something we merged during the v4.13 merge window, or (2) critical > fixes that can't wait for v4.14. OK, I thought the original Linus' commit was a bit more recent, like 4.13... > If we want something backported to stable kernels, we can add a tag > for that. That might apply in this case? Your patch is a fix for > d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host > Bridge driver"), which appeared in v4.12, If not in 4.11... > so we might want to tag it for any v4.12 or v4.13 stable kernels. I think the -stable people look at the Fixes: tag now... > Bjorn MBR, Sergei
Index: pci/drivers/pci/host/pci-ftpci100.c =================================================================== --- pci.orig/drivers/pci/host/pci-ftpci100.c +++ pci/drivers/pci/host/pci-ftpci100.c @@ -350,9 +350,9 @@ static int faraday_pci_setup_cascaded_ir /* All PCI IRQs cascade off this one */ irq = of_irq_get(intc, 0); - if (!irq) { + if (irq <= 0) { dev_err(p->dev, "failed to get parent IRQ\n"); - return -EINVAL; + return irq ?: -EINVAL; } p->irqdomain = irq_domain_add_linear(intc, 4,
of_irq_get() may return a negative error number as well as 0 on failure, while the driver only checks for 0, blithely continuing with the call to irq_set_chained_handler_and_data() -- that function expects *unsigned int* so should probably do nothing when a large IRQ number resulting from a conversion of a negative error number is passed to it. The driver then probes successfully while being only partly functional... Check for 'irq <= 0' instead and propagate the negative error number to the probe method -- that will allow the deferred probing as well... Fixes: d3c68e0a7e34 ("PCI: faraday: Add Faraday Technology FTPCI100 PCI Host Bridge driver") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/pci/host/pci-ftpci100.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)