| Message ID | 20211021183956.920822-8-wuhaotsh@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc NPCM7XX patches | expand |
On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote: > > The ID can be used to indicate SMBus modules when adding > dynamic devices to them. > > Signed-off-by: Hao Wu <wuhaotsh@google.com> > --- > hw/arm/npcm7xx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c > index 2ab0080e0b..72953d65ef 100644 > --- a/hw/arm/npcm7xx.c > +++ b/hw/arm/npcm7xx.c > @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj) > for (i = 0; i < ARRAY_SIZE(s->smbus); i++) { > object_initialize_child(obj, "smbus[*]", &s->smbus[i], > TYPE_NPCM7XX_SMBUS); > + DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i); > } This one looks weird to me -- I'm pretty sure we shouldn't be messing about with the DeviceState id string like that. It's supposed to be internal to the QOM/qdev code. -- PMM
I was trying to allow attaching a device using "-device xxx,bus=smbus[0]" Maybe there's a better way to allow that? I guess I can drop this one from the patch set. On Mon, Nov 1, 2021 at 10:33 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote: > > > > The ID can be used to indicate SMBus modules when adding > > dynamic devices to them. > > > > Signed-off-by: Hao Wu <wuhaotsh@google.com> > > --- > > hw/arm/npcm7xx.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c > > index 2ab0080e0b..72953d65ef 100644 > > --- a/hw/arm/npcm7xx.c > > +++ b/hw/arm/npcm7xx.c > > @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj) > > for (i = 0; i < ARRAY_SIZE(s->smbus); i++) { > > object_initialize_child(obj, "smbus[*]", &s->smbus[i], > > TYPE_NPCM7XX_SMBUS); > > + DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i); > > } > > This one looks weird to me -- I'm pretty sure we shouldn't be messing > about with the DeviceState id string like that. It's supposed to be > internal to the QOM/qdev code. > > -- PMM >
+Markus/Eduardo On 11/1/21 18:33, Peter Maydell wrote: > On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote: >> >> The ID can be used to indicate SMBus modules when adding >> dynamic devices to them. >> >> Signed-off-by: Hao Wu <wuhaotsh@google.com> >> --- >> hw/arm/npcm7xx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c >> index 2ab0080e0b..72953d65ef 100644 >> --- a/hw/arm/npcm7xx.c >> +++ b/hw/arm/npcm7xx.c >> @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj) >> for (i = 0; i < ARRAY_SIZE(s->smbus); i++) { >> object_initialize_child(obj, "smbus[*]", &s->smbus[i], >> TYPE_NPCM7XX_SMBUS); >> + DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i); >> } > > This one looks weird to me -- I'm pretty sure we shouldn't be messing > about with the DeviceState id string like that. It's supposed to be > internal to the QOM/qdev code. I agree with you, however: $ git grep -F -- '->id = g_strdup' hw hw/arm/virt.c:1512: dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); hw/block/xen-block.c:761: drive->id = g_strdup(id); hw/block/xen-block.c:853: iothread->id = g_strdup(id); hw/core/machine-qmp-cmds.c:169: m->id = g_strdup(object_get_canonical_path_component(obj)); hw/mem/pc-dimm.c:244: di->id = g_strdup(dev->id); hw/pci-bridge/pci_expander_bridge.c:248: bds->id = g_strdup(dev_name); hw/ppc/e500.c:1009: dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); hw/s390x/s390-pci-bus.c:1003: dev->id = g_strdup_printf("auto_%02x:%02x.%01x", hw/sh4/sh7750.c:819: dev->id = g_strdup("sci"); hw/sh4/sh7750.c:836: dev->id = g_strdup("scif"); hw/virtio/virtio-mem-pci.c:69: vi->id = g_strdup(dev->id); hw/virtio/virtio-pmem-pci.c:74: vi->id = g_strdup(dev->id);
Philippe Mathieu-Daudé <f4bug@amsat.org> writes: > +Markus/Eduardo > > On 11/1/21 18:33, Peter Maydell wrote: >> On Thu, 21 Oct 2021 at 19:40, Hao Wu <wuhaotsh@google.com> wrote: >>> >>> The ID can be used to indicate SMBus modules when adding >>> dynamic devices to them. >>> >>> Signed-off-by: Hao Wu <wuhaotsh@google.com> >>> --- >>> hw/arm/npcm7xx.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c >>> index 2ab0080e0b..72953d65ef 100644 >>> --- a/hw/arm/npcm7xx.c >>> +++ b/hw/arm/npcm7xx.c >>> @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj) >>> for (i = 0; i < ARRAY_SIZE(s->smbus); i++) { >>> object_initialize_child(obj, "smbus[*]", &s->smbus[i], >>> TYPE_NPCM7XX_SMBUS); >>> + DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i); >>> } >> >> This one looks weird to me -- I'm pretty sure we shouldn't be messing >> about with the DeviceState id string like that. It's supposed to be >> internal to the QOM/qdev code. It's meant for the user. Devices created by code shouldn't set it. Not least because any ID they pick could clash with the user's. > I agree with you, however: > > $ git grep -F -- '->id = g_strdup' hw > hw/arm/virt.c:1512: dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); > hw/block/xen-block.c:761: drive->id = g_strdup(id); > hw/block/xen-block.c:853: iothread->id = g_strdup(id); > hw/core/machine-qmp-cmds.c:169: m->id = g_strdup(object_get_canonical_path_component(obj)); > hw/mem/pc-dimm.c:244: di->id = g_strdup(dev->id); > hw/pci-bridge/pci_expander_bridge.c:248: bds->id = g_strdup(dev_name); > hw/ppc/e500.c:1009: dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); > hw/s390x/s390-pci-bus.c:1003: dev->id = g_strdup_printf("auto_%02x:%02x.%01x", > hw/sh4/sh7750.c:819: dev->id = g_strdup("sci"); > hw/sh4/sh7750.c:836: dev->id = g_strdup("scif"); > hw/virtio/virtio-mem-pci.c:69: vi->id = g_strdup(dev->id); > hw/virtio/virtio-pmem-pci.c:74: vi->id = g_strdup(dev->id); This includes false positives, i.e. assignments to members of structs other than DeviceState. It also misses other ways to mess with DeviceState member id. Compiling with diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 1bad07002d..b17ccd6065 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -176,7 +176,7 @@ struct DeviceState { Object parent_obj; /*< public >*/ - char *id; + char *const id; char *canonical_path; bool realized; bool pending_deleted_event; ferrets out the actual assignments. I got: ../hw/ppc/spapr_vio.c: In function ‘spapr_vio_busdev_realize’: ../hw/ppc/spapr_vio.c:505:22: error: assignment of read-only member ‘id’ 505 | dev->qdev.id = id; | ^ This supplies a default ID to a vio-spapr-device. Feels like a bad idea. ../softmmu/qdev-monitor.c: In function ‘qdev_set_id’: ../softmmu/qdev-monitor.c:593:21: error: assignment of read-only member ‘id’ 593 | dev->id = id; | ^ This assigns the user-supplied ID. ../hw/dma/xlnx-zdma.c: In function ‘zdma_realize’: ../hw/dma/xlnx-zdma.c:777:12: error: assignment of read-only location ‘*r’ 777 | *r = (RegisterInfo) { | ^ This *clobbers* the DeviceState embedded in *r, including its member id. Suspicious. ../hw/pci-bridge/pci_expander_bridge.c: In function ‘pxb_dev_realize_common’: ../hw/pci-bridge/pci_expander_bridge.c:248:17: error: assignment of read-only member ‘id’ 248 | bds->id = g_strdup(dev_name); | ^ This creates a pci-bridge device and gives it the same ID as the pxb device being realized. Doesn't this result in duplicate IDs?!? ../hw/arm/virt.c: In function ‘create_platform_bus’: ../hw/arm/virt.c:1512:13: error: assignment of read-only member ‘id’ 1512 | dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); | ^ Helper function to create a platform-bus-device. ID is set to "platform-bus-device". Feels like a bad idea. ../hw/ppc/e500.c: In function ‘ppce500_init’: ../hw/ppc/e500.c:1009:17: error: assignment of read-only member ‘id’ 1009 | dev->id = g_strdup(TYPE_PLATFORM_BUS_DEVICE); | ^ Likewise. ../hw/s390x/s390-pci-bus.c: In function ‘s390_pcihost_plug’: ../hw/s390x/s390-pci-bus.c:1003:21: error: assignment of read-only member ‘id’ 1003 | dev->id = g_strdup_printf("auto_%02x:%02x.%01x", | ^ This supplies a default ID to a the device being plugged (I think). Feels like a bad idea. ../hw/sh4/sh7750.c: In function ‘sh7750_init’: ../hw/sh4/sh7750.c:819:13: error: assignment of read-only member ‘id’ 819 | dev->id = g_strdup("sci"); | ^ ../hw/sh4/sh7750.c:836:13: error: assignment of read-only member ‘id’ 836 | dev->id = g_strdup("scif"); | ^ These create sh-serial devices. Their IDs are set to "sci" and "scif", respectively. Feels like a bad idea.
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c index 2ab0080e0b..72953d65ef 100644 --- a/hw/arm/npcm7xx.c +++ b/hw/arm/npcm7xx.c @@ -421,6 +421,7 @@ static void npcm7xx_init(Object *obj) for (i = 0; i < ARRAY_SIZE(s->smbus); i++) { object_initialize_child(obj, "smbus[*]", &s->smbus[i], TYPE_NPCM7XX_SMBUS); + DEVICE(&s->smbus[i])->id = g_strdup_printf("smbus[%d]", i); } object_initialize_child(obj, "ehci", &s->ehci, TYPE_NPCM7XX_EHCI);
The ID can be used to indicate SMBus modules when adding dynamic devices to them. Signed-off-by: Hao Wu <wuhaotsh@google.com> --- hw/arm/npcm7xx.c | 1 + 1 file changed, 1 insertion(+)