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

[Viresh Kumar]

On 12-03-21, 15:51, Jie Deng wrote:
> 
> On 2021/3/12 14:10, Viresh Kumar wrote:
> > I saw your email about wrong version being sent, I already wrote some
> > reviews. Sending them anyway for FWIW :)
> > 
> > On 12-03-21, 21:33, Jie Deng wrote:
> > > +struct virtio_i2c {
> > > +	struct virtio_device *vdev;
> > > +	struct completion completion;
> > > +	struct i2c_adapter adap;
> > > +	struct mutex lock;
> > As I said in the previous version (Yes, we were both waiting for
> > Wolfram to answer that), this lock shouldn't be required at all.
> > 
> > And since none of us have a use-case at hand where we will have a
> > problem without this lock, we should really skip it. We can always
> > come back and add it if we find an issue somewhere. Until then, better
> > to keep it simple.

> The problem is 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.

See my last reply here, (almost) no one in the mainline kernel call it
directly. And perhaps you can consider the caller broken in that case
and so there is no need of an extra lock, unless you have a case that
is broken.

https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/

> I prefer to avoid potential issues rather
> than
> find a issue then fix.

This is a very hypothetical issue IMHO as the kernel code doesn't have
such a user. There is no need of locks here, else the i2c core won't
have handled it by itself.

> > 
> > > +
> > > +static struct i2c_adapter virtio_adapter = {
> > > +	.owner = THIS_MODULE,
> > > +	.name = "Virtio I2C Adapter",
> > > +	.class = I2C_CLASS_DEPRECATED,
> > What happened to this discussion ?
> > 
> > https://lore.kernel.org/lkml/20210305072903.wtw645rukmqr5hx5@vireshk-i7/
> 
> My understanding is that new driver sets this to warn users that the adapter
> doesn't support classes anymore.

I think the warning is relevant for old drivers who used to support
classes and not for new ones.

> I'm not sure if Wolfram can explain it more clear for you.

Okay, lemme dig in a bit then.

$ git grep -l i2c_add_adapter drivers/i2c/busses/ | wc -l
77

$ git grep -l I2C_CLASS_DEPRECATED drivers/i2c/busses/
drivers/i2c/busses/i2c-at91-core.c
drivers/i2c/busses/i2c-bcm2835.c
drivers/i2c/busses/i2c-davinci.c
drivers/i2c/busses/i2c-designware-platdrv.c
drivers/i2c/busses/i2c-mv64xxx.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-ocores.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-rcar.c
drivers/i2c/busses/i2c-s3c2410.c
drivers/i2c/busses/i2c-tegra.c
drivers/i2c/busses/i2c-virtio.c
drivers/i2c/busses/i2c-xiic.c

i.e. only 13 of 77 drivers are using this flag.

The latest addition among these drivers is i2c-bcm2835.c and it was
added back in 2013 and the flag was added to it in 2014:

commit 37888f71e2c9 ("i2c: i2c-bcm2835: deprecate class based instantiation")

FWIW, I also checked all the new drivers added since kernel release
v4.0 and none of them set this flag.

-- 
viresh