| Message ID | 20200508114453.15436-1-tangbin@cmss.chinamobile.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d49292025f79693d3348f8e2029a8b4703be0f0a |
| Headers | show |
| Series | USB: host: ehci: Add error handling in ehci_mxc_drv_probe() | expand |
On Fri, May 08, 2020 at 07:44:53PM +0800, Tang Bin wrote: > The function ehci_mxc_drv_probe() does not perform sufficient error > checking after executing platform_get_irq(), thus fix it. > > Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > drivers/usb/host/ehci-mxc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c > index a1eb5ee77..a0b42ba59 100644 > --- a/drivers/usb/host/ehci-mxc.c > +++ b/drivers/usb/host/ehci-mxc.c > @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) > } > > irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; <= ?
On Fri, 8 May 2020, Tang Bin wrote: > The function ehci_mxc_drv_probe() does not perform sufficient error > checking after executing platform_get_irq(), thus fix it. Aside from the "irq <= 0" issue, the Subject: line should say "ehci-mxc", not "ehci". Alan Stern > Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > --- > drivers/usb/host/ehci-mxc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c > index a1eb5ee77..a0b42ba59 100644 > --- a/drivers/usb/host/ehci-mxc.c > +++ b/drivers/usb/host/ehci-mxc.c > @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) > } > > irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > > hcd = usb_create_hcd(&ehci_mxc_hc_driver, dev, dev_name(dev)); > if (!hcd) >
Hi, Greg: On 2020/5/8 19:48, Greg KH wrote: > On Fri, May 08, 2020 at 07:44:53PM +0800, Tang Bin wrote: >> The function ehci_mxc_drv_probe() does not perform sufficient error >> checking after executing platform_get_irq(), thus fix it. >> >> Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> --- >> drivers/usb/host/ehci-mxc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c >> index a1eb5ee77..a0b42ba59 100644 >> --- a/drivers/usb/host/ehci-mxc.c >> +++ b/drivers/usb/host/ehci-mxc.c >> @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) >> } >> >> irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; > <= ? In the file 'drivers/base/platform.c'， the function platform_get_irq() is explained and used as follows: * Gets an IRQ for a platform device and prints an error message if finding the * IRQ fails. Device drivers should check the return value for errors so as to * not pass a negative integer value to the request_irq() APIs. * * Example: * int irq = platform_get_irq(pdev, 0); * if (irq < 0) * return irq; * * Return: IRQ number on success, negative error number on failure. And in my hardware experiment, even if I set the irq failed deliberately in the DTS, the returned value is negative instead of zero. Thanks for your patience and replay. Tang Bin
On Fri, May 08, 2020 at 09:55:53PM +0800, Tang Bin wrote: > Hi, Greg: > > On 2020/5/8 19:48, Greg KH wrote: > > On Fri, May 08, 2020 at 07:44:53PM +0800, Tang Bin wrote: > > > The function ehci_mxc_drv_probe() does not perform sufficient error > > > checking after executing platform_get_irq(), thus fix it. > > > > > > Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") > > > Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> > > > Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> > > > --- > > > drivers/usb/host/ehci-mxc.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c > > > index a1eb5ee77..a0b42ba59 100644 > > > --- a/drivers/usb/host/ehci-mxc.c > > > +++ b/drivers/usb/host/ehci-mxc.c > > > @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) > > > } > > > irq = platform_get_irq(pdev, 0); > > > + if (irq < 0) > > > + return irq; > > <= ? > > In the file 'drivers/base/platform.c'， the function platform_get_irq() is > explained and used as follows: > > * Gets an IRQ for a platform device and prints an error message if > finding the > * IRQ fails. Device drivers should check the return value for errors so > as to > * not pass a negative integer value to the request_irq() APIs. > * > * Example: > * int irq = platform_get_irq(pdev, 0); > * if (irq < 0) > * return irq; > * > * Return: IRQ number on success, negative error number on failure. > > And in my hardware experiment, even if I set the irq failed deliberately in > the DTS, the returned value is negative instead of zero. Please read the thread at https://lore.kernel.org/r/20200501224042.141366-1-helgaas%40kernel.org for more details about this. thanks, greg k-h
On 2020/5/8 22:31, Greg KH wrote: > On Fri, May 08, 2020 at 09:55:53PM +0800, Tang Bin wrote: >> Hi, Greg: >> >> On 2020/5/8 19:48, Greg KH wrote: >>> On Fri, May 08, 2020 at 07:44:53PM +0800, Tang Bin wrote: >>>> The function ehci_mxc_drv_probe() does not perform sufficient error >>>> checking after executing platform_get_irq(), thus fix it. >>>> >>>> Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") >>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>>> --- >>>> drivers/usb/host/ehci-mxc.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c >>>> index a1eb5ee77..a0b42ba59 100644 >>>> --- a/drivers/usb/host/ehci-mxc.c >>>> +++ b/drivers/usb/host/ehci-mxc.c >>>> @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) >>>> } >>>> irq = platform_get_irq(pdev, 0); >>>> + if (irq < 0) >>>> + return irq; >>> <= ? >> In the file 'drivers/base/platform.c'， the function platform_get_irq() is >> explained and used as follows: >> >> * Gets an IRQ for a platform device and prints an error message if >> finding the >> * IRQ fails. Device drivers should check the return value for errors so >> as to >> * not pass a negative integer value to the request_irq() APIs. >> * >> * Example: >> * int irq = platform_get_irq(pdev, 0); >> * if (irq < 0) >> * return irq; >> * >> * Return: IRQ number on success, negative error number on failure. >> >> And in my hardware experiment, even if I set the irq failed deliberately in >> the DTS, the returned value is negative instead of zero. > Please read the thread at > https://lore.kernel.org/r/20200501224042.141366-1-helgaas%40kernel.org > for more details about this. > Great, It looks beautiful, finally someone took a knife to the file 'platform.c'. I have been studied this place for a long time, and don't know what platform can return 0, which made me curious. So the example should be: * int irq = platform_get_irq(pdev, 0); * if (irq <= 0) * return irq; Thanks, Tang Bin
On 05/08/2020 06:03 PM, Tang Bin wrote: >>>> On Fri, May 08, 2020 at 07:44:53PM +0800, Tang Bin wrote: >>>>> The function ehci_mxc_drv_probe() does not perform sufficient error >>>>> checking after executing platform_get_irq(), thus fix it. >>>>> >>>>> Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") >>>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>>>> --- >>>>> drivers/usb/host/ehci-mxc.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c >>>>> index a1eb5ee77..a0b42ba59 100644 >>>>> --- a/drivers/usb/host/ehci-mxc.c >>>>> +++ b/drivers/usb/host/ehci-mxc.c >>>>> @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) >>>>> } >>>>> irq = platform_get_irq(pdev, 0); >>>>> + if (irq < 0) >>>>> + return irq; >>>> <= ? >>> In the file 'drivers/base/platform.c'， the function platform_get_irq() is >>> explained and used as follows: >>> >>> * Gets an IRQ for a platform device and prints an error message if >>> finding the >>> * IRQ fails. Device drivers should check the return value for errors so >>> as to >>> * not pass a negative integer value to the request_irq() APIs. >>> * >>> * Example: >>> * int irq = platform_get_irq(pdev, 0); >>> * if (irq < 0) >>> * return irq; >>> * >>> * Return: IRQ number on success, negative error number on failure. >>> >>> And in my hardware experiment, even if I set the irq failed deliberately in >>> the DTS, the returned value is negative instead of zero. >> Please read the thread at >> https://lore.kernel.org/r/20200501224042.141366-1-helgaas%40kernel.org >> for more details about this. >> > Great, It looks beautiful, finally someone took a knife to the file 'platform.c'. I thought I did that already couple years ago, when returned 0 from platform_get_irq() could mean both IRQ # and error... :-) > > I have been studied this place for a long time, and don't know what platform can return 0, which made me curious. > > So the example should be: > > * int irq = platform_get_irq(pdev, 0); > * if (irq <= 0) > * return irq; And you then return 0 (success) as if your probe() succeeded. Congratulations! :-P > > Thanks, > > Tang Bin MBR, Sergei
On 05/08/2020 02:48 PM, Greg KH wrote: >> The function ehci_mxc_drv_probe() does not perform sufficient error >> checking after executing platform_get_irq(), thus fix it. >> >> Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> --- >> drivers/usb/host/ehci-mxc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c >> index a1eb5ee77..a0b42ba59 100644 >> --- a/drivers/usb/host/ehci-mxc.c >> +++ b/drivers/usb/host/ehci-mxc.c >> @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) >> } >> >> irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; > > <= ? I thought I've fixed the ambivalent zero bug some years ago... Please don't do this... MBR, Sergei
Hi Sergei: On 2020/5/9 4:27, Sergei Shtylyov wrote: > On 05/08/2020 06:03 PM, Tang Bin wrote: > >>>>> On Fri, May 08, 2020 at 07:44:53PM +0800, Tang Bin wrote: >>>>>> The function ehci_mxc_drv_probe() does not perform sufficient error >>>>>> checking after executing platform_get_irq(), thus fix it. >>>>>> >>>>>> Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") >>>>>> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >>>>>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >>>>>> --- >>>>>> drivers/usb/host/ehci-mxc.c | 2 ++ >>>>>> 1 file changed, 2 insertions(+) >>>>>> >>>>>> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c >>>>>> index a1eb5ee77..a0b42ba59 100644 >>>>>> --- a/drivers/usb/host/ehci-mxc.c >>>>>> +++ b/drivers/usb/host/ehci-mxc.c >>>>>> @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) >>>>>> } >>>>>> irq = platform_get_irq(pdev, 0); >>>>>> + if (irq < 0) >>>>>> + return irq; >>>>> <= ? >>>> In the file 'drivers/base/platform.c'， the function platform_get_irq() is >>>> explained and used as follows: >>>> >>>> * Gets an IRQ for a platform device and prints an error message if >>>> finding the >>>> * IRQ fails. Device drivers should check the return value for errors so >>>> as to >>>> * not pass a negative integer value to the request_irq() APIs. >>>> * >>>> * Example: >>>> * int irq = platform_get_irq(pdev, 0); >>>> * if (irq < 0) >>>> * return irq; >>>> * >>>> * Return: IRQ number on success, negative error number on failure. >>>> >>>> And in my hardware experiment, even if I set the irq failed deliberately in >>>> the DTS, the returned value is negative instead of zero. >>> Please read the thread at >>> https://lore.kernel.org/r/20200501224042.141366-1-helgaas%40kernel.org >>> for more details about this. >>> >> Great, It looks beautiful, finally someone took a knife to the file 'platform.c'. > I thought I did that already couple years ago, when returned 0 from platform_get_irq() could mean both IRQ # and error... :-) Can you tell me what platform can returned 0? I want to do this test in the hardware. >> I have been studied this place for a long time, and don't know what platform can return 0, which made me curious. >> >> So the example should be: >> >> * int irq = platform_get_irq(pdev, 0); >> * if (irq <= 0) >> * return irq; > And you then return 0 (success) as if your probe() succeeded. Congratulations! :-P Thanks, Tang Bin
Hi gregkh: On 2020/5/8 21:51, Alan Stern wrote: > On Fri, 8 May 2020, Tang Bin wrote: > >> The function ehci_mxc_drv_probe() does not perform sufficient error >> checking after executing platform_get_irq(), thus fix it. > Aside from the "irq <= 0" issue, the Subject: line should say > "ehci-mxc", not "ehci". > I know this patch need v2, whether just fix the subject? Thanks, Tang Bin > >> Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") >> Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> >> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> >> --- >> drivers/usb/host/ehci-mxc.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c >> index a1eb5ee77..a0b42ba59 100644 >> --- a/drivers/usb/host/ehci-mxc.c >> +++ b/drivers/usb/host/ehci-mxc.c >> @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) >> } >> >> irq = platform_get_irq(pdev, 0); >> + if (irq < 0) >> + return irq; >> >> hcd = usb_create_hcd(&ehci_mxc_hc_driver, dev, dev_name(dev)); >> if (!hcd) >>
On Wed, May 13, 2020 at 08:55:59PM +0800, Tang Bin wrote: > Hi gregkh: > > On 2020/5/8 21:51, Alan Stern wrote: > > On Fri, 8 May 2020, Tang Bin wrote: > > > > > The function ehci_mxc_drv_probe() does not perform sufficient error > > > checking after executing platform_get_irq(), thus fix it. > > Aside from the "irq <= 0" issue, the Subject: line should say > > "ehci-mxc", not "ehci". > > > I know this patch need v2, whether just fix the subject? 0 is an invalid irq.
diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c index a1eb5ee77..a0b42ba59 100644 --- a/drivers/usb/host/ehci-mxc.c +++ b/drivers/usb/host/ehci-mxc.c @@ -50,6 +50,8 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) } irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; hcd = usb_create_hcd(&ehci_mxc_hc_driver, dev, dev_name(dev)); if (!hcd)
The function ehci_mxc_drv_probe() does not perform sufficient error checking after executing platform_get_irq(), thus fix it. Fixes: 7e8d5cd93fa ("USB: Add EHCI support for MX27 and MX31 based boards") Signed-off-by: Zhang Shengju <zhangshengju@cmss.chinamobile.com> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com> --- drivers/usb/host/ehci-mxc.c | 2 ++ 1 file changed, 2 insertions(+)