[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-p
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct |
Date: |
Wed, 3 Jul 2019 10:43:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
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);
>