| Message ID | 20240621112303.1607621-5-claudiu.beznea.uj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | i2c: riic: Add support for Renesas RZ/G3S | expand |
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 >
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 --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);