linux-kernel - Re: [PATCH v3 1/5] usb: aspeed_udc: Use devm_clk_get

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 for Android: free password hash cracker in your pocket

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

Message-ID: <zi5lq44bc4vd33y42zkmn7hr7rw64wfxe2rvuvkekola6ctckc@buv7ybvq5skt>
Date: Wed, 25 Sep 2024 17:07:32 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Lei Liu <liulei.rjpt@...o.com>
Cc: Neal Liu <neal_liu@...eedtech.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Joel Stanley <joel@....id.au>, 
	Andrew Jeffery <andrew@...econstruct.com.au>, linux-aspeed@...ts.ozlabs.org, linux-usb@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, opensource.kernel@...o.com
Subject: Re: [PATCH v3 1/5] usb: aspeed_udc: Use devm_clk_get_enabled()
 helpers

Hello,

On Mon, Aug 26, 2024 at 04:21:39PM +0800, Lei Liu wrote:
> diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c
> index f4781e611aaa..a362e31f7550 100644
> --- a/drivers/usb/gadget/udc/aspeed_udc.c
> +++ b/drivers/usb/gadget/udc/aspeed_udc.c
> @@ -1459,8 +1459,6 @@ static void ast_udc_remove(struct platform_device *pdev)
>  	ctrl = ast_udc_read(udc, AST_UDC_FUNC_CTRL) & ~USB_UPSTREAM_EN;
>  	ast_udc_write(udc, ctrl, AST_UDC_FUNC_CTRL);
>  
> -	clk_disable_unprepare(udc->clk);
> -
>  	spin_unlock_irqrestore(&udc->lock, flags);

Isn't it broken to call clk_disable_unprepare() while holding a
spinlock?

I guess that means that the remove path is untested in practise and this
patches fixes a sleep-in-atomic. IMHO this invalidates Ulf's concern in
his reply to the cover letter for this patch at least.
  
>  	if (udc->ep0_buf)
> @@ -1500,16 +1498,11 @@ static int ast_udc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, udc);
>  
> -	udc->clk = devm_clk_get(&pdev->dev, NULL);
> +	udc->clk = devm_clk_get_enabled(&pdev->dev, NULL);
>  	if (IS_ERR(udc->clk)) {
>  		rc = PTR_ERR(udc->clk);

An error message here would be nice. Something like

	rc = dev_err_probe(&pdev->dev, PTR_ERR(udc->clk), "Failed to get clock\n");

should work.

>  		goto err;
>  	}
> -	rc = clk_prepare_enable(udc->clk);
> -	if (rc) {
> -		dev_err(&pdev->dev, "Failed to enable clock (0x%x)\n", rc);
> -		goto err;
> -	}
>  
>  	/* Check if we need to limit the HW to USB1 */
>  	max_speed = usb_get_maximum_speed(&pdev->dev);

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)