lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 2 Oct 2020 13:06:24 +0300 From: Roger Quadros <rogerq@...com> To: Pawel Laszczak <pawell@...ence.com>, "balbi@...nel.org" <balbi@...nel.org> CC: "peter.chen@....org" <peter.chen@....org>, "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Rahul Kumar <kurahul@...ence.com> Subject: Re: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname Pawel, On 02/10/2020 12:08, Pawel Laszczak wrote: > Roger, > >> >> On 30/09/2020 09:57, Pawel Laszczak wrote: >>> To avoid duplicate error information patch replaces platform_get_irq_byname >>> into platform_get_irq_byname_optional. >> >> What is duplicate error information? > > The function platform_get_irq_byname print: > " dev_err(&dev->dev, "IRQ %s not found\n", name);" if error occurred. > > In core.c we have the another error message below invoking this function. > e.g > if (cdns->dev_irq < 0) > dev_err(dev, "couldn't get peripheral irq\n"); > > So, it's looks like one dev_err is redundant. If we want all 3 IRQs to be valid irrespective of dr_mode then we should use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER). We can get rid of the irq check and duplicate error message in other places. cheers, -roger > >> >>> >>> A change was suggested during reviewing CDNSP driver by Chunfeng Yun. >>> >>> Signed-off-by: Pawel Laszczak <pawell@...ence.com> >>> --- >>> drivers/usb/cdns3/core.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c >>> index a0f73d4711ae..a3f6dc44cf3a 100644 >>> --- a/drivers/usb/cdns3/core.c >>> +++ b/drivers/usb/cdns3/core.c >>> @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev) >>> >>> cdns->xhci_res[1] = *res; >>> >>> - cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral"); >>> + cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral"); >> >> As per DT binding document, these are mandatory properties > > I think that name platform_get_irq_byname_optional is little confusing. > Function descriptions show that both are almost identical: > /** > * platform_get_irq_byname_optional - get an optional IRQ for a device by name > * @dev: platform device > * @name: IRQ name > * > * Get an optional IRQ by name like platform_get_irq_byname(). Except that it > * does not print an error message if an IRQ can not be obtained. > * > * Return: non-zero IRQ number on success, negative error number on failure. > */ > >> >> - interrupts: Interrupts used by cdns3 controller: >> "host" - interrupt used by XHCI driver. >> "peripheral" - interrupt used by device driver >> "otg" - interrupt used by DRD/OTG part of driver >> >> for dr_mode == "otg" -> all 3 are mandatory. >> for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required. >> for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required. >> >>> if (cdns->dev_irq == -EPROBE_DEFER) >>> return cdns->dev_irq; >>> >>> @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev) >>> return PTR_ERR(regs); >>> cdns->dev_regs = regs; >>> >>> - cdns->otg_irq = platform_get_irq_byname(pdev, "otg"); >>> + cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg"); >>> if (cdns->otg_irq == -EPROBE_DEFER) >>> return cdns->otg_irq; >>> >>> >> > > Regards, > Pawel > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Powered by blists - more mailing lists