[PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[Dmitry Torokhov]

Caution needs to be exercised when mixing together regular and managed
resources. In case of this driver devm_request_threaded_irq() should
not be used, because it will result in the interrupt being freed too
late, and there being a chance that it fires up at an inopportune
moment and reference already freed data structures.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/nfc/s3fwrn5/i2c.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index f824dc7099ce..fb36860df81c 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -231,9 +231,9 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto disable_clk;
 
-	ret = devm_request_threaded_irq(&client->dev, phy->i2c_dev->irq, NULL,
-		s3fwrn5_i2c_irq_thread_fn, IRQF_ONESHOT,
-		S3FWRN5_I2C_DRIVER_NAME, phy);
+	ret = request_threaded_irq(phy->i2c_dev->irq,
+				   NULL, s3fwrn5_i2c_irq_thread_fn,
+				   IRQF_ONESHOT, S3FWRN5_I2C_DRIVER_NAME, phy);
 	if (ret)
 		goto s3fwrn5_remove;
 
@@ -250,6 +250,7 @@ static void s3fwrn5_i2c_remove(struct i2c_client *client)
 {
 	struct s3fwrn5_i2c_phy *phy = i2c_get_clientdata(client);
 
+	free_irq(phy->i2c_dev->irq, phy);
 	s3fwrn5_remove(phy->common.ndev);
 	clk_disable_unprepare(phy->clk);
 }
-- 
2.38.0.rc1.362.ged0d419d3c-goog

[PATCH net-next 2/2] nfc: s3fwrn5: use devm_clk_get_optional_enabled() helper

[Dmitry Torokhov]

Because we enable the clock immediately after acquiring it in probe,
we can combine the 2 operations and use devm_clk_get_optional_enabled()
helper.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/nfc/s3fwrn5/i2c.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
index fb36860df81c..2aaee2a8ff1c 100644
--- a/drivers/nfc/s3fwrn5/i2c.c
+++ b/drivers/nfc/s3fwrn5/i2c.c
@@ -209,22 +209,16 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	phy->clk = devm_clk_get_optional(&client->dev, NULL);
-	if (IS_ERR(phy->clk))
-		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
-				     "failed to get clock\n");
-
 	/*
 	 * S3FWRN5 depends on a clock input ("XI" pin) to function properly.
 	 * Depending on the hardware configuration this could be an always-on
 	 * oscillator or some external clock that must be explicitly enabled.
 	 * Make sure the clock is running before starting S3FWRN5.
 	 */
-	ret = clk_prepare_enable(phy->clk);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed to enable clock: %d\n", ret);
-		return ret;
-	}
+	phy->clk = devm_clk_get_optional_enabled(&client->dev, NULL);
+	if (IS_ERR(phy->clk))
+		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
+				     "failed to get clock\n");
 
 	ret = s3fwrn5_probe(&phy->common.ndev, phy, &phy->i2c_dev->dev,
 			    &i2c_phy_ops);
-- 
2.38.0.rc1.362.ged0d419d3c-goog

Re: [PATCH net-next 2/2] nfc: s3fwrn5: use devm_clk_get_optional_enabled() helper

[Dmitry Torokhov]

On Wed, Sep 28, 2022 at 10:04:26PM -0700, Dmitry Torokhov wrote:
> Because we enable the clock immediately after acquiring it in probe,
> we can combine the 2 operations and use devm_clk_get_optional_enabled()
> helper.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/nfc/s3fwrn5/i2c.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nfc/s3fwrn5/i2c.c b/drivers/nfc/s3fwrn5/i2c.c
> index fb36860df81c..2aaee2a8ff1c 100644
> --- a/drivers/nfc/s3fwrn5/i2c.c
> +++ b/drivers/nfc/s3fwrn5/i2c.c
> @@ -209,22 +209,16 @@ static int s3fwrn5_i2c_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	phy->clk = devm_clk_get_optional(&client->dev, NULL);
> -	if (IS_ERR(phy->clk))
> -		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
> -				     "failed to get clock\n");
> -
>  	/*
>  	 * S3FWRN5 depends on a clock input ("XI" pin) to function properly.
>  	 * Depending on the hardware configuration this could be an always-on
>  	 * oscillator or some external clock that must be explicitly enabled.
>  	 * Make sure the clock is running before starting S3FWRN5.
>  	 */
> -	ret = clk_prepare_enable(phy->clk);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "failed to enable clock: %d\n", ret);
> -		return ret;
> -	}
> +	phy->clk = devm_clk_get_optional_enabled(&client->dev, NULL);
> +	if (IS_ERR(phy->clk))
> +		return dev_err_probe(&client->dev, PTR_ERR(phy->clk),
> +				     "failed to get clock\n");

I forgot to remove clk_disable_unprepare() in remove method, I will
resubmit.

Thanks.

-- 
Dmitry

