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 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 <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'.
> 
> 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 <address@hidden>

>> ---
>> 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 <address@hidden>
> 
> 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
>