[PATCH 0/2] tosa: fix Coverity CID 1421929

[PATCH 0/2] tosa: fix Coverity CID 1421929

[Peter Maydell]

This series fixes Coverity issue CID 1421929, which pointed out that
the 'outsignals' in tosa_gpio_setup() were leaked, in the same way
that the equivalent bug for spitz was fixed in the series I posted
earlier: instead of using qemu_allocate_irqs() we create a device
to encapsulate the handling of the misc GPIO lines on this board.
This is simpler than the spitz case because for tosa we don't need
to work with any of the other devices on the board, so the misc-gpio
device can be simple and self-contained.

As with spitz, I started out by tidying up some stray hard-tabs.

thanks
-- PMM

Peter Maydell (2):
  hw/arm/tosa.c: Detabify
  hw/arm/tosa: Encapsulate misc GPIO handling in a device

 hw/arm/tosa.c | 132 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 46 deletions(-)

-- 
2.20.1

[PATCH 1/2] hw/arm/tosa.c: Detabify

[Peter Maydell]

Remove the hardcoded tabs from hw/arm/tosa.c. There aren't
many, but since they're all in constant #defines they're not
going to go away with our usual "only when we touch a function"
policy on reformatting.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/tosa.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
index 5dee2d76c61..06ecf1e7824 100644
--- a/hw/arm/tosa.c
+++ b/hw/arm/tosa.c
@@ -26,32 +26,32 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 
-#define TOSA_RAM    0x04000000
-#define TOSA_ROM	0x00800000
+#define TOSA_RAM 0x04000000
+#define TOSA_ROM 0x00800000
 
-#define TOSA_GPIO_USB_IN		(5)
-#define TOSA_GPIO_nSD_DETECT	(9)
-#define TOSA_GPIO_ON_RESET		(19)
-#define TOSA_GPIO_CF_IRQ		(21)	/* CF slot0 Ready */
-#define TOSA_GPIO_CF_CD			(13)
-#define TOSA_GPIO_TC6393XB_INT  (15)
-#define TOSA_GPIO_JC_CF_IRQ		(36)	/* CF slot1 Ready */
+#define TOSA_GPIO_USB_IN                (5)
+#define TOSA_GPIO_nSD_DETECT            (9)
+#define TOSA_GPIO_ON_RESET              (19)
+#define TOSA_GPIO_CF_IRQ                (21)    /* CF slot0 Ready */
+#define TOSA_GPIO_CF_CD                 (13)
+#define TOSA_GPIO_TC6393XB_INT          (15)
+#define TOSA_GPIO_JC_CF_IRQ             (36)    /* CF slot1 Ready */
 
-#define TOSA_SCOOP_GPIO_BASE	1
-#define TOSA_GPIO_IR_POWERDWN	(TOSA_SCOOP_GPIO_BASE + 2)
-#define TOSA_GPIO_SD_WP			(TOSA_SCOOP_GPIO_BASE + 3)
-#define TOSA_GPIO_PWR_ON		(TOSA_SCOOP_GPIO_BASE + 4)
+#define TOSA_SCOOP_GPIO_BASE            1
+#define TOSA_GPIO_IR_POWERDWN           (TOSA_SCOOP_GPIO_BASE + 2)
+#define TOSA_GPIO_SD_WP                 (TOSA_SCOOP_GPIO_BASE + 3)
+#define TOSA_GPIO_PWR_ON                (TOSA_SCOOP_GPIO_BASE + 4)
 
-#define TOSA_SCOOP_JC_GPIO_BASE		1
-#define TOSA_GPIO_BT_LED		(TOSA_SCOOP_JC_GPIO_BASE + 0)
-#define TOSA_GPIO_NOTE_LED		(TOSA_SCOOP_JC_GPIO_BASE + 1)
-#define TOSA_GPIO_CHRG_ERR_LED		(TOSA_SCOOP_JC_GPIO_BASE + 2)
-#define TOSA_GPIO_TC6393XB_L3V_ON	(TOSA_SCOOP_JC_GPIO_BASE + 5)
-#define TOSA_GPIO_WLAN_LED		(TOSA_SCOOP_JC_GPIO_BASE + 7)
+#define TOSA_SCOOP_JC_GPIO_BASE         1
+#define TOSA_GPIO_BT_LED                (TOSA_SCOOP_JC_GPIO_BASE + 0)
+#define TOSA_GPIO_NOTE_LED              (TOSA_SCOOP_JC_GPIO_BASE + 1)
+#define TOSA_GPIO_CHRG_ERR_LED          (TOSA_SCOOP_JC_GPIO_BASE + 2)
+#define TOSA_GPIO_TC6393XB_L3V_ON       (TOSA_SCOOP_JC_GPIO_BASE + 5)
+#define TOSA_GPIO_WLAN_LED              (TOSA_SCOOP_JC_GPIO_BASE + 7)
 
