linux-kernel - Re: [PATCH] USB: EHCI: use module_platform

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1611231550510.22736-100000@netrider.rowland.org>
Date:   Wed, 23 Nov 2016 15:56:43 -0500 (EST)
From:   Alan Stern <stern@...land.harvard.edu>
To:     csmanjuvijay@...il.com
cc:     Arnd Bergmann <arnd@...db.de>, Greg KH <greg@...ah.com>,
        Wan ZongShun <mcuos.com@...il.com>,
        <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] USB: EHCI: use module_platform_driver macro

On Tue, 22 Nov 2016 csmanjuvijay@...il.com wrote:

> From: Majunath Goudar <csmanjuvijay@...il.com>
> 
> Use the module_platform_driver macro to do module init/exit.
> This eliminates a lot of boilerplate.This also removes redundant
> code and overhead of a function call.

I really don't like this patch, or the corresponding patch for 
ohci-nxp.c.  By moving the contents of ehci_w90X900_init() into
ehci_w90x900_probe(), you cause it to run at the wrong time and 
possibly run more than once.

(Also, the title of this patch is wrong.  You are not changing the EHCI 
core files; you are changing the specific ehci-w90x900 driver.)

On the other hand, I do like the fact that the patch removes two 
useless functions in the probe and remove paths.  But that can be done 
separately.

Alan Stern

> Signed-off-by: Manjunath Goudar <csmanjuvijay@...il.com>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Greg KH <greg@...ah.com>
> Cc: Alan Stern <stern@...land.harvard.edu>
> Cc: Wan ZongShun <mcuos.com@...il.com>
> Cc: linux-usb@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  drivers/usb/host/ehci-w90x900.c | 55 ++++++++++++-----------------------------
>  1 file changed, 16 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-w90x900.c b/drivers/usb/host/ehci-w90x900.c
> index e42a29e..4cb2651 100644
> --- a/drivers/usb/host/ehci-w90x900.c
> +++ b/drivers/usb/host/ehci-w90x900.c
> @@ -33,8 +33,7 @@ static const char hcd_name[] = "ehci-w90x900 ";
>  
>  static struct hc_driver __read_mostly ehci_w90x900_hc_driver;
>  
> -static int usb_w90x900_probe(const struct hc_driver *driver,
> -		      struct platform_device *pdev)
> +static int ehci_w90x900_probe(struct platform_device *pdev)
>  {
>  	struct usb_hcd *hcd;
>  	struct ehci_hcd *ehci;
> @@ -42,7 +41,15 @@ static int usb_w90x900_probe(const struct hc_driver *driver,
>  	int retval = 0, irq;
>  	unsigned long val;
>  
> -	hcd = usb_create_hcd(driver, &pdev->dev, "w90x900 EHCI");
> +	if (usb_disabled())
> +		return -ENODEV;
> +
> +	pr_info("%s: " DRIVER_DESC "\n", hcd_name);
> +
> +	ehci_init_driver(&ehci_w90x900_hc_driver, NULL);
> +
> +	hcd = usb_create_hcd(&ehci_w90x900_hc_driver, &pdev->dev,
> +			"w90x900 EHCI");
>  	if (!hcd) {
>  		retval = -ENOMEM;
>  		goto err1;
> @@ -63,9 +70,9 @@ static int usb_w90x900_probe(const struct hc_driver *driver,
>  		HC_LENGTH(ehci, ehci_readl(ehci, &ehci->caps->hc_capbase));
>  
>  	/* enable PHY 0,1,the regs only apply to w90p910
> -	*  0xA4,0xA8 were offsets of PHY0 and PHY1 controller of
> -	*  w90p910 IC relative to ehci->regs.
> -	*/
> +	 *  0xA4,0xA8 were offsets of PHY0 and PHY1 controller of
> +	 *  w90p910 IC relative to ehci->regs.
> +	 */
>  	val = __raw_readl(ehci->regs+PHY0_CTR);
>  	val |= ENPHY;
>  	__raw_writel(val, ehci->regs+PHY0_CTR);
> @@ -92,26 +99,12 @@ static int usb_w90x900_probe(const struct hc_driver *driver,
>  	return retval;
>  }
>  
> -static void usb_w90x900_remove(struct usb_hcd *hcd,
> -			struct platform_device *pdev)
> -{
> -	usb_remove_hcd(hcd);
> -	usb_put_hcd(hcd);
> -}
> -
> -static int ehci_w90x900_probe(struct platform_device *pdev)
> -{
> -	if (usb_disabled())
> -		return -ENODEV;
> -
> -	return usb_w90x900_probe(&ehci_w90x900_hc_driver, pdev);
> -}
> -
>  static int ehci_w90x900_remove(struct platform_device *pdev)
>  {
>  	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>  
> -	usb_w90x900_remove(hcd, pdev);
> +	usb_remove_hcd(hcd);
> +	usb_put_hcd(hcd);
>  
>  	return 0;
>  }
> @@ -124,23 +117,7 @@ static struct platform_driver ehci_hcd_w90x900_driver = {
>  	},
>  };
>  
> -static int __init ehci_w90X900_init(void)
> -{
> -	if (usb_disabled())
> -		return -ENODEV;
> -
> -	pr_info("%s: " DRIVER_DESC "\n", hcd_name);
> -
> -	ehci_init_driver(&ehci_w90x900_hc_driver, NULL);
> -	return platform_driver_register(&ehci_hcd_w90x900_driver);
> -}
> -module_init(ehci_w90X900_init);
> -
> -static void __exit ehci_w90X900_cleanup(void)
> -{
> -	platform_driver_unregister(&ehci_hcd_w90x900_driver);
> -}
> -module_exit(ehci_w90X900_cleanup);
> +module_platform_driver(ehci_hcd_w90x900_driver);
>  
>  MODULE_DESCRIPTION(DRIVER_DESC);
>  MODULE_AUTHOR("Wan ZongShun <mcuos.com@...il.com>");
>