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

[Viresh Kumar]

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@xxxxxxxxxxxxx/

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