linux-kernel - Re: [RFC PATCH v2 0/7] i2c: core: introduce atomic transfers

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: <20190304224856.w7egngqynl3hlabp@ninjato>
Date:   Mon, 4 Mar 2019 23:48:56 +0100
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Peter Rosin <peda@...ntia.se>
Cc:     Wolfram Sang <wsa+renesas@...g-engineering.com>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, Keerthy <j-keerthy@...com>,
        Tony Lindgren <tony@...mide.com>,
        Russell King <linux@...linux.org.uk>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Stefan Lengfeld <contact@...fanchrist.eu>,
        Phil Reid <preid@...ctromag.com.au>,
        Tero Kristo <t-kristo@...com>,
        "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [RFC PATCH v2 0/7] i2c: core: introduce atomic transfers

Hi Peda,

> The way I read this series, you are not giving atomic transfers priority. The

You are reading correctly. I could have made more clear that the issue
pointed out by Russell is not handled by this series but discussion
about it is welcome / needed to decide if we can take this series as is
or if we need to redesign it. But here we are anyhow :)

> only thing that happens is that if an xfer happens in atomic/irq context,
> trylock is used instead of an ordinary (unconditional) lock (this is just
> like it is already). If a mux is sitting in between the client device and
> the root adapter, the trylock operation will percolate to the root. Sure,
> there are more trylock ops that may fail and abort the xfer, but if
> everything is uncontended, then things should proceed in orderly fashion.
> Also, sure, the mux may need additional resources that are no longer
> available if the machine is half way down (or worse). But I don't see any
> fundamental *locking* issue with muxes that is different from the case
> without a mux.

Good, that was my conclusion as well. The series, as is, doesn't change
the locking behaviour, so that will work exactly as before. Or, it will
not work in the case described by Russell. Like before.

> That said, if you then want to introduce xfers that want to circumvent the
> locking, then parent-locked muxes are easier since the actual muxing operation
> is performed as an unlocked xfer (if one is needed) while the client device
> has grabbed the adapter lock "from the outside". Sure, there is a list of
> locks going up through the adapter tree to handle, but that can probably be
> handled in one place. I.e. the locking must have been avoided prior to the
> actual muxing operation, but the code to do so can be in one place. The

That was my gut feeling, too...

> mux-locked case is where the trouble is, since the muxing operation is done
> as a normal xfer and needs to be classified as a special xfer that just like
> the original client xfer also needs to break through any existing locks in
> the adapter tree. And those muxing xfers might come from anywhere, e.g.
> 
> 	- IO-expander controlling a gpio/pinctrl mux
> 	- dedicated I2C mux (e.g. the LTC4306)
> 	- regmap device
> 	- etc, who knows what muxing options will evolve?
> 
> So, any scheme that require a white-list will work poorly for mux-locked
> muxes, unless you can add some new grip/pinctrl/regmap flags to
> gpios/pins/registers so that the particular accesses can be white-listed.
> Adding those flags seem rather invasive?

... and sadly, this too. We would need the same kind of flag which I
described in my first paragraph of the original posting where I wanted
the flag to detect "unauthorized" uses of late I2C transfers. And this
is gonna be invasive. And I am not sure it is worth the effort.

I wonder what a reasonable effort is? Simply ignore the lock from the
"current" adapter and hope for the best that there is no mux or at
least no mux which needs interrupts / a lock attached to it?

> But of course, you need to actually do something about the added FIXME in
> the demux-pinctrl driver... BTW, that driver should forward ->smbus_xfer
> just like it does for ->master_xfer, no?

Yes. The idea of having two seperate SMBus controllers in one SoC which
would need demuxing is amusing, but still, it exists for I2C and you are
right.

Thanks,

   Wolfram


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