Re: [PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[Krzysztof Kozlowski]

On 29/09/2022 07:04, Dmitry Torokhov wrote:
> Caution needs to be exercised when mixing together regular and managed
> resources. In case of this driver devm_request_threaded_irq() should
> not be used, because it will result in the interrupt being freed too
> late, and there being a chance that it fires up at an inopportune
> moment and reference already freed data structures.

Non-devm was so far recommended only for IRQF_SHARED, not for regular
ones. Otherwise you have to fix half of Linux kernel drivers... why is
s3fwrn5 special?

Please use scripts/get_maintainers.pl to Cc also netdev folks.

> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/nfc/s3fwrn5/i2c.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
Best regards,
Krzysztof

Re: [PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[Krzysztof Kozlowski]

On 29/09/2022 09:26, Krzysztof Kozlowski wrote:
> On 29/09/2022 07:04, Dmitry Torokhov wrote:
>> Caution needs to be exercised when mixing together regular and managed
>> resources. In case of this driver devm_request_threaded_irq() should
>> not be used, because it will result in the interrupt being freed too
>> late, and there being a chance that it fires up at an inopportune
>> moment and reference already freed data structures.
> 
> Non-devm was so far recommended only for IRQF_SHARED, not for regular
> ones. Otherwise you have to fix half of Linux kernel drivers... why is
> s3fwrn5 special?
> 


> Please use scripts/get_maintainers.pl to Cc also netdev folks.

Ah, they are not printed for NFC drivers. So never mind last comment.

Best regards,
Krzysztof

Re: [PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[Dmitry Torokhov]

On September 29, 2022 12:27:19 AM PDT, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>On 29/09/2022 09:26, Krzysztof Kozlowski wrote:
>> On 29/09/2022 07:04, Dmitry Torokhov wrote:
>>> Caution needs to be exercised when mixing together regular and managed
>>> resources. In case of this driver devm_request_threaded_irq() should
>>> not be used, because it will result in the interrupt being freed too
>>> late, and there being a chance that it fires up at an inopportune
>>> moment and reference already freed data structures.
>> 
>> Non-devm was so far recommended only for IRQF_SHARED, not for regular
>> ones.

If we are absolutely sure there is no possibility of interrupts firing then devm
should be ok, but it is much safer not to use it. Or use custom devm actions
to free non-managed resources.

>> Otherwise you have to fix half of Linux kernel drivers... 

Yes, if they are doing the wrong thing.

>> why is s3fwrn5 special?
>> 

I simply happened to look at it.

>
>> Please use scripts/get_maintainers.pl to Cc also netdev folks.
>
>Ah, they are not printed for NFC drivers. So never mind last comment.
>
>Best regards,
>Krzysztof
>

Thanks.

-- 
Dmitry

Re: [PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[Krzysztof Kozlowski]

On 29/09/2022 09:37, Dmitry Torokhov wrote:
> On September 29, 2022 12:27:19 AM PDT, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> On 29/09/2022 09:26, Krzysztof Kozlowski wrote:
>>> On 29/09/2022 07:04, Dmitry Torokhov wrote:
>>>> Caution needs to be exercised when mixing together regular and managed
>>>> resources. In case of this driver devm_request_threaded_irq() should
>>>> not be used, because it will result in the interrupt being freed too
>>>> late, and there being a chance that it fires up at an inopportune
>>>> moment and reference already freed data structures.
>>>
>>> Non-devm was so far recommended only for IRQF_SHARED, not for regular
>>> ones.
> 
> If we are absolutely sure there is no possibility of interrupts firing then devm
> should be ok, but it is much safer not to use it. Or use custom devm actions
> to free non-managed resources.

I am not sure and the pattern itself is a bit risky, I agree. However
the driver calls s3fwrn5_remove() which then calls
s3fwrn5_phy_power_ctrl() which cuts the power via GPIO pin. I assume
that the hardware should stop generating interrupts at this point.

> 
>>> Otherwise you have to fix half of Linux kernel drivers... 
> 
> Yes, if they are doing the wrong thing.

What I meant, that this pattern appears pretty often. If we agree that
this driver has a risky pattern (hardware might not be off after
remove() callback), then we should maybe document it somewhere and
include it in usual reviews.

Best regards,
Krzysztof

Re: [PATCH net-next 1/2] nfc: s3fwrn5: fix order of freeing resources

[Dmitry Torokhov]

On September 29, 2022 12:42:08 AM PDT, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>On 29/09/2022 09:37, Dmitry Torokhov wrote:
>> On September 29, 2022 12:27:19 AM PDT, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>> On 29/09/2022 09:26, Krzysztof Kozlowski wrote:
>>>> On 29/09/2022 07:04, Dmitry Torokhov wrote:
>>>>> Caution needs to be exercised when mixing together regular and managed
>>>>> resources. In case of this driver devm_request_threaded_irq() should
>>>>> not be used, because it will result in the interrupt being freed too
>>>>> late, and there being a chance that it fires up at an inopportune
>>>>> moment and reference already freed data structures.
>>>>
>>>> Non-devm was so far recommended only for IRQF_SHARED, not for regular
>>>> ones.
>> 
>> If we are absolutely sure there is no possibility of interrupts firing then devm
>> should be ok, but it is much safer not to use it. Or use custom devm actions
>> to free non-managed resources.
>
>I am not sure and the pattern itself is a bit risky, I agree. However
>the driver calls s3fwrn5_remove() which then calls
>s3fwrn5_phy_power_ctrl() which cuts the power via GPIO pin. I assume
>that the hardware should stop generating interrupts at this point.

Ok, fair enough. The GPIO is mandatory, so I guess the chip will be disabled.

>
>> 
>>>> Otherwise you have to fix half of Linux kernel drivers... 
>> 
>> Yes, if they are doing the wrong thing.
>
>What I meant, that this pattern appears pretty often. If we agree that
>this driver has a risky pattern (hardware might not be off after
>remove() callback), then we should maybe document it somewhere and
>include it in usual reviews.

I have been saying that mixing managed and non managed resource is
dangerous for years. Disabling clocks before shutting off interrupts, freeing
memory, etc, is not safe. The best approach with devm is all or nothing.

Thanks.

-- 
Dmitry