| Message ID | 20181115192446.17187-1-minyard@acm.org |
|---|---|
| Headers | show |
| Series | RFC: Fix/add vmstate handling in some I2C code | expand |
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 have these queued for the SMBus IPMI driver work, of course. > > I had submitted this before and then lost track of the work since I > started finding all kinds of broken things in the I2C code. I > have fixed the broken things I found first, and then added the > previous patches. > > I have tested this in q35 and it works without issue. On piix4 the > pm_smbus code is broken on a migration, however. The device disappears > from the PCI bus on a migration, from what I can tell. It's not the > fault of this code, something more fundamental is going on. The > following comment in piix4.c may have something to do with it: > > /* qemu-kvm 1.2 uses version 3 but advertised as 2 > * To support incoming qemu-kvm 1.2 migration, change version_id > * and minimum_version_id to 2 below (which breaks migration from > * qemu 1.2). > > Anyway, I need to chase that down. > > 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 Thanks, Phil.
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 have these queued for the SMBus IPMI driver work, of course. >> >> I had submitted this before and then lost track of the work since I >> started finding all kinds of broken things in the I2C code. I >> have fixed the broken things I found first, and then added the >> previous patches. >> >> I have tested this in q35 and it works without issue. On piix4 the >> pm_smbus code is broken on a migration, however. The device disappears >> from the PCI bus on a migration, from what I can tell. It's not the >> fault of this code, something more fundamental is going on. The >> following comment in piix4.c may have something to do with it: >> >> /* qemu-kvm 1.2 uses version 3 but advertised as 2 >> * To support incoming qemu-kvm 1.2 migration, change version_id >> * and minimum_version_id to 2 below (which breaks migration from >> * qemu 1.2). >> >> Anyway, I need to chase that down. >> >> 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 > Ok, but who should the maintainer be? I guess I can take it, if that is what you are implying. But most of the files in i2c are not maintained. Thanks, -corey
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. * 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.
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. > >