diff mbox series

[04/12] i2c: riic: Use pm_runtime_resume_and_get()

Message ID 20240621112303.1607621-5-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series i2c: riic: Add support for Renesas RZ/G3S | expand

Commit Message

Claudiu Beznea June 21, 2024, 11:22 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pm_runtime_get_sync() may return with error. In case it returns with error
dev->power.usage_count needs to be decremented. pm_runtime_resume_and_get()
takes care of this. Thus use it.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/i2c/busses/i2c-riic.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

Comments

Biju Das June 21, 2024, 12:24 p.m. UTC | #1 
Hi Claudiu,

Thanks for the patch.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@tuxon.dev>
> Sent: Friday, June 21, 2024 12:23 PM
> Subject: [PATCH 04/12] i2c: riic: Use pm_runtime_resume_and_get()
> 
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> pm_runtime_get_sync() may return with error. In case it returns with error
> dev->power.usage_count needs to be decremented.
> dev->pm_runtime_resume_and_get()
> takes care of this. Thus use it.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>  drivers/i2c/busses/i2c-riic.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
> 83e4d5e14ab6..6b739483ef37 100644
> --- a/drivers/i2c/busses/i2c-riic.c
> +++ b/drivers/i2c/busses/i2c-riic.c
> @@ -113,6 +113,8 @@ struct riic_irq_desc {
>  	char *name;
>  };
> 
> +static const char * const riic_rpm_err_msg = "Failed to runtime
> +resume";
> +
>  static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)  {
>  	writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 +135,14 @@ static int
> riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	struct riic_dev *riic = i2c_get_adapdata(adap);
>  	struct device *dev = adap->dev.parent;
>  	unsigned long time_left;
> -	int i;
> +	int i, ret;
>  	u8 start_bit;
> 
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, riic_rpm_err_msg);
> +		return ret;
> +	}
> 
>  	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
>  		riic->err = -EBUSY;
> @@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = {
> 
>  static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)  {
> +	int ret;
>  	unsigned long rate;
>  	int total_ticks, cks, brl, brh;
>  	struct device *dev = riic->adapter.dev.parent; @@ -379,7 +386,11 @@ static int
> riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
>  		 t->scl_fall_ns / (1000000000 / rate),
>  		 t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
> 
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, riic_rpm_err_msg);
> +		return ret;
> +	}
> 
>  	/* Changing the order of accessing IICRST and ICE may break things! */
>  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1); @@ -498,8 +509,14 @@ static void
> riic_i2c_remove(struct platform_device *pdev)  {
>  	struct riic_dev *riic = platform_get_drvdata(pdev);
>  	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		dev_err(dev, riic_rpm_err_msg);
> +		return;
> +	}

This change will lead to resource leak. Maybe if there is error
skip accessing the register. Or restore previous code,
just ignore condition in remove.

There are other place in i2c core driver where this call can fail.
You could fix as well.
https://elixir.bootlin.com/linux/v6.10-rc4/source/drivers/i2c/i2c-core-base.c#L509

Cheers,
Biju

> 
> -	pm_runtime_get_sync(dev);
>  	riic_writeb(riic, 0, RIIC_ICIER);
>  	pm_runtime_put(dev);
>  	i2c_del_adapter(&riic->adapter);
> --
> 2.39.2
>
Claudiu Beznea June 21, 2024, 12:49 p.m. UTC | #2 
Hi, Biju,

