diff mbox series

i2c: bcm2835: Fix error handling in bcm2835_i2c_probe

Message ID 20220403063310.7525-1-linmq006@gmail.com
State New
Headers show
Series i2c: bcm2835: Fix error handling in bcm2835_i2c_probe | expand

Commit Message

Miaoqian Lin April 3, 2022, 6:33 a.m. UTC
In the error handling path, the clk_prepare_enable() function
call should be balanced by a corresponding 'clk_disable_unprepare()'
call. And clk_set_rate_exclusive calls clk_rate_exclusive_get(),
it should be balanced with call to clk_rate_exclusive_put().
, as already done in the remove function.

Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")
Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
---
 drivers/i2c/busses/i2c-bcm2835.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Wolfram Sang May 21, 2022, 11:46 a.m. UTC | #1
On Sun, Apr 03, 2022 at 06:33:08AM +0000, Miaoqian Lin wrote:
> In the error handling path, the clk_prepare_enable() function
> call should be balanced by a corresponding 'clk_disable_unprepare()'
> call. And clk_set_rate_exclusive calls clk_rate_exclusive_get(),
> it should be balanced with call to clk_rate_exclusive_put().
> , as already done in the remove function.
> 
> Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")
> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>

Looking for a reviewer here, pretty please?

> ---
>  drivers/i2c/busses/i2c-bcm2835.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
> index 5149454eef4a..d794448866a7 100644
> --- a/drivers/i2c/busses/i2c-bcm2835.c
> +++ b/drivers/i2c/busses/i2c-bcm2835.c
> @@ -454,18 +454,21 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>  	ret = clk_prepare_enable(i2c_dev->bus_clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Couldn't prepare clock");
> -		return ret;
> +		goto err_put_clk;
>  	}
>  
>  	i2c_dev->irq = platform_get_irq(pdev, 0);
> -	if (i2c_dev->irq < 0)
> -		return i2c_dev->irq;
> +	if (i2c_dev->irq < 0) {
> +		ret =  i2c_dev->irq;
> +		goto err_disable_clk;
> +	}
>  
>  	ret = request_irq(i2c_dev->irq, bcm2835_i2c_isr, IRQF_SHARED,
>  			  dev_name(&pdev->dev), i2c_dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Could not request IRQ\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto err_disable_clk;
>  	}
>  
>  	adap = &i2c_dev->adapter;
> @@ -489,8 +492,16 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>  
>  	ret = i2c_add_adapter(adap);
>  	if (ret)
> -		free_irq(i2c_dev->irq, i2c_dev);
> +		goto err_free_irq;
> +
> +	return ret;
>  
> +err_free_irq:
> +	free_irq(i2c_dev->irq, i2c_dev);
> +err_disable_clk:
> +	clk_disable_unprepare(i2c_dev->bus_clk);
> +err_put_clk:
> +	clk_rate_exclusive_put(i2c_dev->bus_clk);
>  	return ret;
>  }
>  
> -- 
> 2.17.1
>
Christophe JAILLET May 21, 2022, 12:28 p.m. UTC | #2
Le 21/05/2022 à 13:46, Wolfram Sang a écrit :
> On Sun, Apr 03, 2022 at 06:33:08AM +0000, Miaoqian Lin wrote:
>> In the error handling path, the clk_prepare_enable() function
>> call should be balanced by a corresponding 'clk_disable_unprepare()'
>> call. And clk_set_rate_exclusive calls clk_rate_exclusive_get(),
>> it should be balanced with call to clk_rate_exclusive_put().
>> , as already done in the remove function.
>>
>> Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")
>> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
> 
> Looking for a reviewer here, pretty please?

Hi,

on which tree are you working?
A similar patch is already available in -next since several months. (see 
[1])

CJ

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/i2c/busses/i2c-bcm2835.c?id=b205f5850263632b6897d8f0bfaeeea4955f8663
> 
>> ---
>>   drivers/i2c/busses/i2c-bcm2835.c | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
>> index 5149454eef4a..d794448866a7 100644
>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>> @@ -454,18 +454,21 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>>   	ret = clk_prepare_enable(i2c_dev->bus_clk);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Couldn't prepare clock");
>> -		return ret;
>> +		goto err_put_clk;
>>   	}
>>   
>>   	i2c_dev->irq = platform_get_irq(pdev, 0);
>> -	if (i2c_dev->irq < 0)
>> -		return i2c_dev->irq;
>> +	if (i2c_dev->irq < 0) {
>> +		ret =  i2c_dev->irq;
>> +		goto err_disable_clk;
>> +	}
>>   
>>   	ret = request_irq(i2c_dev->irq, bcm2835_i2c_isr, IRQF_SHARED,
>>   			  dev_name(&pdev->dev), i2c_dev);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Could not request IRQ\n");
>> -		return -ENODEV;
>> +		ret = -ENODEV;
>> +		goto err_disable_clk;
>>   	}
>>   
>>   	adap = &i2c_dev->adapter;
>> @@ -489,8 +492,16 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>>   
>>   	ret = i2c_add_adapter(adap);
>>   	if (ret)
>> -		free_irq(i2c_dev->irq, i2c_dev);
>> +		goto err_free_irq;
>> +
>> +	return ret;
>>   
>> +err_free_irq:
>> +	free_irq(i2c_dev->irq, i2c_dev);
>> +err_disable_clk:
>> +	clk_disable_unprepare(i2c_dev->bus_clk);
>> +err_put_clk:
>> +	clk_rate_exclusive_put(i2c_dev->bus_clk);
>>   	return ret;
>>   }
>>   
>> -- 
>> 2.17.1
>>
Miaoqian Lin May 21, 2022, 1:56 p.m. UTC | #3
On 2022/5/21 20:28, Christophe JAILLET wrote:
> Le 21/05/2022 à 13:46, Wolfram Sang a écrit :
>> On Sun, Apr 03, 2022 at 06:33:08AM +0000, Miaoqian Lin wrote:
>>> In the error handling path, the clk_prepare_enable() function
>>> call should be balanced by a corresponding 'clk_disable_unprepare()'
>>> call. And clk_set_rate_exclusive calls clk_rate_exclusive_get(),
>>> it should be balanced with call to clk_rate_exclusive_put().
>>> , as already done in the remove function.
>>>
>>> Fixes: bebff81fb8b9 ("i2c: bcm2835: Model Divider in CCF")
>>> Signed-off-by: Miaoqian Lin <linmq006@gmail.com>
>>
>> Looking for a reviewer here, pretty please?
>
> Hi,
>
> on which tree are you working?
> A similar patch is already available in -next since several months. (see [1])
>

