linux-kernel - Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver

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: <20210305072903.wtw645rukmqr5hx5@vireshk-i7>
Date:   Fri, 5 Mar 2021 12:59:03 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Jie Deng <jie.deng@...el.com>
Cc:     linux-i2c@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, mst@...hat.com, wsa@...nel.org,
        jasowang@...hat.com, wsa+renesas@...g-engineering.com,
        andriy.shevchenko@...ux.intel.com, conghui.chen@...el.com,
        arnd@...db.de, kblaiech@...lanox.com,
        jarkko.nikula@...ux.intel.com, Sergey.Semin@...kalelectronics.ru,
        rppt@...nel.org, loic.poulain@...aro.org, tali.perry1@...il.com,
        u.kleine-koenig@...gutronix.de, bjorn.andersson@...aro.org,
        yu1.wang@...el.com, shuo.a.liu@...el.com, stefanha@...hat.com,
        pbonzini@...hat.com,
        Alex Bennée <alex.bennee@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH v6] i2c: virtio: add a virtio i2c frontend driver

On 05-03-21, 15:00, Jie Deng wrote:
> On 2021/3/5 11:09, Viresh Kumar wrote:
> > On 05-03-21, 09:46, Jie Deng wrote:
> > > On 2021/3/4 14:06, Viresh Kumar wrote:
> > > > > +	mutex_lock(&vi->i2c_lock);
> > > > I have never worked with i2c stuff earlier, but I don't think you need a lock
> > > > here. The transactions seem to be serialized by the i2c-core by itself (look at
> > > > i2c_transfer() in i2c-core-base.c), though there is another exported version
> > > > __i2c_transfer() but the comment over it says the callers must take adapter lock
> > > > before calling it.
> > > Lock is needed since no "lock_ops" is registered in this i2c_adapter.
> > drivers/i2c/i2c-core-base.c:
> > 
> > static int i2c_register_adapter(struct i2c_adapter *adap)
> > {
> >          ...
> > 
> >          if (!adap->lock_ops)
> >                  adap->lock_ops = &i2c_adapter_lock_ops;
> > 
> >          ...
> > }
> > 
> > This should take care of it ?
> 
> 
> The problem is that you can't guarantee that adap->algo->master_xfer is only
> called
> from i2c_transfer. Any function who holds the adapter can call
> adap->algo->master_xfer
> directly. So I think it is safer to have a lock in virtio_i2c_xfer.

So I tried to look for such callers in the kernel.

$ git grep -l "\<master_xfer(" 
Documentation/i2c/dev-interface.rst
drivers/gpu/drm/gma500/intel_gmbus.c
drivers/gpu/drm/gma500/psb_intel_sdvo.c
drivers/gpu/drm/i915/display/intel_gmbus.c
drivers/gpu/drm/i915/display/intel_sdvo.c
drivers/i2c/busses/i2c-iop3xx.c
drivers/i2c/i2c-core-base.c
drivers/media/usb/dvb-usb/dw2102.c
drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
drivers/mfd/88pm860x-i2c.c
include/uapi/linux/i2c.h

Out of these only one caller is not registering the adapter itself.

drivers/mfd/88pm860x-i2c.c

I was expecting everyone to call the generic functions provided by the i2c core,
not sure why this ended up calling the master_xfer stuff directly.

So this should be general practice to go via i2c core I believe, unless I am
missing something here.

Wolfram, can you please clarify if locking is required here or not ?

> > > > > +static struct i2c_adapter virtio_adapter = {
> > > > > +	.owner = THIS_MODULE,
> > > > > +	.name = "Virtio I2C Adapter",
> > > > > +	.class = I2C_CLASS_DEPRECATED,
> > > > Why are we using something that is deprecated here ?
> > > Warn users that the adapter doesn't support classes anymore.
> > So this is the right thing to do? Or this is what we expect from new drivers?
> > Sorry, I am just new to this stuff and so...
> 
> 
> https://patchwork.ozlabs.org/project/linux-i2c/patch/20170729121143.3980-1-wsa@the-dreams.de/

Frankly this confused me even further :)

The earlier comment in the code said:
        "/* Warn users that adapter will stop using classes */"

so this looks more for existing drivers..

Then the commit message says this:

        "Hopefully making clear that it is not needed for new drivers."

and comment says:

        "/* Warn users that the adapter doesn't support classes anymore */"

Reading this it looks this is only required for existing adapters so they can
warn userspace and shouldn't be required for new drivers.

Am I reading it incorrectly ?

-- 
viresh