Re: [Qemu-devel] [PATCH v2] i2c: Fix SMBus read transactions to avoid double events

[Peter Maydell]

On 21 October 2016 at 19:21, Corey Minyard <address@hidden> wrote:
> On 10/21/2016 12:57 PM, Peter Maydell wrote:
>>
>> On 2 August 2016 at 17:00,  <address@hidden> wrote:
>>>
>>> From: Corey Minyard <address@hidden>
>>>
>>> Change 2293c27faddf (i2c: implement broadcast write) added broadcast
>>> capability to the I2C bus, but it broke SMBus read transactions.
>>> An SMBus read transaction does two i2c_start_transaction() calls
>>> without an intervening i2c_end_transfer() call.  This will
>>> result in i2c_start_transfer() adding the same device to the
>>> current_devs list twice, and then the ->event() for the same
>>> device gets called twice in the second call to i2c_start_transfer(),
>>> resulting in the smbus code getting confused.

>> Hi. Did this patch get lost, or was the bug fixed in a different
>> way instead?
>
>
> It was lost, I'm probably doing something wrong, I'm not getting
> much traction on getting patches into qemu.

The way our system works (for better or for worse) is that
until the patch is actively picked up by some subsystem
maintainer it's still "owned" by the patch submitter, so
you have to keep prodding people (by sending periodic 'ping'
emails, etc) until that happens. There is no mechanism for
automatically finding patches that got sent and never
applied. This is particularly important for patches like
this that don't obviously belong to any particular
well-maintained subsystem and will otherwise fall through
the gaps :-(

>> I got here because Coverity complains that the
>> i2c_start_transfer() calls in smbus.c don't check their
>> return value. That suggests to me that we'd be better off
>> having a different function (i2c_restart_transfer() ??)
>> for the "I know we already did this once, so don't try to
>> re-determine who to send this to" case, rather than trying to
>> handle both cases in the same function.
>
>
> Perhaps so.  Or maybe i2c_continue_transfer().  That would
> be more clear.  The second operation can't fail, but relying
> on that is frail.

i2c_continue_transfer() sounds like a better name, yes.
Would you like to write a patch that takes that approach?
If you cc me I'll review it and put it through the
target-arm tree.

thanks
-- PMM