linux-kernel - Re: [PATCH v2 4/5] i2c: tegra: Add support for SW mutex register

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] [day] [month] [year] [list]

Message-ID: <e1c72d7ac3f21ece4bf6a3f4c58f3fc300292b2f.camel@nvidia.com>
Date: Fri, 31 Jan 2025 06:46:54 +0000
From: Kartik Rajput <kkartik@...dia.com>
To: Laxman Dewangan <ldewangan@...dia.com>, "krzk@...nel.org"
	<krzk@...nel.org>, Jon Hunter <jonathanh@...dia.com>, Akhil R
	<akhilrajeev@...dia.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "robh@...nel.org" <robh@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "andi.shyti@...nel.org"
	<andi.shyti@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"thierry.reding@...il.com" <thierry.reding@...il.com>, "digetx@...il.com"
	<digetx@...il.com>, "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v2 4/5] i2c: tegra: Add support for SW mutex register

On Thu, 2025-01-30 at 18:49 +0100, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 30/01/2025 17:35, Kartik Rajput wrote:
> > > >  /**
> > > > @@ -372,6 +382,103 @@ static void i2c_readsl(struct
> > > > tegra_i2c_dev
> > > > *i2c_dev, void *data,
> > > >       readsl(i2c_dev->base + tegra_i2c_reg_addr(i2c_dev, reg),
> > > > data, len);
> > > >  }
> > > > 
> > > > +static int tegra_i2c_poll_register(struct tegra_i2c_dev
> > > > *i2c_dev,
> > > > +                                u32 reg, u32 mask, u32
> > > > delay_us,
> > > > +                                u32 timeout_us)
> > > > +{
> > > > +     void __iomem *addr = i2c_dev->base +
> > > > tegra_i2c_reg_addr(i2c_dev, reg);
> > > > +     u32 val;
> > > > +
> > > > +     if (!i2c_dev->atomic_mode)
> > > > +             return readl_relaxed_poll_timeout(addr, val,
> > > > !(val &
> > > > mask),
> > > > +                                               delay_us,
> > > > timeout_us);
> > > > +
> > > > +     return readl_relaxed_poll_timeout_atomic(addr, val, !(val
> > > > &
> > > > mask),
> > > > +                                              delay_us,
> > > > timeout_us);
> > > > +}
> > > > +
> > > > +static int tegra_i2c_mutex_trylock(struct tegra_i2c_dev
> > > > *i2c_dev)
> > > > +{
> > > > +     u32 val, id;
> > > > +
> > > > +     val = i2c_readl(i2c_dev, I2C_SW_MUTEX);
> > > > +     id = FIELD_GET(I2C_SW_MUTEX_GRANT, val);
> > > > +     if (id != 0 && id != I2C_SW_MUTEX_ID)
> > > > +             return 0;
> > > > +
> > > > +     val = FIELD_PREP(I2C_SW_MUTEX_REQUEST, I2C_SW_MUTEX_ID);
> > > > +     i2c_writel(i2c_dev, val, I2C_SW_MUTEX);
> > > 
> > > And how do you exactly prevent concurrent, overwriting write?
> > > This
> > > looks
> > > like pure race.
> > > 
> > 
> > The I2C_SW_MUTEX_GRANT field reflects the id of the current mutex
> > owner. The I2C_SW_MUTEX_GRANT field does not change with overwrites
> > to
> > the I2C_SW_MUTEX_REQUEST field, unless I2C_SW_MUTEX_REQUEST field
> > is
> > cleared.
> 
> 
> So second concurrent write to I2C_SW_MUTEX_REQUEST will fail
> silently,
> and you rely on below check which ID succeeded to write?
> 

Correct.

> If that is how it works, then should succeed... except the trouble is
> that you use here i2c_readl/writel wrappers (which was already a poor
> idea, because it hides the implementation for no real gain) and it
> turns
> out they happen to be relaxed making all your assumptions about
> ordering
> inaccurate. You need to switch to non-relaxed API.
> 

Ack. I will update the implementation to use non-relaxed APIs instead.

> 
> Best regards,
> Krzysztof

Thanks & Regards,
Kartik