| Message ID | 20190702163844.20458-1-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct | expand |
On Tue, 2 Jul 2019, 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 <peter.maydell@linaro.org> > --- > 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'. I've tested the ppc4xx and ati-vga parts with AROS and MorphOS and these can still access i2c devices so I think it works. > --- > 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; Maybe this typedef ^^^ can now be moved to the struct below instead of coming first (or even written in the same line as typedef struct { } bitbang_i2c_interface; as there is no need to have both struct bitbang_i2c_interface and the typedeffed name available). But I don't really mind, so Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> Thanks for fixing this. Regards, BALATON Zoltan > #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
On Tue, Jul 02, 2019 at 05:38:44PM +0100, 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 <peter.maydell@linaro.org> ppc parts Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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; > 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); -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
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 <peter.maydell@linaro.org> > --- > 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é <philmd@redhat.com> > 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); >
On 7/2/19 11:52 PM, BALATON Zoltan wrote: > On Tue, 2 Jul 2019, 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 <peter.maydell@linaro.org> >> --- >> 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'. > > I've tested the ppc4xx and ati-vga parts with AROS and MorphOS and these > can still access i2c devices so I think it works. So: Tested-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> 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; > > Maybe this typedef ^^^ can now be moved to the struct below instead of > coming first (or even written in the same line as > typedef struct { } bitbang_i2c_interface; > as there is no need to have both struct bitbang_i2c_interface and the > typedeffed name available). Agreed, from "hw/i2c/i2c.h" to "hw/i2c/bitbang_i2c.h". > But I don't really mind, so > > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > > Thanks for fixing this. > > Regards, > BALATON Zoltan > >> #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 >
On Wed, 3 Jul 2019, Philippe Mathieu-Daudé wrote: > On 7/2/19 11:52 PM, BALATON Zoltan wrote: >> On Tue, 2 Jul 2019, 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 <peter.maydell@linaro.org> >>> --- >>> 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'. >> >> I've tested the ppc4xx and ati-vga parts with AROS and MorphOS and these >> can still access i2c devices so I think it works. > > So: > > Tested-by: BALATON Zoltan <balaton@eik.bme.hu> Yes, but I gave my Reviewed-by so I thought no need for both but if you want can also add Tested-by. Regards, BALATON Zoltan >>> --- >>> 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; >> >> Maybe this typedef ^^^ can now be moved to the struct below instead of >> coming first (or even written in the same line as >> typedef struct { } bitbang_i2c_interface; >> as there is no need to have both struct bitbang_i2c_interface and the >> typedeffed name available). > > Agreed, from "hw/i2c/i2c.h" to "hw/i2c/bitbang_i2c.h". > >> But I don't really mind, so >> >> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> >> >> Thanks for fixing this. >> >> Regards, >> BALATON Zoltan >> >>> #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/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; 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);
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 <peter.maydell@linaro.org> --- 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(-) -- 2.20.1