Re: [Qemu-devel] [PATCH v2 00/12] RFC: Fix/add vmstate handling in some I2C code

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 26/11/18 15:11, Corey Minyard wrote:
> On 11/15/18 5:01 PM, Philippe Mathieu-Daudé wrote:
>> Hi Corey,
>>
>> On 15/11/18 20:24, minyard@acm.org wrote:
>>> These changes allow SMBus access while doing a state transfer.
>>> Seems like a good idea to me in general.
>>>
>>>
>>>
>>> I'm primarily submitting this to make sure I'm doing the backwards
>>> compatability with .needed correctly.  I'm adding a new field in
>>> the machine class and setting it in the initialization code for
>>> older versions.  David, is this what you wanted?  It will have to
>>> be adjusted for the proper version if/when it really goes in, of
>>> course.  You can see those in the following commits:
>>>    boards.h: Ignore migration for SMBus devices on
>>>    i2c:pm_smbus: Fix state transfer
>>>    i2c: Add vmstate handling to the smbus eeprom
>>> I thought about adding a field to the pm_smbus code to only transfer
>>> if it was accessed, but I'm assuming that most modern OSes will
>>> at least initialized the device based on its presence on the PCI
>>> bus.  So that didn't seem like it would add any value.
>>>
>>> I'm also submitting to see if all the fixes and cleanups look ok.
>>> That's the first 5 commits.
>>
>> $ git diff origin/master --summary
>>  delete mode 100644 hw/i2c/smbus.c
>>  create mode 100644 hw/i2c/smbus_master.c
>>  create mode 100644 hw/i2c/smbus_slave.c
>>  create mode 100644 include/hw/i2c/smbus_eeprom.h
>>  rename include/hw/i2c/{smbus.h => smbus_master.h} (56%)
>>  create mode 100644 include/hw/i2c/smbus_slave.h
>>
>> Can you add the following files in the MAINTAINERS file:
>> - hw/i2c/smbus_master.c
>> - hw/i2c/smbus_slave.c
>> - include/hw/i2c/smbus_eeprom.h
>> - include/hw/i2c/smbus_master.h
>> - include/hw/i2c/smbus_slave.h
> 
> I'm almost ready to re-submit this series, but I'd like to do 3
> things:
> 
>  * Add the proper person as the maintainer.  I can be the
>    maintainer, but I don't want to presume that's what you
>    meant.  No general I2C code has a maintainer at the moment.

Looking at the git history, the i2c logical code seems stable, most of
the recent changes are API improvements.

The devices are mostly split into x86 (old, stable) and arm/ppc, merged
by respective maintainers, except various cleanups/improvements merged
by Paolo's Misc tree, but Paolo recently asked for help to reduce his
workload.

You seem to worry enough about this subsystem to refactor/improve it,
and this series show you have a deep understanding.

I'm not a QEMU maintainer, but if you agree to step in with the I2C
subsystem, it looks natural to me for you to be there.
This doesn't mean you will be alone, I am pretty sure the previous
maintainers merging the previous patches there will help you reviewing.

For the I2C devices, the get_maintainer script already show the
maintainers, so if this isn't a core I2C change, you can review the
patch and let them merge. For example:

  $ ./scripts/get_maintainer.pl -f hw/i2c/versatile_i2c.c
  Peter Maydell <peter.maydell@linaro.org> (maintainer:Versatile PB)
  qemu-arm@nongnu.org (open list:Versatile PB)

  $ ./scripts/get_maintainer.pl -f hw/i2c/smbus_ich9.c
  "Michael S. Tsirkin" <mst@redhat.com> (supporter:PC)
  Marcel Apfelbaum <marcel.apfelbaum@gmail.com> (supporter:PC)

  $ ./scripts/get_maintainer.pl -f hw/i2c/ppc4xx_i2c.c
  David Gibson <david@gibson.dropbear.id.au> (odd fixer:ppc4xx)
  qemu-ppc@nongnu.org (open list:ppc4xx)

Regards,

Phil.

>  * I'd like to get David's comments on the .needed addition, as
>    I mention above.
>  * I need to figure out why piix4 smbus does not work after a
>    migration.  I'll work on that today.
> 
> Thanks,
> 
> -corey
> 
>>
>> Thanks,
>>
>> Phil.
> 
>