Re: [Qemu-arm] [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct

[Philippe Mathieu-Daudé]

On 7/2/19 6:38 PM, Peter Maydell wrote:
> Currently the bitbang_i2c_init() function allocates a
> bitbang_i2c_interface struct which it returns.  This is unfortunate
> because it means that if the function is used from a DeviceState
> init method then the memory will be leaked by an "init then delete"
> cycle, as used by the qmp/hmp commands that list device properties.
> 
> Since three out of four of the uses of this function are in
> device init methods, switch the function to do an in-place
> initialization of a struct that can be embedded in the
> device state struct of the caller.
> 
> This fixes LeakSanitizer leak warnings that have appeared in the
> patchew configuration (which only tries to run the sanitizers
> for the x86_64-softmmu target) now that we use the bitbang-i2c
> code in an x86-64 config.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This isn't the only problem with this code : it is also
> missing reset and migration handling and generally looks like
> it needs proper conversion to QOM somehow. But this will shut
> the patchew complaints up and seems ok for 4.1.
> 
> Disclaimer: checked only that the leak-sanitizer is now happy
> and with a 'make check'.
> ---
>  hw/display/ati_int.h         |  2 +-
>  include/hw/i2c/bitbang_i2c.h | 38 ++++++++++++++++++++++++++++-
>  include/hw/i2c/ppc4xx_i2c.h  |  2 +-
>  hw/display/ati.c             |  7 +++---
>  hw/i2c/bitbang_i2c.c         | 47 +++---------------------------------
>  hw/i2c/ppc4xx_i2c.c          |  6 ++---
>  hw/i2c/versatile_i2c.c       |  8 +++---
>  7 files changed, 53 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> index 9b67d0022ad..31a1927b3ec 100644
> --- a/hw/display/ati_int.h
> +++ b/hw/display/ati_int.h
> @@ -88,7 +88,7 @@ typedef struct ATIVGAState {
>      uint16_t cursor_size;
>      uint32_t cursor_offset;
>      QEMUCursor *cursor;
> -    bitbang_i2c_interface *bbi2c;
> +    bitbang_i2c_interface bbi2c;
>      MemoryRegion io;
>      MemoryRegion mm;
>      ATIVGARegs regs;
> diff --git a/include/hw/i2c/bitbang_i2c.h b/include/hw/i2c/bitbang_i2c.h
> index 3a7126d5dee..92334e9016a 100644
> --- a/include/hw/i2c/bitbang_i2c.h
> +++ b/include/hw/i2c/bitbang_i2c.h
> @@ -8,7 +8,43 @@ typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>  #define BITBANG_I2C_SDA 0
>  #define BITBANG_I2C_SCL 1
>  
> -bitbang_i2c_interface *bitbang_i2c_init(I2CBus *bus);
> +typedef enum bitbang_i2c_state {
> +    STOPPED = 0,
> +    SENDING_BIT7,
> +    SENDING_BIT6,
> +    SENDING_BIT5,
> +    SENDING_BIT4,
> +    SENDING_BIT3,
> +    SENDING_BIT2,
> +    SENDING_BIT1,
> +    SENDING_BIT0,
> +    WAITING_FOR_ACK,
> +    RECEIVING_BIT7,
> +    RECEIVING_BIT6,
> +    RECEIVING_BIT5,
> +    RECEIVING_BIT4,
> +    RECEIVING_BIT3,
> +    RECEIVING_BIT2,
> +    RECEIVING_BIT1,
> +    RECEIVING_BIT0,
> +    SENDING_ACK,
> +    SENT_NACK
> +} bitbang_i2c_state;
> +
> +struct bitbang_i2c_interface {
> +    I2CBus *bus;
> +    bitbang_i2c_state state;
> +    int last_data;
> +    int last_clock;
> +    int device_out;
> +    uint8_t buffer;
> +    int current_addr;
> +};
> +
> +/**
> + * bitbang_i2c_init: in-place initialize the bitbang_i2c_interface struct
> + */
> +void bitbang_i2c_init(bitbang_i2c_interface *s, I2CBus *bus);
>  int bitbang_i2c_set(bitbang_i2c_interface *i2c, int line, int level);
>  
>  #endif
> diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> index 8437bf070b8..f6f837fbec0 100644
> --- a/include/hw/i2c/ppc4xx_i2c.h
> +++ b/include/hw/i2c/ppc4xx_i2c.h
> @@ -41,7 +41,7 @@ typedef struct PPC4xxI2CState {
>      I2CBus *bus;
>      qemu_irq irq;
>      MemoryRegion iomem;
> -    bitbang_i2c_interface *bitbang;
> +    bitbang_i2c_interface bitbang;
>      int mdidx;
>      uint8_t mdata[4];
>      uint8_t lmadr;
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 0cb11738483..c1d9d1518f4 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -538,7 +538,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>          break;
>      case GPIO_DVI_DDC:
>          if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
> -            s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data, 0);
> +            s->regs.gpio_dvi_ddc = ati_i2c(&s->bbi2c, data, 0);
>          }
>          break;
>      case GPIO_MONID ... GPIO_MONID + 3:
> @@ -554,7 +554,7 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>               */
>              if ((s->regs.gpio_monid & BIT(25)) &&
>                  addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) {
> -                s->regs.gpio_monid = ati_i2c(s->bbi2c, s->regs.gpio_monid, 
> 1);
> +                s->regs.gpio_monid = ati_i2c(&s->bbi2c, s->regs.gpio_monid, 
> 1);
>              }
>          }
>          break;
> @@ -856,7 +856,7 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>  
>      /* ddc, edid */
>      I2CBus *i2cbus = i2c_init_bus(DEVICE(s), "ati-vga.ddc");
> -    s->bbi2c = bitbang_i2c_init(i2cbus);
> +    bitbang_i2c_init(&s->bbi2c, i2cbus);
>      I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
>      i2c_set_slave_address(i2cddc, 0x50);
>  
> @@ -885,7 +885,6 @@ static void ati_vga_exit(PCIDevice *dev)
>      ATIVGAState *s = ATI_VGA(dev);
>  
>      graphic_console_close(s->vga.con);
> -    g_free(s->bbi2c);
>  }
>  
>  static Property ati_vga_properties[] = {
> diff --git a/hw/i2c/bitbang_i2c.c b/hw/i2c/bitbang_i2c.c
> index 3cb0509b020..60c7a9be0bc 100644
> --- a/hw/i2c/bitbang_i2c.c
> +++ b/hw/i2c/bitbang_i2c.c
> @@ -25,39 +25,6 @@ do { printf("bitbang_i2c: " fmt , ## __VA_ARGS__); } while 
> (0)
>  #define DPRINTF(fmt, ...) do {} while(0)
>  #endif
>  
> -typedef enum bitbang_i2c_state {
> -    STOPPED = 0,
> -    SENDING_BIT7,
> -    SENDING_BIT6,
> -    SENDING_BIT5,
> -    SENDING_BIT4,
> -    SENDING_BIT3,
> -    SENDING_BIT2,
> -    SENDING_BIT1,
> -    SENDING_BIT0,
> -    WAITING_FOR_ACK,
> -    RECEIVING_BIT7,
> -    RECEIVING_BIT6,
> -    RECEIVING_BIT5,
> -    RECEIVING_BIT4,
> -    RECEIVING_BIT3,
> -    RECEIVING_BIT2,
> -    RECEIVING_BIT1,
> -    RECEIVING_BIT0,
> -    SENDING_ACK,
> -    SENT_NACK
> -} bitbang_i2c_state;
> -
> -struct bitbang_i2c_interface {
> -    I2CBus *bus;
> -    bitbang_i2c_state state;
> -    int last_data;
> -    int last_clock;
> -    int device_out;
> -    uint8_t buffer;
> -    int current_addr;
> -};
> -
>  static void bitbang_i2c_enter_stop(bitbang_i2c_interface *i2c)
>  {
>      DPRINTF("STOP\n");
> @@ -184,18 +151,12 @@ int bitbang_i2c_set(bitbang_i2c_interface *i2c, int 
> line, int level)
>      abort();
>  }
>  
> -bitbang_i2c_interface *bitbang_i2c_init(I2CBus *bus)
> +void bitbang_i2c_init(bitbang_i2c_interface *s, I2CBus *bus)
>  {
> -    bitbang_i2c_interface *s;
> -
> -    s = g_malloc0(sizeof(bitbang_i2c_interface));
> -
>      s->bus = bus;

So, QOM speaking, bitbang_i2c_init() is a DeviceRealize with a .bus
property. We'll clean that up later, OK.

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>      s->last_data = 1;
>      s->last_clock = 1;
>      s->device_out = 1;
> -
> -    return s;
>  }
>  
>  /* GPIO interface.  */
> @@ -207,7 +168,7 @@ typedef struct GPIOI2CState {
>      SysBusDevice parent_obj;
>  
>      MemoryRegion dummy_iomem;
> -    bitbang_i2c_interface *bitbang;
> +    bitbang_i2c_interface bitbang;
>      int last_level;
>      qemu_irq out;
>  } GPIOI2CState;
> @@ -216,7 +177,7 @@ static void bitbang_i2c_gpio_set(void *opaque, int irq, 
> int level)
>  {
>      GPIOI2CState *s = opaque;
>  
> -    level = bitbang_i2c_set(s->bitbang, irq, level);
> +    level = bitbang_i2c_set(&s->bitbang, irq, level);
>      if (level != s->last_level) {
>          s->last_level = level;
>          qemu_set_irq(s->out, level);
> @@ -234,7 +195,7 @@ static void gpio_i2c_init(Object *obj)
>      sysbus_init_mmio(sbd, &s->dummy_iomem);
>  
>      bus = i2c_init_bus(dev, "i2c");
> -    s->bitbang = bitbang_i2c_init(bus);
> +    bitbang_i2c_init(&s->bitbang, bus);
>  
>      qdev_init_gpio_in(dev, bitbang_i2c_gpio_set, 2);
>      qdev_init_gpio_out(dev, &s->out, 1);
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index 5fb4f86c38f..462729db4ea 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -311,9 +311,9 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, 
> uint64_t value,
>      case IIC_DIRECTCNTL:
>          i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & 
> IIC_DIRECTCNTL_SCLC);
>          i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> -        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL,
> +        bitbang_i2c_set(&i2c->bitbang, BITBANG_I2C_SCL,
>                          i2c->directcntl & IIC_DIRECTCNTL_MSCL);
> -        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> +        i2c->directcntl |= bitbang_i2c_set(&i2c->bitbang, BITBANG_I2C_SDA,
>                                 (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
>          break;
>      default:
> @@ -347,7 +347,7 @@ static void ppc4xx_i2c_init(Object *o)
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
>      sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
>      s->bus = i2c_init_bus(DEVICE(s), "i2c");
> -    s->bitbang = bitbang_i2c_init(s->bus);
> +    bitbang_i2c_init(&s->bitbang, s->bus);
>  }
>  
>  static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/i2c/versatile_i2c.c b/hw/i2c/versatile_i2c.c
> index 24b6e36b6d5..1ac2a6f59a0 100644
> --- a/hw/i2c/versatile_i2c.c
> +++ b/hw/i2c/versatile_i2c.c
> @@ -35,7 +35,7 @@ typedef struct VersatileI2CState {
>      SysBusDevice parent_obj;
>  
>      MemoryRegion iomem;
> -    bitbang_i2c_interface *bitbang;
> +    bitbang_i2c_interface bitbang;
>      int out;
>      int in;
>  } VersatileI2CState;
> @@ -70,8 +70,8 @@ static void versatile_i2c_write(void *opaque, hwaddr offset,
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "%s: Bad offset 0x%x\n", __func__, (int)offset);
>      }
> -    bitbang_i2c_set(s->bitbang, BITBANG_I2C_SCL, (s->out & 1) != 0);
> -    s->in = bitbang_i2c_set(s->bitbang, BITBANG_I2C_SDA, (s->out & 2) != 0);
> +    bitbang_i2c_set(&s->bitbang, BITBANG_I2C_SCL, (s->out & 1) != 0);
> +    s->in = bitbang_i2c_set(&s->bitbang, BITBANG_I2C_SDA, (s->out & 2) != 0);
>  }
>  
>  static const MemoryRegionOps versatile_i2c_ops = {
> @@ -88,7 +88,7 @@ static void versatile_i2c_init(Object *obj)
>      I2CBus *bus;
>  
>      bus = i2c_init_bus(dev, "i2c");
> -    s->bitbang = bitbang_i2c_init(bus);
> +    bitbang_i2c_init(&s->bitbang, bus);
>      memory_region_init_io(&s->iomem, obj, &versatile_i2c_ops, s,
>                            "versatile_i2c", 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
>