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);
}
object_initialize_child(obj, "ehci", &s->ehci, TYPE_NPCM7XX_EHCI);
--
2.33.0.1079.g6e70778dc9-goog
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.
© 2016 - 2026 Red Hat, Inc.