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

On 2021/3/12 16:11, Viresh Kumar wrote:

On 12-03-21, 15:51, Jie Deng wrote:

quoted

On 2021/3/12 14:10, Viresh Kumar wrote:

quoted

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:

quoted

+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/ (local)

quoted

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.

I'd like to see Wolfram's opinion.
Is it safe to remove lock in adap->algo->master_xfer ?


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization