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
