qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather t


From: BALATON Zoltan
Subject: Re: [Qemu-arm] [PATCH for-4.1] hw/i2c/bitbang_i2c: Use in-place rather than malloc'd bitbang_i2c_interface struct
Date: Tue, 2 Jul 2019 23:52:52 +0200 (CEST)
User-agent: Alpine 2.21.9999 (BSF 287 2018-06-16)

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.

---
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 <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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]