linux-kernel - Re: [PATCH v3] i2c: at91: support atomic write xfer

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: <20200506171710.GA6019@qmqm.qmqm.pl>
Date:   Wed, 6 May 2020 19:17:10 +0200
From:   Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:     Wolfram Sang <wsa@...-dreams.de>
Cc:     Ludovic Desroches <ludovic.desroches@...rochip.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Stefan Lengfeld <contact@...fanchrist.eu>,
        Marco Felsch <m.felsch@...gutronix.de>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] i2c: at91: support atomic write xfer

On Tue, May 05, 2020 at 06:47:39PM +0200, Michał Mirosław wrote:
> On Tue, May 05, 2020 at 05:52:28PM +0200, Wolfram Sang wrote:
[...]
> > > BTW, I found this comment in i2c-core.h:
> > > 
> > >  * We only allow atomic transfers for very late communication, e.g. to send
> > >  * the powerdown command to a PMIC. Atomic transfers are a corner case and not
> > >  * for generic use! 
> > > 
> > > I think this covers the idea.
> > 
> > Well, since I implemented the atomic_xfer mechanism, I think I am the
> > primary authority of what "covers the idea", so I will fix the comment
> > above :) Note, there is also this comment in the way more user-visible
> > include/linux/i2c.h:
> > 
> >  509  * @master_xfer_atomic: same as @master_xfer. Yet, only using atomic context
> >  510  *   so e.g. PMICs can be accessed very late before shutdown. Optional.
> 
> So, we don't have to wonder what the author had in mind. Lets expand
> the idea then. :-) 
> 
> Shutdown is kind of special atomic context in that it is ok to do long
> waits (as I2C requires) because nothing else is there to do. This is
> very unlike normal atomic context. Do you plan to have it work in other
> contexts? What are the idea and use cases for atomic-context transfers?
> 
> I guess we might want it for suspend/resume, but I think there is an
> early stage (with all non-atomic stuff working) and NOIRQ stage (when
> most everything is already shutdown). When a PMIC needs a read, I would
> actually do it ("prepare" the PMIC) in the early stage if possible.

For a followup, I did a quick grep for pm_power_off in i2c drivers [1]
and looked around how are the shutdown handlers implemented. Mostly I
see regmap_update_bits() (almost all with a regcache) and plain writes.
No driver checks if the I2C controller provides atomic transfers - all
assume it is possible.

Coming back to the original patch, I think that WARN on error from the
atomic is transfer is missing here. The core tries to use normal
master_xfer in atomic context as a fallback, but I'm not sure this
actually works (I wrote the patch because it didn't).

If the driver API had split submit and wait callbacks, this could be
much easier, as there would only be need to implement atomic wait part
differently most of the time.

Best Regards,
Michał Mirosław

[1] grep -rl 'i2c\|smbus' $(grep pm_power_off -rl drivers/)