On 21.06.2024 15:24, Biju Das wrote:
> Hi Claudiu,
> 
> Thanks for the patch.
> 
>> -----Original Message-----
>> From: Claudiu <claudiu.beznea@tuxon.dev>
>> Sent: Friday, June 21, 2024 12:23 PM
>> Subject: [PATCH 04/12] i2c: riic: Use pm_runtime_resume_and_get()
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> pm_runtime_get_sync() may return with error. In case it returns with error
>> dev->power.usage_count needs to be decremented.
>> dev->pm_runtime_resume_and_get()
>> takes care of this. Thus use it.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>  drivers/i2c/busses/i2c-riic.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index
>> 83e4d5e14ab6..6b739483ef37 100644
>> --- a/drivers/i2c/busses/i2c-riic.c
>> +++ b/drivers/i2c/busses/i2c-riic.c
>> @@ -113,6 +113,8 @@ struct riic_irq_desc {
>>  	char *name;
>>  };
>>
>> +static const char * const riic_rpm_err_msg = "Failed to runtime
>> +resume";
>> +
>>  static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)  {
>>  	writeb(val, riic->base + riic->info->regs[offset]); @@ -133,10 +135,14 @@ static int
>> riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>  	struct riic_dev *riic = i2c_get_adapdata(adap);
>>  	struct device *dev = adap->dev.parent;
>>  	unsigned long time_left;
>> -	int i;
>> +	int i, ret;
>>  	u8 start_bit;
>>
>> -	pm_runtime_get_sync(dev);
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret) {
>> +		dev_err(dev, riic_rpm_err_msg);
>> +		return ret;
>> +	}
>>
>>  	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
>>  		riic->err = -EBUSY;
>> @@ -301,6 +307,7 @@ static const struct i2c_algorithm riic_algo = {
>>
>>  static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)  {
>> +	int ret;
>>  	unsigned long rate;
>>  	int total_ticks, cks, brl, brh;
>>  	struct device *dev = riic->adapter.dev.parent; @@ -379,7 +386,11 @@ static int
>> riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
>>  		 t->scl_fall_ns / (1000000000 / rate),
>>  		 t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
>>
>> -	pm_runtime_get_sync(dev);
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret) {
>> +		dev_err(dev, riic_rpm_err_msg);
>> +		return ret;
>> +	}
>>
>>  	/* Changing the order of accessing IICRST and ICE may break things! */
>>  	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1); @@ -498,8 +509,14 @@ static void
>> riic_i2c_remove(struct platform_device *pdev)  {
>>  	struct riic_dev *riic = platform_get_drvdata(pdev);
>>  	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret) {
>> +		dev_err(dev, riic_rpm_err_msg);
>> +		return;
>> +	}
> 
> This change will lead to resource leak. Maybe if there is error
> skip accessing the register. Or restore previous code,
> just ignore condition in remove.

Ok, I'll delete the adapter.

> 
> There are other place in i2c core driver where this call can fail.
> You could fix as well.
> https://elixir.bootlin.com/linux/v6.10-rc4/source/drivers/i2c/i2c-core-base.c#L509

Yes, there are many other places as well. Wolfram, would you prefer
touching that code, as well?

Thank you,
Claudiu Beznea

> 
> Cheers,
> Biju
> 
>>
>> -	pm_runtime_get_sync(dev);
>>  	riic_writeb(riic, 0, RIIC_ICIER);
>>  	pm_runtime_put(dev);
>>  	i2c_del_adapter(&riic->adapter);
>> --
>> 2.39.2
>>
>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c
index 83e4d5e14ab6..6b739483ef37 100644
--- a/drivers/i2c/busses/i2c-riic.c
+++ b/drivers/i2c/busses/i2c-riic.c
@@ -113,6 +113,8 @@  struct riic_irq_desc {
 	char *name;
 };
 
+static const char * const riic_rpm_err_msg = "Failed to runtime resume";
+
 static inline void riic_writeb(struct riic_dev *riic, u8 val, u8 offset)
 {
 	writeb(val, riic->base + riic->info->regs[offset]);
@@ -133,10 +135,14 @@  static int riic_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	struct riic_dev *riic = i2c_get_adapdata(adap);
 	struct device *dev = adap->dev.parent;
 	unsigned long time_left;
-	int i;
+	int i, ret;
 	u8 start_bit;
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, riic_rpm_err_msg);
+		return ret;
+	}
 
 	if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) {
 		riic->err = -EBUSY;
@@ -301,6 +307,7 @@  static const struct i2c_algorithm riic_algo = {
 
 static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 {
+	int ret;
 	unsigned long rate;
 	int total_ticks, cks, brl, brh;
 	struct device *dev = riic->adapter.dev.parent;
@@ -379,7 +386,11 @@  static int riic_init_hw(struct riic_dev *riic, struct i2c_timings *t)
 		 t->scl_fall_ns / (1000000000 / rate),
 		 t->scl_rise_ns / (1000000000 / rate), cks, brl, brh);
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, riic_rpm_err_msg);
+		return ret;
+	}
 
 	/* Changing the order of accessing IICRST and ICE may break things! */
 	riic_writeb(riic, ICCR1_IICRST | ICCR1_SOWP, RIIC_ICCR1);
@@ -498,8 +509,14 @@  static void riic_i2c_remove(struct platform_device *pdev)
 {
 	struct riic_dev *riic = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		dev_err(dev, riic_rpm_err_msg);
+		return;
+	}
 
-	pm_runtime_get_sync(dev);
 	riic_writeb(riic, 0, RIIC_ICIER);
 	pm_runtime_put(dev);
 	i2c_del_adapter(&riic->adapter);