* Re: [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct
2019-07-02 16:38 [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct Peter Maydell
@ 2019-07-02 21:52 ` BALATON Zoltan
2019-07-03 8:43 ` Philippe Mathieu-Daudé
2019-07-03 0:45 ` David Gibson
2019-07-03 8:43 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 6+ messages in thread
From: BALATON Zoltan @ 2019-07-02 21:52 UTC (permalink / raw)
To: Peter Maydell; +Cc: Gerd Hoffmann, qemu-arm, qemu-ppc, qemu-devel, David Gibson
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct
2019-07-02 21:52 ` BALATON Zoltan
@ 2019-07-03 8:43 ` Philippe Mathieu-Daudé
2019-07-03 11:07 ` BALATON Zoltan
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-03 8:43 UTC (permalink / raw)
To: BALATON Zoltan, Peter Maydell
Cc: qemu-arm, qemu-ppc, Gerd Hoffmann, David Gibson, qemu-devel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct
2019-07-03 8:43 ` Philippe Mathieu-Daudé
@ 2019-07-03 11:07 ` BALATON Zoltan
0 siblings, 0 replies; 6+ messages in thread
From: BALATON Zoltan @ 2019-07-03 11:07 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, qemu-devel, qemu-arm, qemu-ppc, Gerd Hoffmann,
David Gibson
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
>>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct
2019-07-02 16:38 [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct Peter Maydell
2019-07-02 21:52 ` BALATON Zoltan
@ 2019-07-03 0:45 ` David Gibson
2019-07-03 8:43 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 6+ messages in thread
From: David Gibson @ 2019-07-03 0:45 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, qemu-ppc, qemu-devel, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 11197 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct
2019-07-02 16:38 [Qemu-devel] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct Peter Maydell
2019-07-02 21:52 ` BALATON Zoltan
2019-07-03 0:45 ` David Gibson
@ 2019-07-03 8:43 ` Philippe Mathieu-Daudé
2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-03 8:43 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: qemu-arm, qemu-ppc, Gerd Hoffmann, David Gibson
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread