linux-kernel - RE: [PATCH 04/12] i2c: riic: Use pm_runtime_resume_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 12:24:15 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Claudiu.Beznea <claudiu.beznea@...on.dev>, Chris Brandt
	<Chris.Brandt@...esas.com>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"geert+renesas@...der.be" <geert+renesas@...der.be>, "magnus.damm@...il.com"
	<magnus.damm@...il.com>, "mturquette@...libre.com" <mturquette@...libre.com>,
	"sboyd@...nel.org" <sboyd@...nel.org>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, "wsa+renesas@...g-engineering.com"
	<wsa+renesas@...g-engineering.com>
CC: "linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>, Claudiu.Beznea
	<claudiu.beznea@...on.dev>, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: RE: [PATCH 04/12] i2c: riic: Use pm_runtime_resume_and_get()

Hi Claudiu,

Thanks for the patch.

> -----Original Message-----
> From: Claudiu <claudiu.beznea@...on.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@...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@...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
>