linux-kernel - Re: [PATCH 01/10] i2c: add suspended flag and accessors for i2c adapters

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]

Message-ID: <20181219223341.GA998@kunai>
Date:   Wed, 19 Dec 2018 23:33:41 +0100
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Lukas Wunner <lukas@...ner.de>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        linux-i2c@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/10] i2c: add suspended flag and accessors for i2c
 adapters

Hi Lukas, Hans,

On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 19-12-18 18:22, Lukas Wunner wrote:
> > On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> > > +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> > > +{
> > > +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> > > +	set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> > > +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> > > +}
> > 
> > This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> > Any reason why you can't use that?  If so, it would be good to document the
> > reason in the commit message.
> 
> Oh, that is a very good point and that one only gets set on system suspend
> and not on resume suspend, working around the problems with the i2c-designware

Just to make it clear: you mean runtime suspend, not resume suspend, or?

> driver.
> 
> I think this might be as simple as adding:
> 
> 	if (WARN_ON(adap->dev.parent->power.is_suspended))
> 		return -ESHUTDOWN;

I have seen this flag but decided against it. One reason is because it
is marked as "PM core only". The other reason is that it doesn't know
about the adapter lock. It might get set while a transfer is on going.
Or even right after the suggested if-block above. The code from this
series gets the mutex first which ensures that on going transfers are
completed and no new ones are started in parallel.

Unless I am totally overlooking something...

Thanks,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)