-#define	DAC_BASE	0x4e
-#define DAC_CH1		0
-#define DAC_CH2		1
+#define DAC_BASE 0x4e
+#define DAC_CH1 0
+#define DAC_CH2 1
 
 static void tosa_microdrive_attach(PXA2xxState *cpu)
 {
-- 
2.20.1

[PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Peter Maydell]

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;
+
+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)
-- 
2.20.1

Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

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)
> 

Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Peter Maydell]

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

Re: [PATCH 1/2] hw/arm/tosa.c: Detabify

[Philippe Mathieu-Daudé]

On 6/28/20 10:37 PM, Peter Maydell wrote:
> Remove the hardcoded tabs from hw/arm/tosa.c. There aren't
> many, but since they're all in constant #defines they're not
> going to go away with our usual "only when we touch a function"
> policy on reformatting.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

No change using 'git-diff -w'.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/tosa.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/arm/tosa.c b/hw/arm/tosa.c
> index 5dee2d76c61..06ecf1e7824 100644
> --- a/hw/arm/tosa.c
> +++ b/hw/arm/tosa.c
> @@ -26,32 +26,32 @@
>  #include "hw/sysbus.h"
>  #include "exec/address-spaces.h"
>  
> -#define TOSA_RAM    0x04000000
> -#define TOSA_ROM	0x00800000
> +#define TOSA_RAM 0x04000000
> +#define TOSA_ROM 0x00800000
>  
> -#define TOSA_GPIO_USB_IN		(5)
> -#define TOSA_GPIO_nSD_DETECT	(9)
> -#define TOSA_GPIO_ON_RESET		(19)
> -#define TOSA_GPIO_CF_IRQ		(21)	/* CF slot0 Ready */
> -#define TOSA_GPIO_CF_CD			(13)
> -#define TOSA_GPIO_TC6393XB_INT  (15)
> -#define TOSA_GPIO_JC_CF_IRQ		(36)	/* CF slot1 Ready */
> +#define TOSA_GPIO_USB_IN                (5)
> +#define TOSA_GPIO_nSD_DETECT            (9)
> +#define TOSA_GPIO_ON_RESET              (19)
> +#define TOSA_GPIO_CF_IRQ                (21)    /* CF slot0 Ready */
> +#define TOSA_GPIO_CF_CD                 (13)
> +#define TOSA_GPIO_TC6393XB_INT          (15)
> +#define TOSA_GPIO_JC_CF_IRQ             (36)    /* CF slot1 Ready */
>  
> -#define TOSA_SCOOP_GPIO_BASE	1
> -#define TOSA_GPIO_IR_POWERDWN	(TOSA_SCOOP_GPIO_BASE + 2)
> -#define TOSA_GPIO_SD_WP			(TOSA_SCOOP_GPIO_BASE + 3)
> -#define TOSA_GPIO_PWR_ON		(TOSA_SCOOP_GPIO_BASE + 4)
> +#define TOSA_SCOOP_GPIO_BASE            1
> +#define TOSA_GPIO_IR_POWERDWN           (TOSA_SCOOP_GPIO_BASE + 2)
> +#define TOSA_GPIO_SD_WP                 (TOSA_SCOOP_GPIO_BASE + 3)
> +#define TOSA_GPIO_PWR_ON                (TOSA_SCOOP_GPIO_BASE + 4)
>  
> -#define TOSA_SCOOP_JC_GPIO_BASE		1
> -#define TOSA_GPIO_BT_LED		(TOSA_SCOOP_JC_GPIO_BASE + 0)
> -#define TOSA_GPIO_NOTE_LED		(TOSA_SCOOP_JC_GPIO_BASE + 1)
> -#define TOSA_GPIO_CHRG_ERR_LED		(TOSA_SCOOP_JC_GPIO_BASE + 2)
> -#define TOSA_GPIO_TC6393XB_L3V_ON	(TOSA_SCOOP_JC_GPIO_BASE + 5)
> -#define TOSA_GPIO_WLAN_LED		(TOSA_SCOOP_JC_GPIO_BASE + 7)
> +#define TOSA_SCOOP_JC_GPIO_BASE         1
> +#define TOSA_GPIO_BT_LED                (TOSA_SCOOP_JC_GPIO_BASE + 0)
> +#define TOSA_GPIO_NOTE_LED              (TOSA_SCOOP_JC_GPIO_BASE + 1)
> +#define TOSA_GPIO_CHRG_ERR_LED          (TOSA_SCOOP_JC_GPIO_BASE + 2)
> +#define TOSA_GPIO_TC6393XB_L3V_ON       (TOSA_SCOOP_JC_GPIO_BASE + 5)
> +#define TOSA_GPIO_WLAN_LED              (TOSA_SCOOP_JC_GPIO_BASE + 7)
>  
> -#define	DAC_BASE	0x4e
> -#define DAC_CH1		0
> -#define DAC_CH2		1
> +#define DAC_BASE 0x4e
> +#define DAC_CH1 0
> +#define DAC_CH2 1
>  
>  static void tosa_microdrive_attach(PXA2xxState *cpu)
>  {
> 

Re: [PATCH 2/2] hw/arm/tosa: Encapsulate misc GPIO handling in a device

[Philippe Mathieu-Daudé]

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>