| Message ID | 20240625121358.590547-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, > -----Original Message----- > From: Claudiu <claudiu.beznea@tuxon.dev> > Sent: Tuesday, June 25, 2024 1:14 PM > Subject: [PATCH v2 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> > --- > > Changes in v2: > - delete i2c adapter all the time in remove > > drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index > 83e4d5e14ab6..002b11b020fa 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); As at the moment we don't know how to reproduce this error condition Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? Cheers, Biju > + 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,11 +509,18 @@ static void > riic_i2c_remove(struct platform_device *pdev) { > struct riic_dev *riic = platform_get_drvdata(pdev); > struct device *dev = &pdev->dev; > + int ret; > > - pm_runtime_get_sync(dev); > - riic_writeb(riic, 0, RIIC_ICIER); > - pm_runtime_put(dev); > i2c_del_adapter(&riic->adapter); > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + } else { > + riic_writeb(riic, 0, RIIC_ICIER); > + pm_runtime_put(dev); > + } > + > pm_runtime_disable(dev); > } > > -- > 2.39.2 >
Hi, Biju, On 25.06.2024 18:53, Biju Das wrote: > Hi Claudiu, > >> -----Original Message----- >> From: Claudiu <claudiu.beznea@tuxon.dev> >> Sent: Tuesday, June 25, 2024 1:14 PM >> Subject: [PATCH v2 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> >> --- >> >> Changes in v2: >> - delete i2c adapter all the time in remove >> >> drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index >> 83e4d5e14ab6..002b11b020fa 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); > > As at the moment we don't know how to reproduce this error condition > Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with dev_err() or something similar. Thank you, Claudiu Beznea [1] https://lwn.net/Articles/969923/ > > Cheers, > Biju > >> + 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,11 +509,18 @@ static void >> riic_i2c_remove(struct platform_device *pdev) { >> struct riic_dev *riic = platform_get_drvdata(pdev); >> struct device *dev = &pdev->dev; >> + int ret; >> >> - pm_runtime_get_sync(dev); >> - riic_writeb(riic, 0, RIIC_ICIER); >> - pm_runtime_put(dev); >> i2c_del_adapter(&riic->adapter); >> + >> + ret = pm_runtime_resume_and_get(dev); >> + if (ret) { >> + dev_err(dev, riic_rpm_err_msg); >> + } else { >> + riic_writeb(riic, 0, RIIC_ICIER); >> + pm_runtime_put(dev); >> + } >> + >> pm_runtime_disable(dev); >> } >> >> -- >> 2.39.2 >> >
> -----Original Message----- > From: claudiu beznea <claudiu.beznea@tuxon.dev> > Sent: Wednesday, June 26, 2024 7:14 AM > To: Biju Das <biju.das.jz@bp.renesas.com>; Chris Brandt <Chris.Brandt@renesas.com>; > andi.shyti@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > geert+renesas@glider.be; magnus.damm@gmail.com; mturquette@baylibre.com; sboyd@kernel.org; > p.zabel@pengutronix.de; wsa+renesas@sang-engineering.com > Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; Claudiu Beznea > <claudiu.beznea.uj@bp.renesas.com> > Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi, Biju, > > On 25.06.2024 18:53, Biju Das wrote: > > Hi Claudiu, > > > >> -----Original Message----- > >> From: Claudiu <claudiu.beznea@tuxon.dev> > >> Sent: Tuesday, June 25, 2024 1:14 PM > >> Subject: [PATCH v2 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> > >> --- > >> > >> Changes in v2: > >> - delete i2c adapter all the time in remove > >> > >> drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ > >> 1 file changed, 24 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/i2c/busses/i2c-riic.c > >> b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa > >> 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); > > > > As at the moment we don't know how to reproduce this error condition > > Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > > [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with > dev_err() or something similar. WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? Currently we don't know how to trigger pm_runtime_resume_and_get() error condition in our setup using a testapp and we are expecting an error may happen in future. If at all there is an error in future, we need detailed error info so that we can handle it and fix the bug. Cheers, Biju > > Thank you, > Claudiu Beznea > > [1] https://lwn.net/Articles/969923/ > > > > > Cheers, > > Biju > > > >> + 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,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) { > >> struct riic_dev *riic = platform_get_drvdata(pdev); > >> struct device *dev = &pdev->dev; > >> + int ret; > >> > >> - pm_runtime_get_sync(dev); > >> - riic_writeb(riic, 0, RIIC_ICIER); > >> - pm_runtime_put(dev); > >> i2c_del_adapter(&riic->adapter); > >> + > >> + ret = pm_runtime_resume_and_get(dev); > >> + if (ret) { > >> + dev_err(dev, riic_rpm_err_msg); > >> + } else { > >> + riic_writeb(riic, 0, RIIC_ICIER); > >> + pm_runtime_put(dev); > >> + } > >> + > >> pm_runtime_disable(dev); > >> } > >> > >> -- > >> 2.39.2 > >> > >
On 26.06.2024 09:23, Biju Das wrote: > > >> -----Original Message----- >> From: claudiu beznea <claudiu.beznea@tuxon.dev> >> Sent: Wednesday, June 26, 2024 7:14 AM >> To: Biju Das <biju.das.jz@bp.renesas.com>; Chris Brandt <Chris.Brandt@renesas.com>; >> andi.shyti@kernel.org; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; >> geert+renesas@glider.be; magnus.damm@gmail.com; mturquette@baylibre.com; sboyd@kernel.org; >> p.zabel@pengutronix.de; wsa+renesas@sang-engineering.com >> Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; devicetree@vger.kernel.org; >> linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; Claudiu Beznea >> <claudiu.beznea.uj@bp.renesas.com> >> Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() >> >> Hi, Biju, >> >> On 25.06.2024 18:53, Biju Das wrote: >>> Hi Claudiu, >>> >>>> -----Original Message----- >>>> From: Claudiu <claudiu.beznea@tuxon.dev> >>>> Sent: Tuesday, June 25, 2024 1:14 PM >>>> Subject: [PATCH v2 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> >>>> --- >>>> >>>> Changes in v2: >>>> - delete i2c adapter all the time in remove >>>> >>>> drivers/i2c/busses/i2c-riic.c | 30 ++++++++++++++++++++++++------ >>>> 1 file changed, 24 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-riic.c >>>> b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa >>>> 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); >>> >>> As at the moment we don't know how to reproduce this error condition >>> Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? >> >> [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with >> dev_err() or something similar. > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? Ok, I'm leaving this to I2C maintainers. Andi, Wolfram, Would you prefer having WARN_ON_ONCE() instead of dev_err() for potential failures of pm_runtime_resume_and_get()? Thank you, Claudiu Beznea > > Currently we don't know how to trigger pm_runtime_resume_and_get() error > condition in our setup using a testapp and we are expecting an error may > happen in future. If at all there is an error in future, we need detailed > error info so that we can handle it and fix the bug. > > Cheers, > Biju > >> >> Thank you, >> Claudiu Beznea >> >> [1] https://lwn.net/Articles/969923/ >> >>> >>> Cheers, >>> Biju >>> >>>> + 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,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) { >>>> struct riic_dev *riic = platform_get_drvdata(pdev); >>>> struct device *dev = &pdev->dev; >>>> + int ret; >>>> >>>> - pm_runtime_get_sync(dev); >>>> - riic_writeb(riic, 0, RIIC_ICIER); >>>> - pm_runtime_put(dev); >>>> i2c_del_adapter(&riic->adapter); >>>> + >>>> + ret = pm_runtime_resume_and_get(dev); >>>> + if (ret) { >>>> + dev_err(dev, riic_rpm_err_msg); >>>> + } else { >>>> + riic_writeb(riic, 0, RIIC_ICIER); >>>> + pm_runtime_put(dev); >>>> + } >>>> + >>>> pm_runtime_disable(dev); >>>> } >>>> >>>> -- >>>> 2.39.2 >>>> >>>
Hi Biju, On Wed, Jun 26, 2024 at 8:23 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > From: claudiu beznea <claudiu.beznea@tuxon.dev> > > On 25.06.2024 18:53, Biju Das wrote: > > >> From: Claudiu <claudiu.beznea@tuxon.dev> > > >> 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> > > >> - pm_runtime_get_sync(dev); > > >> + ret = pm_runtime_resume_and_get(dev); > > >> + if (ret) { > > >> + dev_err(dev, riic_rpm_err_msg); > > > > > > As at the moment we don't know how to reproduce this error condition > > > Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > > > > [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with > > dev_err() or something similar. > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? > > Currently we don't know how to trigger pm_runtime_resume_and_get() error > condition in our setup using a testapp and we are expecting an error may > happen in future. If at all there is an error in future, we need detailed > error info so that we can handle it and fix the bug. On Renesas systems, pm_runtime_resume_and_get() never fails. That's the reason why originally we didn't care to check the return value of pm_runtime_get_sync(). The various janitors disagreed, causing cascaded changes all over the place... IMHO, WARN_ON_ONCE() is definitely overkill, only bloating the code. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Wednesday, June 26, 2024 8:11 AM > Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Biju, > > On Wed, Jun 26, 2024 at 8:23 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > From: claudiu beznea <claudiu.beznea@tuxon.dev> On 25.06.2024 18:53, > > > Biju Das wrote: > > > >> From: Claudiu <claudiu.beznea@tuxon.dev> > > > >> 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> > > > > >> - pm_runtime_get_sync(dev); > > > >> + ret = pm_runtime_resume_and_get(dev); if (ret) { > > > >> + dev_err(dev, riic_rpm_err_msg); > > > > > > > > As at the moment we don't know how to reproduce this error > > > > condition Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > > > > > > [1] states "So, naturally, use of WARN_ON() is also now discouraged > > > much of the time". I've go with > > > dev_err() or something similar. > > > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? > > > > Currently we don't know how to trigger pm_runtime_resume_and_get() > > error condition in our setup using a testapp and we are expecting an > > error may happen in future. If at all there is an error in future, we > > need detailed error info so that we can handle it and fix the bug. > > On Renesas systems, pm_runtime_resume_and_get() never fails. > That's the reason why originally we didn't care to check the return value of pm_runtime_get_sync(). I agree, I was under the impression, if the code guarantees balanced usage, then pm_runtime_get_sync()/put() it will never fails. But here we are adding checks in frequent calls like xfer on the assumption it may fail in future due to PM changes. Xfer, we are incrementing pm usage count with pm_runtime_get_sync() And then decrementing it with pm_runtime_put() once transfer completed So, there is no imbalance here. > > The various janitors disagreed, causing cascaded changes all over the place... Even the core code does not have check for this. https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L792 > > IMHO, WARN_ON_ONCE() is definitely overkill, only bloating the code. I suggested because we are adding this check because something wrong will happen in future due to PM subsystem changes and the check will capture the issue and will give detailed warning info in kernel log. Cheers, Biju
Hi Claudiu, First of all, thanks Biju for checking the code and bringing up this topic. On Wed, Jun 26, 2024 at 09:30:52AM GMT, claudiu beznea wrote: > >> On 25.06.2024 18:53, Biju Das wrote: ... > >>>> 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); > >>> > >>> As at the moment we don't know how to reproduce this error condition > >>> Can we use WARN_ON_ONCE() instead to catch detailed error condition here?? > >> > >> [1] states "So, naturally, use of WARN_ON() is also now discouraged much of the time". I've go with > >> dev_err() or something similar. > > > > WARN_ON_ONCE() should be ok I guess as people are using for printing this info only once?? > > Ok, I'm leaving this to I2C maintainers. > > Andi, Wolfram, > > Would you prefer having WARN_ON_ONCE() instead of dev_err() for potential > failures of pm_runtime_resume_and_get()? I prefer dev_err. WARN_ON should be used for some serious failures in the code. E.g. memory corruption, like: a = 5; WARN_ON(a != 5); but the system might still work even with such errors (otherwise there is BUG_ON()). Besides, WARN_ON() prints some information that are not really useful to understand why the system didn't resume. For example you don't need the stack trace for power management failures, but you need it for code tracing code bugs. Andi
Hi Claudiu, > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > index 83e4d5e14ab6..002b11b020fa 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"; Please, don't do this. Much clearer to write the message explicitly. > + > 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); In principle I like the error message to be always checked and I will always approve it. Whenever there is a return value, even when we are sure it's always '0', it needs to be checked. I had lots of discussions in the past about this topic but I haven't always found support. I'd love to have the ack from a renesas maintainer here. > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + return ret; > + } > > if (riic_readb(riic, RIIC_ICCR2) & ICCR2_BBSY) { > riic->err = -EBUSY; ... > @@ -498,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) > { > struct riic_dev *riic = platform_get_drvdata(pdev); > struct device *dev = &pdev->dev; > + int ret; > > - pm_runtime_get_sync(dev); > - riic_writeb(riic, 0, RIIC_ICIER); > - pm_runtime_put(dev); > i2c_del_adapter(&riic->adapter); You swapped the position of this. Please put the i2c_del_adapter() at the end. Thanks, Andi > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, riic_rpm_err_msg); > + } else { > + riic_writeb(riic, 0, RIIC_ICIER); > + pm_runtime_put(dev); > + } > + > pm_runtime_disable(dev); > } > > -- > 2.39.2 >
Hi Andi, On Fri, Jul 5, 2024 at 12:42 AM Andi Shyti <andi.shyti@kernel.org> wrote: > > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > > index 83e4d5e14ab6..002b11b020fa 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"; > > Please, don't do this. Much clearer to write the message > explicitly. And the compiler will merge all identical strings, emitting just a single string. > > > + > > 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); > > In principle I like the error message to be always checked and I s/message/condition/? > will always approve it. Whenever there is a return value, even > when we are sure it's always '0', it needs to be checked. > > I had lots of discussions in the past about this topic but I > haven't always found support. I'd love to have the ack from a > renesas maintainer here. I don't mind checking for the error here. > > > + if (ret) { > > + dev_err(dev, riic_rpm_err_msg); Do you need to print these error messages? AFAIU, this cannot happen anyway. Ultimately, I expect the device driver that requested the transfer to handle failures, and print a message when needed. Gr{oetje,eeting}s, Geert
Hi Geert, > > > 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); > > > > In principle I like the error message to be always checked and I > > s/message/condition/? yes :-) > > will always approve it. Whenever there is a return value, even > > when we are sure it's always '0', it needs to be checked. > > > > I had lots of discussions in the past about this topic but I > > haven't always found support. I'd love to have the ack from a > > renesas maintainer here. > > I don't mind checking for the error here. > > > > > > + if (ret) { > > > + dev_err(dev, riic_rpm_err_msg); > > Do you need to print these error messages? I don't think it's needed, indeed. > AFAIU, this cannot happen anyway. That's what I was saying earlier. It's just a different point of view. To be honest, I don't really mind. Thanks, Andi > Ultimately, I expect the device driver that requested the transfer to > handle failures, and print a message when needed. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On 05.07.2024 10:19, Geert Uytterhoeven wrote: > Hi Andi, > > On Fri, Jul 5, 2024 at 12:42 AM Andi Shyti <andi.shyti@kernel.org> wrote: >>> diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c >>> index 83e4d5e14ab6..002b11b020fa 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"; >> >> Please, don't do this. Much clearer to write the message >> explicitly. > > And the compiler will merge all identical strings, emitting > just a single string. > >> >>> + >>> 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); >> >> In principle I like the error message to be always checked and I > > s/message/condition/? > >> will always approve it. Whenever there is a return value, even >> when we are sure it's always '0', it needs to be checked. >> >> I had lots of discussions in the past about this topic but I >> haven't always found support. I'd love to have the ack from a >> renesas maintainer here. > > I don't mind checking for the error here. > >> >>> + if (ret) { >>> + dev_err(dev, riic_rpm_err_msg); > > Do you need to print these error messages? No. I have it here as a result of some internal reviews. Thank you, Claudiu Beznea > AFAIU, this cannot happen anyway. > Ultimately, I expect the device driver that requested the transfer to > handle failures, and print a message when needed. > > Gr{oetje,eeting}s, > > Geert >
Hi Andi and Claudiu, > -----Original Message----- > From: Andi Shyti <andi.shyti@kernel.org> > Sent: Thursday, July 4, 2024 11:43 PM > Subject: Re: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Claudiu, > > > diff --git a/drivers/i2c/busses/i2c-riic.c > > b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa > > 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"; > > Please, don't do this. Much clearer to write the message explicitly. > > > + > > 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); > > In principle I like the error message to be always checked and I will always approve it. Whenever > there is a return value, even when we are sure it's always '0', it needs to be checked. > > I had lots of discussions in the past about this topic but I haven't always found support. I'd love > to have the ack from a renesas maintainer here. > > > + if (ret) { Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and the API does not call put() when ret = 1; see [1] and [2] [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 Another question, pm_runtime_put [3] can also return error, don't we need to propagate that as well to caller?? [3] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L1086 Cheers, Biju
> -----Original Message----- > From: Biju Das > Sent: Monday, July 8, 2024 6:37 AM > Subject: RE: [PATCH v2 04/12] i2c: riic: Use pm_runtime_resume_and_get() > > Hi Andi and Claudiu, > > > -----Original Message----- > > From: Andi Shyti <andi.shyti@kernel.org> > > Sent: Thursday, July 4, 2024 11:43 PM > > Subject: Re: [PATCH v2 04/12] i2c: riic: Use > > pm_runtime_resume_and_get() > > > > Hi Claudiu, > > > > > diff --git a/drivers/i2c/busses/i2c-riic.c > > > b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa > > > 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"; > > > > Please, don't do this. Much clearer to write the message explicitly. > > > > > + > > > 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); > > > > In principle I like the error message to be always checked and I will > > always approve it. Whenever there is a return value, even when we are sure it's always '0', it > needs to be checked. > > > > I had lots of discussions in the past about this topic but I haven't > > always found support. I'd love to have the ack from a renesas maintainer here. > > > > > + if (ret) { > > Checking ret will lead to imbalance. It should be ret < 0 as ret = 1 corresponds to RPM_ACTIVE and > the API does not call put() when ret = 1; see [1] and [2] > > [1] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L778 > > [2] https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/pm_runtime.h#L431 I got confused. the code for pm_runtime_resume_and_get() seems correct. https://elixir.bootlin.com/linux/latest/source/include/linux/pm_runtime.h#L436 > > > Another question, pm_runtime_put [3] can also return error, don't we need to propagate that as well > to caller?? > > [3] https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/base/power/runtime.c#L1086 > > Cheers, > Biju
diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index 83e4d5e14ab6..002b11b020fa 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,11 +509,18 @@ static void riic_i2c_remove(struct platform_device *pdev) { struct riic_dev *riic = platform_get_drvdata(pdev); struct device *dev = &pdev->dev; + int ret; - pm_runtime_get_sync(dev); - riic_writeb(riic, 0, RIIC_ICIER); - pm_runtime_put(dev); i2c_del_adapter(&riic->adapter); + + ret = pm_runtime_resume_and_get(dev); + if (ret) { + dev_err(dev, riic_rpm_err_msg); + } else { + riic_writeb(riic, 0, RIIC_ICIER); + pm_runtime_put(dev); + } + pm_runtime_disable(dev); }