Re: [PATCH] serial: pl011: Fix error handling in pl011_init()

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Nov 09, 2022 at 04:46:26PM +0800, Gaosheng Cui wrote:
> When amba_driver_register failed, we need to unregister the platform
> driver which have been registered, otherwise there maybe resource leak,
> so we add error handlings in pl011_init() to fix it.
> 
> Fixes: 0dd1e247fd39 ("drivers: PL011: add support for the ARM SBSA generic UART")
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 6d8552506091..ef6d9941f972 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2982,11 +2982,23 @@ static struct amba_driver pl011_driver = {
>  
>  static int __init pl011_init(void)
>  {
> +	int ret;
> +
>  	printk(KERN_INFO "Serial: AMBA PL011 UART driver\n");
>  
> -	if (platform_driver_register(&arm_sbsa_uart_platform_driver))
> +	ret = platform_driver_register(&arm_sbsa_uart_platform_driver);
> +	if (unlikely(ret)) {

Only use likely/unlikely if you can prove with a benchmark that it makes
a measurable change to do so.

Here, on the init error path, it will never happen.

And how are you causing this call to fail?

How was this tested?

>  		pr_warn("could not register SBSA UART platform driver\n");
> -	return amba_driver_register(&pl011_driver);
> +		return ret;
> +	}
> +
> +	ret = amba_driver_register(&pl011_driver);
> +	if (unlikely(ret)) {

Again, never use unlikely() like this.

And again, how did you get this to fail under normal operation?  And how
was this tested?

thanks,

greg k-h