Hi,

I mainly work on master, sorry I didn't notice it when submitted.

I may make a mistake with synchronize, thanks for your reply.

> CJ
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/i2c/busses/i2c-bcm2835.c?id=b205f5850263632b6897d8f0bfaeeea4955f8663
>>
>>> ---
>>>   drivers/i2c/busses/i2c-bcm2835.c | 21 ++++++++++++++++-----
>>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
>>> index 5149454eef4a..d794448866a7 100644
>>> --- a/drivers/i2c/busses/i2c-bcm2835.c
>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c
>>> @@ -454,18 +454,21 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>>>       ret = clk_prepare_enable(i2c_dev->bus_clk);
>>>       if (ret) {
>>>           dev_err(&pdev->dev, "Couldn't prepare clock");
>>> -        return ret;
>>> +        goto err_put_clk;
>>>       }
>>>         i2c_dev->irq = platform_get_irq(pdev, 0);
>>> -    if (i2c_dev->irq < 0)
>>> -        return i2c_dev->irq;
>>> +    if (i2c_dev->irq < 0) {
>>> +        ret =  i2c_dev->irq;
>>> +        goto err_disable_clk;
>>> +    }
>>>         ret = request_irq(i2c_dev->irq, bcm2835_i2c_isr, IRQF_SHARED,
>>>                 dev_name(&pdev->dev), i2c_dev);
>>>       if (ret) {
>>>           dev_err(&pdev->dev, "Could not request IRQ\n");
>>> -        return -ENODEV;
>>> +        ret = -ENODEV;
>>> +        goto err_disable_clk;
>>>       }
>>>         adap = &i2c_dev->adapter;
>>> @@ -489,8 +492,16 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
>>>         ret = i2c_add_adapter(adap);
>>>       if (ret)
>>> -        free_irq(i2c_dev->irq, i2c_dev);
>>> +        goto err_free_irq;
>>> +
>>> +    return ret;
>>>   +err_free_irq:
>>> +    free_irq(i2c_dev->irq, i2c_dev);
>>> +err_disable_clk:
>>> +    clk_disable_unprepare(i2c_dev->bus_clk);
>>> +err_put_clk:
>>> +    clk_rate_exclusive_put(i2c_dev->bus_clk);
>>>       return ret;
>>>   }
>>>   -- 
>>> 2.17.1
>>>
>
Wolfram Sang May 22, 2022, 9:01 p.m. UTC | #4
> on which tree are you working?

Thanks for the heads up!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 5149454eef4a..d794448866a7 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -454,18 +454,21 @@  static int bcm2835_i2c_probe(struct platform_device *pdev)
 	ret = clk_prepare_enable(i2c_dev->bus_clk);
 	if (ret) {
 		dev_err(&pdev->dev, "Couldn't prepare clock");
-		return ret;
+		goto err_put_clk;
 	}
 
 	i2c_dev->irq = platform_get_irq(pdev, 0);
-	if (i2c_dev->irq < 0)
-		return i2c_dev->irq;
+	if (i2c_dev->irq < 0) {
+		ret =  i2c_dev->irq;
+		goto err_disable_clk;
+	}
 
 	ret = request_irq(i2c_dev->irq, bcm2835_i2c_isr, IRQF_SHARED,
 			  dev_name(&pdev->dev), i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not request IRQ\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto err_disable_clk;
 	}
 
 	adap = &i2c_dev->adapter;
@@ -489,8 +492,16 @@  static int bcm2835_i2c_probe(struct platform_device *pdev)
 
 	ret = i2c_add_adapter(adap);
 	if (ret)
-		free_irq(i2c_dev->irq, i2c_dev);
+		goto err_free_irq;
+
+	return ret;
 
+err_free_irq:
+	free_irq(i2c_dev->irq, i2c_dev);
+err_disable_clk:
+	clk_disable_unprepare(i2c_dev->bus_clk);
+err_put_clk:
+	clk_rate_exclusive_put(i2c_dev->bus_clk);
 	return ret;
 }