| Message ID | 20200628203748.14250-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tosa: fix Coverity CID 1421929 | expand |
Hi Peter, On 6/28/20 10:37 PM, Peter Maydell wrote: > Currently we have a free-floating set of IRQs and a function > tosa_out_switch() which handle the GPIO lines on the tosa board which > connect to LEDs, and another free-floating IRQ and tosa_reset() > function to handle the GPIO line that resets the system. Encapsulate > this behaviour in a simple QOM device. > > This commit fixes Coverity issue CID 1421929 (which pointed out that > the 'outsignals' in tosa_gpio_setup() were leaked), because it > removes the use of the qemu_allocate_irqs() API from this code > entirely. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This is simpler than the spitz changes because the new device > doesn't need to refer to any of the other devices on the board. > --- > hw/arm/tosa.c | 88 +++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 64 insertions(+), 24 deletions(-) > > diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c > index 06ecf1e7824..383b3b22e24 100644 > --- a/hw/arm/tosa.c > +++ b/hw/arm/tosa.c > @@ -65,24 +65,39 @@ static void tosa_microdrive_attach(PXA2xxState *cpu) > pxa2xx_pcmcia_attach(cpu->pcmcia[0], md); > } > > -static void tosa_out_switch(void *opaque, int line, int level) > +/* > + * Encapsulation of some GPIO line behaviour for the Tosa board > + * > + * QEMU interface: > + * + named GPIO inputs "leds[0..3]": assert to light LEDs > + * + named GPIO input "reset": when asserted, resets the system > + */ > + > +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio" > +#define TOSA_MISC_GPIO(obj) \ > + OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO) > + > +typedef struct TosaMiscGPIOState { > + SysBusDevice parent_obj; > +} TosaMiscGPIOState; Since we don't really use this type, can we avoid declaring it? Like: #define TOSA_MISC_GPIO(obj) \ OBJECT_CHECK(SysBusDevice, (obj), TYPE_TOSA_MISC_GPIO) And in tosa_misc_gpio_info: .instance_size = sizeof(SysBusDevice) > + > +static void tosa_gpio_leds(void *opaque, int line, int level) > { > switch (line) { > - case 0: > - fprintf(stderr, "blue LED %s.\n", level ? "on" : "off"); > - break; > - case 1: > - fprintf(stderr, "green LED %s.\n", level ? "on" : "off"); > - break; > - case 2: > - fprintf(stderr, "amber LED %s.\n", level ? "on" : "off"); > - break; > - case 3: > - fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off"); > - break; > - default: > - fprintf(stderr, "Uhandled out event: %d = %d\n", line, level); > - break; > + case 0: > + fprintf(stderr, "blue LED %s.\n", level ? "on" : "off"); > + break; > + case 1: > + fprintf(stderr, "green LED %s.\n", level ? "on" : "off"); > + break; > + case 2: > + fprintf(stderr, "amber LED %s.\n", level ? "on" : "off"); > + break; > + case 3: > + fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off"); > + break; Nitpicking, the indentation change might go in the previous patch. > + default: > + g_assert_not_reached(); > } > } > > @@ -93,13 +108,22 @@ static void tosa_reset(void *opaque, int line, int level) > } > } > > +static void tosa_misc_gpio_init(Object *obj) > +{ > + DeviceState *dev = DEVICE(obj); > + Ah, MachineClass does not inherit from DeviceClass, so we can use it to create GPIOs. Something is bugging me here, similar with the LEDs series I sent recently. GPIOs are not specific to a bus. I see ResettableClass takes Object arguments. We should be able to wire GPIO lines to generic Objects like LEDs. Parents don't have to be qdev. Actually looking at qdev_init_gpio_in_named_with_opaque(), the function only accesses the QOM API, not the QDEV one. The only field stored in the state is the gpio list: struct DeviceState { ... QLIST_HEAD(, NamedGPIOList) gpios; Having to create a container to wire GPIOs or hold a reference to a MemoryRegion sounds wrong. If the MachineState can not do that, can we create a generic BoardState (like PCB to route signals) so all machines can use it? > + qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4); > + qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1); > +} > + > static void tosa_gpio_setup(PXA2xxState *cpu, > DeviceState *scp0, > DeviceState *scp1, > TC6393xbState *tmio) > { > - qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4); > - qemu_irq reset; > + DeviceState *misc_gpio; > + > + misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL); > > /* MMC/SD host */ > pxa2xx_mmci_handlers(cpu->mmc, > @@ -107,8 +131,8 @@ static void tosa_gpio_setup(PXA2xxState *cpu, > qemu_irq_invert(qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_nSD_DETECT))); > > /* Handle reset */ > - reset = qemu_allocate_irq(tosa_reset, cpu, 0); > - qdev_connect_gpio_out(cpu->gpio, TOSA_GPIO_ON_RESET, reset); > + qdev_connect_gpio_out(cpu->gpio, TOSA_GPIO_ON_RESET, > + qdev_get_gpio_in_named(misc_gpio, "reset", 0)); > > /* PCMCIA signals: card's IRQ and Card-Detect */ > pxa2xx_pcmcia_set_irq_cb(cpu->pcmcia[0], > @@ -119,10 +143,14 @@ static void tosa_gpio_setup(PXA2xxState *cpu, > qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ), > NULL); > > - qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]); > - qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]); > - qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]); > - qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]); > + qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, > + qdev_get_gpio_in_named(misc_gpio, "leds", 0)); > + qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, > + qdev_get_gpio_in_named(misc_gpio, "leds", 1)); > + qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, > + qdev_get_gpio_in_named(misc_gpio, "leds", 2)); > + qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, > + qdev_get_gpio_in_named(misc_gpio, "leds", 3)); > > qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio)); > > @@ -287,10 +315,22 @@ static const TypeInfo tosa_ssp_info = { > .class_init = tosa_ssp_class_init, > }; > > +static const TypeInfo tosa_misc_gpio_info = { > + .name = "tosa-misc-gpio", > + .parent = TYPE_SYS_BUS_DEVICE, > + .instance_size = sizeof(TosaMiscGPIOState), > + .instance_init = tosa_misc_gpio_init, > + /* > + * No class init required: device has no internal state so does not > + * need to set up reset or vmstate, and has no realize method. > + */ > +}; > + > static void tosa_register_types(void) > { > type_register_static(&tosa_dac_info); > type_register_static(&tosa_ssp_info); > + type_register_static(&tosa_misc_gpio_info); > } > > type_init(tosa_register_types) >
On Mon, 29 Jun 2020 at 10:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Hi Peter, > > On 6/28/20 10:37 PM, Peter Maydell wrote: > > Currently we have a free-floating set of IRQs and a function > > tosa_out_switch() which handle the GPIO lines on the tosa board which > > connect to LEDs, and another free-floating IRQ and tosa_reset() > > function to handle the GPIO line that resets the system. Encapsulate > > this behaviour in a simple QOM device. > > > > This commit fixes Coverity issue CID 1421929 (which pointed out that > > the 'outsignals' in tosa_gpio_setup() were leaked), because it > > removes the use of the qemu_allocate_irqs() API from this code > > entirely. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > This is simpler than the spitz changes because the new device > > doesn't need to refer to any of the other devices on the board. > > --- > > +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio" > > +#define TOSA_MISC_GPIO(obj) \ > > + OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO) > > + > > +typedef struct TosaMiscGPIOState { > > + SysBusDevice parent_obj; > > +} TosaMiscGPIOState; > > Since we don't really use this type, can we avoid declaring it? I prefer to be consistent. QOM's implementation allows this kind of shortcut where you don't provide everything that a typical leaf class provides if you don't need all of it, but then it gets confusing if later on somebody wants to add something to that leaf class. I think it's less confusing to have just two standard patterns: * leaf class, no subclasses * a class that will be subclassed and then always provide the same set of TYPE_*, cast macros, structs, etc for whichever of the patterns you're following, even if it happens that these aren't all needed. (https://wiki.qemu.org/Documentation/QOMConventions does a reasonable job of saying what the standard pattern is for the subclassed-class case, but is a bit less clear on the leaf-class case.) > > +static void tosa_misc_gpio_init(Object *obj) > > +{ > > + DeviceState *dev = DEVICE(obj); > > + > > Ah, MachineClass does not inherit from DeviceClass, so we can use > it to create GPIOs. > > Something is bugging me here, similar with the LEDs series I sent > recently. > > GPIOs are not specific to a bus. I see ResettableClass takes Object > arguments. > > We should be able to wire GPIO lines to generic Objects like LEDs. > Parents don't have to be qdev. Yes, this is awkward. You pretty much have to inherit from SysBusDevice assuming you want to get reset (the few cases we have which directly inherit from Device like CPUs then end up needing special handling). > Having to create a container to wire GPIOs or hold a reference > to a MemoryRegion sounds wrong. I agree that it would be nicer if MachineState inherited from Device (and if Device got reset properly). But that's a huge amount of rework. For this series I'm just trying to improve the setup for the spitz board, and "logic that's more than just wiring up devices to each other needs to live inside some device, and can't be in the board itself" is the system we have today. thanks -- PMM
On 6/29/20 2:14 PM, Peter Maydell wrote: > On Mon, 29 Jun 2020 at 10:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> Hi Peter, >> >> On 6/28/20 10:37 PM, Peter Maydell wrote: >>> Currently we have a free-floating set of IRQs and a function >>> tosa_out_switch() which handle the GPIO lines on the tosa board which >>> connect to LEDs, and another free-floating IRQ and tosa_reset() >>> function to handle the GPIO line that resets the system. Encapsulate >>> this behaviour in a simple QOM device. >>> >>> This commit fixes Coverity issue CID 1421929 (which pointed out that >>> the 'outsignals' in tosa_gpio_setup() were leaked), because it >>> removes the use of the qemu_allocate_irqs() API from this code >>> entirely. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> This is simpler than the spitz changes because the new device >>> doesn't need to refer to any of the other devices on the board. >>> --- > >>> +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio" >>> +#define TOSA_MISC_GPIO(obj) \ >>> + OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO) >>> + >>> +typedef struct TosaMiscGPIOState { >>> + SysBusDevice parent_obj; >>> +} TosaMiscGPIOState; >> >> Since we don't really use this type, can we avoid declaring it? > > I prefer to be consistent. QOM's implementation allows this kind > of shortcut where you don't provide everything that a typical > leaf class provides if you don't need all of it, but then it > gets confusing if later on somebody wants to add something > to that leaf class. I think it's less confusing to have > just two standard patterns: > * leaf class, no subclasses > * a class that will be subclassed > and then always provide the same set of TYPE_*, cast macros, > structs, etc for whichever of the patterns you're following, > even if it happens that these aren't all needed. Understood. > (https://wiki.qemu.org/Documentation/QOMConventions > does a reasonable job of saying what the standard pattern > is for the subclassed-class case, but is a bit less clear > on the leaf-class case.) > > >>> +static void tosa_misc_gpio_init(Object *obj) >>> +{ >>> + DeviceState *dev = DEVICE(obj); >>> + >> >> Ah, MachineClass does not inherit from DeviceClass, so we can use >> it to create GPIOs. >> >> Something is bugging me here, similar with the LEDs series I sent >> recently. >> >> GPIOs are not specific to a bus. I see ResettableClass takes Object >> arguments. >> >> We should be able to wire GPIO lines to generic Objects like LEDs. >> Parents don't have to be qdev. > > Yes, this is awkward. You pretty much have to inherit from > SysBusDevice assuming you want to get reset (the few cases > we have which directly inherit from Device like CPUs then > end up needing special handling). > >> Having to create a container to wire GPIOs or hold a reference >> to a MemoryRegion sounds wrong. > > I agree that it would be nicer if MachineState inherited from > Device (and if Device got reset properly). But that's a huge > amount of rework. For this series I'm just trying to improve > the setup for the spitz board, and "logic that's more than > just wiring up devices to each other needs to live inside some > device, and can't be in the board itself" is the system we have today. We have a large room for improvement :) Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c index 06ecf1e7824..383b3b22e24 100644 --- a/hw/arm/tosa.c +++ b/hw/arm/tosa.c @@ -65,24 +65,39 @@ static void tosa_microdrive_attach(PXA2xxState *cpu) pxa2xx_pcmcia_attach(cpu->pcmcia[0], md); } -static void tosa_out_switch(void *opaque, int line, int level) +/* + * Encapsulation of some GPIO line behaviour for the Tosa board + * + * QEMU interface: + * + named GPIO inputs "leds[0..3]": assert to light LEDs + * + named GPIO input "reset": when asserted, resets the system + */ + +#define TYPE_TOSA_MISC_GPIO "tosa-misc-gpio" +#define TOSA_MISC_GPIO(obj) \ + OBJECT_CHECK(TosaMiscGPIOState, (obj), TYPE_TOSA_MISC_GPIO) + +typedef struct TosaMiscGPIOState { + SysBusDevice parent_obj; +} TosaMiscGPIOState; + +static void tosa_gpio_leds(void *opaque, int line, int level) { switch (line) { - case 0: - fprintf(stderr, "blue LED %s.\n", level ? "on" : "off"); - break; - case 1: - fprintf(stderr, "green LED %s.\n", level ? "on" : "off"); - break; - case 2: - fprintf(stderr, "amber LED %s.\n", level ? "on" : "off"); - break; - case 3: - fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off"); - break; - default: - fprintf(stderr, "Uhandled out event: %d = %d\n", line, level); - break; + case 0: + fprintf(stderr, "blue LED %s.\n", level ? "on" : "off"); + break; + case 1: + fprintf(stderr, "green LED %s.\n", level ? "on" : "off"); + break; + case 2: + fprintf(stderr, "amber LED %s.\n", level ? "on" : "off"); + break; + case 3: + fprintf(stderr, "wlan LED %s.\n", level ? "on" : "off"); + break; + default: + g_assert_not_reached(); } } @@ -93,13 +108,22 @@ static void tosa_reset(void *opaque, int line, int level) } } +static void tosa_misc_gpio_init(Object *obj) +{ + DeviceState *dev = DEVICE(obj); + + qdev_init_gpio_in_named(dev, tosa_gpio_leds, "leds", 4); + qdev_init_gpio_in_named(dev, tosa_reset, "reset", 1); +} + static void tosa_gpio_setup(PXA2xxState *cpu, DeviceState *scp0, DeviceState *scp1, TC6393xbState *tmio) { - qemu_irq *outsignals = qemu_allocate_irqs(tosa_out_switch, cpu, 4); - qemu_irq reset; + DeviceState *misc_gpio; + + misc_gpio = sysbus_create_simple(TYPE_TOSA_MISC_GPIO, -1, NULL); /* MMC/SD host */ pxa2xx_mmci_handlers(cpu->mmc, @@ -107,8 +131,8 @@ static void tosa_gpio_setup(PXA2xxState *cpu, qemu_irq_invert(qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_nSD_DETECT))); /* Handle reset */ - reset = qemu_allocate_irq(tosa_reset, cpu, 0); - qdev_connect_gpio_out(cpu->gpio, TOSA_GPIO_ON_RESET, reset); + qdev_connect_gpio_out(cpu->gpio, TOSA_GPIO_ON_RESET, + qdev_get_gpio_in_named(misc_gpio, "reset", 0)); /* PCMCIA signals: card's IRQ and Card-Detect */ pxa2xx_pcmcia_set_irq_cb(cpu->pcmcia[0], @@ -119,10 +143,14 @@ static void tosa_gpio_setup(PXA2xxState *cpu, qdev_get_gpio_in(cpu->gpio, TOSA_GPIO_JC_CF_IRQ), NULL); - qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, outsignals[0]); - qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, outsignals[1]); - qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, outsignals[2]); - qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, outsignals[3]); + qdev_connect_gpio_out(scp1, TOSA_GPIO_BT_LED, + qdev_get_gpio_in_named(misc_gpio, "leds", 0)); + qdev_connect_gpio_out(scp1, TOSA_GPIO_NOTE_LED, + qdev_get_gpio_in_named(misc_gpio, "leds", 1)); + qdev_connect_gpio_out(scp1, TOSA_GPIO_CHRG_ERR_LED, + qdev_get_gpio_in_named(misc_gpio, "leds", 2)); + qdev_connect_gpio_out(scp1, TOSA_GPIO_WLAN_LED, + qdev_get_gpio_in_named(misc_gpio, "leds", 3)); qdev_connect_gpio_out(scp1, TOSA_GPIO_TC6393XB_L3V_ON, tc6393xb_l3v_get(tmio)); @@ -287,10 +315,22 @@ static const TypeInfo tosa_ssp_info = { .class_init = tosa_ssp_class_init, }; +static const TypeInfo tosa_misc_gpio_info = { + .name = "tosa-misc-gpio", + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(TosaMiscGPIOState), + .instance_init = tosa_misc_gpio_init, + /* + * No class init required: device has no internal state so does not + * need to set up reset or vmstate, and has no realize method. + */ +}; + static void tosa_register_types(void) { type_register_static(&tosa_dac_info); type_register_static(&tosa_ssp_info); + type_register_static(&tosa_misc_gpio_info); } type_init(tosa_register_types)
Currently we have a free-floating set of IRQs and a function tosa_out_switch() which handle the GPIO lines on the tosa board which connect to LEDs, and another free-floating IRQ and tosa_reset() function to handle the GPIO line that resets the system. Encapsulate this behaviour in a simple QOM device. This commit fixes Coverity issue CID 1421929 (which pointed out that the 'outsignals' in tosa_gpio_setup() were leaked), because it removes the use of the qemu_allocate_irqs() API from this code entirely. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This is simpler than the spitz changes because the new device doesn't need to refer to any of the other devices on the board. --- hw/arm/tosa.c | 88 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 24 deletions(-) -- 2.20.1