Re: [RFC PATCH-for-9.1 3/4] hw/i2c: Convert to spec v7 terminology (automatically)

[Wolfram Sang]

Hi Philippe,

> One of the biggest change from I2C spec v6 -> v7 is:
> 
>   • Updated the terms "master/slave" to "controller/target"
> 
> Since it follows the inclusive terminology from the "Conscious
> Language in your Open Source Projects" guidelines [*], replace
> the I2C terminology.

...

> Inspired-by: Wolfram Sang 

Cool, I am glad that I could inspire you to do this for QEMU.

Good luck with the series,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH v6 2/8] i2c: i801: Use GPIO_LOOKUP() helper macro

[Wolfram Sang]

On Tue, Mar 24, 2020 at 02:56:47PM +0100, Geert Uytterhoeven wrote:
> i801_add_mux() fills in the GPIO lookup table by manually populating an
> array of gpiod_lookup structures.  Use the existing GPIO_LOOKUP() helper
> macro instead, to relax a dependency on the gpiod_lookup structure's
> member names.
> 
> Signed-off-by: Geert Uytterhoeven 
> Cc: Jean Delvare 
> Cc: linux-...@vger.kernel.org

Applied to for-next, thanks!



signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v3 RESEND 1/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

The value for "rom-size" is used as a divisor, so it must not be 0 or it
will segfault. A size of 0 wouldn't make sense anyhow.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 22183f5360..ccf78b25e4 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -121,6 +121,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
 {
 EEPROMState *ee = AT24C_EE(i2c);
 
+if (!ee->rsize) {
+ERR("rom-size not allowed to be 0\n");
+exit(1);
+}
+
 ee->mem = g_malloc0(ee->rsize);
 
 if (ee->blk) {
-- 
2.11.0

[Qemu-devel] [PATCH v3 RESEND 2/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

0 as "rom-size" will lead to an error message. Let's use the size of a
small 24c01 which has 128 byte.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index ccf78b25e4..ab1ef686e2 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -28,6 +28,9 @@
 #define TYPE_AT24C_EE "at24c-eeprom"
 #define AT24C_EE(obj) OBJECT_CHECK(EEPROMState, (obj), TYPE_AT24C_EE)
 
+/* default is the size of a 24c01 EEPROM */
+#define AT24C_ROMSIZE_DEFAULT 128
+
 typedef struct EEPROMState {
 I2CSlave parent_obj;
 
@@ -171,7 +174,7 @@ void at24c_eeprom_reset(DeviceState *state)
 }
 
 static Property at24c_eeprom_props[] = {
-DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, AT24C_ROMSIZE_DEFAULT),
 DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
 DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
 DEFINE_PROP_END_OF_LIST()
-- 
2.11.0

[Qemu-devel] [PATCH v3 RESEND 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

I used this driver as a template for a custom one. While hacking on my own, I
noticed some problems in this driver, too. This series fixes the first set of
them, related to the "rom-size" parameter. It fixes a segfault.

I think the first patch is clearly suitable for stable. I think the second one,
too, but not as clearly. The third one is a cleanup and not for stable. Still,
I am open for opinions about these thoughts.

Thanks,

   Wolfram

This is the same v3 as last time. Rebased to current master, but that produced
no diff.

Changes since v2:

* removed '\n' from error_report-strings
* made sure checkpatch is happy
* added tags from Philippe (thanks!)

Changes since v1:

* reordered patches according to significance for stable
* use AT24C_ROMSIZE_DEFAULT instead of magic value
* patch 3 doesn't improve the ERR macro anymore but replaces
  it completely with error_report().


Wolfram Sang (3):
  nvram: at24c: prevent segfault by checking "rom-size"
  nvram: at24c: use a sane default for "rom-size"
  nvram: at24c: use standard error reporting

 hw/nvram/eeprom_at24c.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v3 RESEND 3/3] nvram: at24c: use standard error reporting

[Wolfram Sang]

Replace the ERR macro with error_report() because fprintf is deprecated.
This also fixes the prefix printed out twice.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index ab1ef686e2..c35b29e503 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -13,6 +13,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/i2c.h"
 #include "sysemu/block-backend.h"
+#include "qemu/error-report.h"
 
 /* #define DEBUG_AT24C */
 
@@ -22,9 +23,6 @@
 #define DPRINTK(FMT, ...) do {} while (0)
 #endif
 
-#define ERR(FMT, ...) fprintf(stderr, TYPE_AT24C_EE " : " FMT, \
-## __VA_ARGS__)
-
 #define TYPE_AT24C_EE "at24c-eeprom"
 #define AT24C_EE(obj) OBJECT_CHECK(EEPROMState, (obj), TYPE_AT24C_EE)
 
@@ -63,8 +61,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
 if (ee->blk && ee->changed) {
 int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : failed to write backing file\n");
+error_report("failed to write backing file");
 }
 DPRINTK("Wrote to backing file\n");
 }
@@ -125,7 +122,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 EEPROMState *ee = AT24C_EE(i2c);
 
 if (!ee->rsize) {
-ERR("rom-size not allowed to be 0\n");
+error_report("rom-size not allowed to be 0");
 exit(1);
 }
 
@@ -135,7 +132,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 int64_t len = blk_getlength(ee->blk);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
+error_report("Backing file size %lu != %u",
 (unsigned long)len, (unsigned)ee->rsize);
 exit(1);
 }
@@ -143,8 +140,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  BLK_PERM_ALL, &error_fatal) < 0)
 {
-ERR(TYPE_AT24C_EE
-" : Backing file incorrect permission\n");
+error_report("Backing file incorrect permission");
 exit(1);
 }
 }
@@ -166,8 +162,7 @@ void at24c_eeprom_reset(DeviceState *state)
 int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : Failed initial sync with backing file\n");
+error_report("Failed initial sync with backing file");
 }
 DPRINTK("Reset read backing file\n");
 }
-- 
2.11.0

Re: [Qemu-devel] [PATCH v3 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

On Tue, Mar 20, 2018 at 05:18:47PM +0100, Wolfram Sang wrote:
> I used this driver as a template for a custom one. While hacking on my own, I
> noticed some problems in this driver, too. This series fixes the first set of
> them, related to the "rom-size" parameter. It fixes a segfault.
> 
> I think the first patch is clearly suitable for stable. I think the second 
> one,
> too, but not as clearly. The third one is a cleanup and not for stable. Still,
> I am open for opinions about these thoughts.

Through which tree should these patches go? get_maintainer doesn't list
a person specifically. Anything else I could do?

> 
> Thanks,
> 
>Wolfram
> 
> Changes since v2:
> 
> * removed '\n' from error_report-strings
> * made sure checkpatch is happy
> * added tags from Philippe (thanks!)
> 
> Changes since v1:
> 
> * reordered patches according to significance for stable
> * use AT24C_ROMSIZE_DEFAULT instead of magic value
> * patch 3 doesn't improve the ERR macro anymore but replaces
>   it completely with error_report().
> 
> 
> Wolfram Sang (3):
>   nvram: at24c: prevent segfault by checking "rom-size"
>   nvram: at24c: use a sane default for "rom-size"
>   nvram: at24c: use standard error reporting
> 
>  hw/nvram/eeprom_at24c.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> -- 
> 2.11.0
> 


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v3 3/3] nvram: at24c: use standard error reporting

[Wolfram Sang]

Replace the ERR macro with error_report() because fprintf is deprecated.
This also fixes the prefix printed out twice.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index ab1ef686e2..c35b29e503 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -13,6 +13,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/i2c.h"
 #include "sysemu/block-backend.h"
+#include "qemu/error-report.h"
 
 /* #define DEBUG_AT24C */
 
@@ -22,9 +23,6 @@
 #define DPRINTK(FMT, ...) do {} while (0)
 #endif
 
-#define ERR(FMT, ...) fprintf(stderr, TYPE_AT24C_EE " : " FMT, \
-## __VA_ARGS__)
-
 #define TYPE_AT24C_EE "at24c-eeprom"
 #define AT24C_EE(obj) OBJECT_CHECK(EEPROMState, (obj), TYPE_AT24C_EE)
 
@@ -63,8 +61,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
 if (ee->blk && ee->changed) {
 int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : failed to write backing file\n");
+error_report("failed to write backing file");
 }
 DPRINTK("Wrote to backing file\n");
 }
@@ -125,7 +122,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 EEPROMState *ee = AT24C_EE(i2c);
 
 if (!ee->rsize) {
-ERR("rom-size not allowed to be 0\n");
+error_report("rom-size not allowed to be 0");
 exit(1);
 }
 
@@ -135,7 +132,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 int64_t len = blk_getlength(ee->blk);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
+error_report("Backing file size %lu != %u",
 (unsigned long)len, (unsigned)ee->rsize);
 exit(1);
 }
@@ -143,8 +140,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  BLK_PERM_ALL, &error_fatal) < 0)
 {
-ERR(TYPE_AT24C_EE
-" : Backing file incorrect permission\n");
+error_report("Backing file incorrect permission");
 exit(1);
 }
 }
@@ -166,8 +162,7 @@ void at24c_eeprom_reset(DeviceState *state)
 int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : Failed initial sync with backing file\n");
+error_report("Failed initial sync with backing file");
 }
 DPRINTK("Reset read backing file\n");
 }
-- 
2.11.0

[Qemu-devel] [PATCH v3 2/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

0 as "rom-size" will lead to an error message. Let's use the size of a
small 24c01 which has 128 byte.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index ccf78b25e4..ab1ef686e2 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -28,6 +28,9 @@
 #define TYPE_AT24C_EE "at24c-eeprom"
 #define AT24C_EE(obj) OBJECT_CHECK(EEPROMState, (obj), TYPE_AT24C_EE)
 
+/* default is the size of a 24c01 EEPROM */
+#define AT24C_ROMSIZE_DEFAULT 128
+
 typedef struct EEPROMState {
 I2CSlave parent_obj;
 
@@ -171,7 +174,7 @@ void at24c_eeprom_reset(DeviceState *state)
 }
 
 static Property at24c_eeprom_props[] = {
-DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, AT24C_ROMSIZE_DEFAULT),
 DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
 DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
 DEFINE_PROP_END_OF_LIST()
-- 
2.11.0

[Qemu-devel] [PATCH v3 1/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

The value for "rom-size" is used as a divisor, so it must not be 0 or it
will segfault. A size of 0 wouldn't make sense anyhow.

Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 22183f5360..ccf78b25e4 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -121,6 +121,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
 {
 EEPROMState *ee = AT24C_EE(i2c);
 
+if (!ee->rsize) {
+ERR("rom-size not allowed to be 0\n");
+exit(1);
+}
+
 ee->mem = g_malloc0(ee->rsize);
 
 if (ee->blk) {
-- 
2.11.0

[Qemu-devel] [PATCH v3 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

I used this driver as a template for a custom one. While hacking on my own, I
noticed some problems in this driver, too. This series fixes the first set of
them, related to the "rom-size" parameter. It fixes a segfault.

I think the first patch is clearly suitable for stable. I think the second one,
too, but not as clearly. The third one is a cleanup and not for stable. Still,
I am open for opinions about these thoughts.

Thanks,

   Wolfram

Changes since v2:

* removed '\n' from error_report-strings
* made sure checkpatch is happy
* added tags from Philippe (thanks!)

Changes since v1:

* reordered patches according to significance for stable
* use AT24C_ROMSIZE_DEFAULT instead of magic value
* patch 3 doesn't improve the ERR macro anymore but replaces
  it completely with error_report().


Wolfram Sang (3):
  nvram: at24c: prevent segfault by checking "rom-size"
  nvram: at24c: use a sane default for "rom-size"
  nvram: at24c: use standard error reporting

 hw/nvram/eeprom_at24c.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

> > +if (!ee->rsize) {
> > +ERR("rom-size not allowed to be 0\n");
> 
> You can directly use error_report() in this patch.

My reasoning was that this patch is suitable for stable while the
error_report() stuff is not. I neither wanted to mix those two here nor
did I want to make the stable-patch depend on the non-stable patch. So,
I chose to use ERR here and fix all of ERR later. OK?



signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2 3/3] nvram: at24c: use standard error reporting

[Wolfram Sang]

> > -ERR(TYPE_AT24C_EE
> > -" : failed to write backing file\n");
> > +error_report("failed to write backing file\n");
> 
> Drop the \n here and elsewhere in your patch; error_report() already does
> that for you.

Darn, I haven't installed my automatic-checkpatch-script for qemu yet :(
Sorry, will wait for more review comments and fix!



signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v2 3/3] nvram: at24c: use standard error reporting

[Wolfram Sang]

Replace the ERR macro with error_report() because fprintf is deprecated.
This also fixes the prefix printed out twice.

Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index ab1ef686e2..1560c9a46b 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -13,6 +13,7 @@
 #include "hw/hw.h"
 #include "hw/i2c/i2c.h"
 #include "sysemu/block-backend.h"
+#include "qemu/error-report.h"
 
 /* #define DEBUG_AT24C */
 
@@ -22,9 +23,6 @@
 #define DPRINTK(FMT, ...) do {} while (0)
 #endif
 
-#define ERR(FMT, ...) fprintf(stderr, TYPE_AT24C_EE " : " FMT, \
-## __VA_ARGS__)
-
 #define TYPE_AT24C_EE "at24c-eeprom"
 #define AT24C_EE(obj) OBJECT_CHECK(EEPROMState, (obj), TYPE_AT24C_EE)
 
@@ -63,8 +61,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
 if (ee->blk && ee->changed) {
 int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : failed to write backing file\n");
+error_report("failed to write backing file\n");
 }
 DPRINTK("Wrote to backing file\n");
 }
@@ -125,7 +122,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 EEPROMState *ee = AT24C_EE(i2c);
 
 if (!ee->rsize) {
-ERR("rom-size not allowed to be 0\n");
+error_report("rom-size not allowed to be 0\n");
 exit(1);
 }
 
@@ -135,7 +132,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 int64_t len = blk_getlength(ee->blk);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
+error_report("Backing file size %lu != %u\n",
 (unsigned long)len, (unsigned)ee->rsize);
 exit(1);
 }
@@ -143,8 +140,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  BLK_PERM_ALL, &error_fatal) < 0)
 {
-ERR(TYPE_AT24C_EE
-" : Backing file incorrect permission\n");
+error_report("Backing file incorrect permission\n");
 exit(1);
 }
 }
@@ -166,8 +162,7 @@ void at24c_eeprom_reset(DeviceState *state)
 int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : Failed initial sync with backing file\n");
+error_report("Failed initial sync with backing file\n");
 }
 DPRINTK("Reset read backing file\n");
 }
-- 
2.11.0

[Qemu-devel] [PATCH v2 1/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

The value for "rom-size" is used as a divisor, so it must not be 0 or it
will segfault. A size of 0 wouldn't make sense anyhow.

Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 22183f5360..ccf78b25e4 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -121,6 +121,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
 {
 EEPROMState *ee = AT24C_EE(i2c);
 
+if (!ee->rsize) {
+ERR("rom-size not allowed to be 0\n");
+exit(1);
+}
+
 ee->mem = g_malloc0(ee->rsize);
 
 if (ee->blk) {
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

0 as "rom-size" will lead to an error message. Let's use the size of a
small 24c01 which has 128 byte.

Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index ccf78b25e4..ab1ef686e2 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -28,6 +28,9 @@
 #define TYPE_AT24C_EE "at24c-eeprom"
 #define AT24C_EE(obj) OBJECT_CHECK(EEPROMState, (obj), TYPE_AT24C_EE)
 
+/* default is the size of a 24c01 EEPROM */
+#define AT24C_ROMSIZE_DEFAULT 128
+
 typedef struct EEPROMState {
 I2CSlave parent_obj;
 
@@ -171,7 +174,7 @@ void at24c_eeprom_reset(DeviceState *state)
 }
 
 static Property at24c_eeprom_props[] = {
-DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, AT24C_ROMSIZE_DEFAULT),
 DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
 DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
 DEFINE_PROP_END_OF_LIST()
-- 
2.11.0

[Qemu-devel] [PATCH v2 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

I used this driver as a template for a custom one. While hacking on my own, I
noticed some problems in this driver, too. This series fixes the first set of
them, related to the "rom-size" parameter. It fixes a segfault.

I think the first patch is clearly suitable for stable. I think the second one,
too, but not as clearly. The third one is a cleanup and not for stable. Still,
I am open for opinions about these thoughts.

Thanks,

   Wolfram

Changes since v1:

* reordered patches according to significance for stable
* use AT24C_ROMSIZE_DEFAULT instead of magic value
* patch 3 doesn't improve the ERR macro anymore but replaces
  it completely with error_report().


Wolfram Sang (3):
  nvram: at24c: prevent segfault by checking "rom-size"
  nvram: at24c: use a sane default for "rom-size"
  nvram: at24c: use standard error reporting

 hw/nvram/eeprom_at24c.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

Hi Philippe,

> > I don't mind much, but why? My reasoning was "let's first fix the cause
> > and then the symptom"?
> 
> The '0' case is worst than incorrect, it segfaults, so you are right :)

Ok, thanks.

> >> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.
> > 
> > Can do, of course. But won't that give room for regressions because
> > people are already using it with lower values?
> 
> Your patch already introduce the regression :)

Not really. The current default setting (0) segfaults. This is how I
discovered it. So, there can't be any active user of that.

> I prefer self-explanatory #defines than magic value, but I see your
> point, so if we can not decide a value, can you add a comment to explain
> the magic value? I think the clearer is to add a #define with a comment.

I wouldn't mind a macro AT24C_ROMSIZE_DEFAULT and a comment explaining
why we chose this value. This is totally fine with me.

It was just the MIN value I saw potential usage regressions with.

> IMO there are too many AT24C eeproms to model, so the "rom-size"
> variable is the easiest way. Your patch #2 is simple enough to avoid the
> #DIV/0!

Fine with me, too. I will update my series accordingly.

Thanks,

   Wolfram


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 1/3] nvram: at24c: remove doubled prefix for ERR

[Wolfram Sang]

> > -ERR(TYPE_AT24C_EE
> > -" : failed to write backing file\n");
> > +ERR("failed to write backing file\n");
> 
> printf/fprintf are deprecated, since you are modifying this file can you
> use a newer API, "qemu/error-report.h" for example.

Sure, I'll have a look.

> >  }
> >  DPRINTK("Wrote to backing file\n");
> 
> DPRINTK() can be converted to tracepoint.

I'd leave that for another incremental change.



signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

Hi Philippe,

> >  static Property at24c_eeprom_props[] = {
> > -DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
> > +DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
> 
> This patch should goes before your 2/3 in your series.

I don't mind much, but why? My reasoning was "let's first fix the cause
and then the symptom"?

> Can you add a #define for this value? Such AT24C_ROMSIZE_MIN.

Can do, of course. But won't that give room for regressions because
people are already using it with lower values?

Ideally, we would have a "model" variable. The model type would define
the size of the memory. The "rom-size" variable could then be kept as is
(except for the 0 bugfix) or deprecated?

Thanks for the review,

   Wolfram


signature.asc
Description: PGP signature

[Qemu-devel] [RFC PATCH] i2c: device passthrough HACK(!) & evaluation

[Wolfram Sang]

I was asked to investigate I2C device passthrough possibilities for QEMU
on Linux. The idea was to expose only a single device, not the whole
bus. There was no specific use case explained, so some decisions are
still to be made. E.g. I think the host-device should get its own
virtualized bus later, but I can't really tell yet.

This is my first contact with QEMU, so the patch is more like getting my
feet wet. I wouldn't even call it a proof-of-concept. However, while
working on this (which gets at least something done), I already gained
experiences which I would like to share here:

Full virtualization
---

Would be nice to have because it would work with all virtual I2C
adapters instantly, however there come problems with it. For that to
work, we would need to be able to transmit the QEMU I2C primitives from
userspace to the kernel. There is currently no such interface for that.
As of today, there is only the i2c-dev interface which allows to send
a complete transfer (which may consist of multiple, combined messages)
or SMBus commands. There is no way to send more primitive commands like
"send {start|stop|acknowledge}-bit". Even if there was, most hardware I
know of wouldn't work well with this. We often need a-priori knowledge
like length of the message to program the controller. Such information
is not available when working with such primitives. On top of that, that
approach would cause quite some overhead, so performance regressions for
drivers which use other devices on the same bus are likely.

virtio?
---

>From what I understood so far, virtio could help here. Yes, we would
need separate drivers, yet data transfer could be super simple. If we
had a simple virtio-PCI I2C master device, it could have a really simple
kernel driver. It basically takes the I2C transfer it gets, does some
sanity checking so no other devices are accessed, and then passes it
to the kernel. I have not fully understood yet, if the virtio
transportation mechanism is better/required here, or if we can/should
still use the existing i2c-dev character interface.

Other problems found


Here is a list of other problems I discovered which need addressing in
one way or the other:

* exclusive access to I2C devices

We currently don't have a way to mark devices as being "in use, don't
touch" from userspace. This could be added but we need to decide on the
transportation layer first (i2c-dev vs. virtio).

* no generic I2C master (at least for x86)

Unless I overlooked something, we currently can't simply add a new I2C
bus on x86 because there is no virtual hardware encoded just doing that.
I found patches for a USB-to-I2C bridge floating around. USB is nicely
generic, so probably worth evaluating those again if this task is to be
continued.

* re-definition of I2C_SLAVE

QEMU defines I2C_SLAVE as well as . So, if that
interface is going to be used, we need something to fix this.

* likely improvements to the QEMU I2C core

>From visual review, I am quite sure QEMU I2C core does not support
'repeated start' with the following message having a different I2C
destination address than the previous one. This is legal, but up to
now very rarely seen in practice. However, with devices becoming more
complex and those devices maybe being passed-through as well, more
improvements to the QEMU I2C core might be needed as well.

Conclusion
--

These are my finding regarding I2C device passthrough with QEMU. The
below patch is a very first step in the "full virtualization" direction
because it transports every read byte access directly to the host
(totally missing proper I2C start/stop/ack generation). Write byte
access is discarded here for security reasons. As described above, I
would not recommend this approach any further. My staring point now
would be a simplified virtio or virtio-alike device where the transfer
is passed-through as such to the host, and not split up into primitives.
So the patch itself is somehow already obsolete, but it served well for
gaining experience.

For the record, the description of my test procedure is here:
https://elinux.org/R-Car/I2C-Virtualization

I will still be around for further discussion. I can't really tell,
though, if there will be a follow-up task for me to continue this.

Signed-off-by: Wolfram Sang 
---
 hw/i2c/Makefile.objs |   2 +-
 hw/i2c/host-i2cdev.c | 110 +++
 2 files changed, 111 insertions(+), 1 deletion(-)
 create mode 100644 hw/i2c/host-i2cdev.c

diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 37cacde978..d0cc08dd10 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -1,4 +1,4 @@
-common-obj-$(CONFIG_I2C) += core.o smbus.o smbus_eeprom.o
+common-obj-$(CONFIG_I2C) += core.o smbus.o smbus_eeprom.o host-i2cdev.o
 common-obj-$(CONFIG_DDC) += i2c-ddc.o
 common-obj-$(CONFIG_VERSATILE_I2C) 

[Qemu-devel] [PATCH 1/3] nvram: at24c: remove doubled prefix for ERR

[Wolfram Sang]

The ERR macro already has the TYPE_AT24C_EE prefix, no need to repeat in
the error message.

Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 5494351976..8507516b7e 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -60,8 +60,7 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
 if (ee->blk && ee->changed) {
 int len = blk_pwrite(ee->blk, 0, ee->mem, ee->rsize, 0);
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : failed to write backing file\n");
+ERR("failed to write backing file\n");
 }
 DPRINTK("Wrote to backing file\n");
 }
@@ -127,7 +126,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 int64_t len = blk_getlength(ee->blk);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE " : Backing file size %lu != %u\n",
+ERR("Backing file size %lu != %u\n",
 (unsigned long)len, (unsigned)ee->rsize);
 exit(1);
 }
@@ -135,8 +134,7 @@ int at24c_eeprom_init(I2CSlave *i2c)
 if (blk_set_perm(ee->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
  BLK_PERM_ALL, &error_fatal) < 0)
 {
-ERR(TYPE_AT24C_EE
-" : Backing file incorrect permission\n");
+ERR("Backing file incorrect permission\n");
 exit(1);
 }
 }
@@ -158,8 +156,7 @@ void at24c_eeprom_reset(DeviceState *state)
 int len = blk_pread(ee->blk, 0, ee->mem, ee->rsize);
 
 if (len != ee->rsize) {
-ERR(TYPE_AT24C_EE
-" : Failed initial sync with backing file\n");
+ERR("Failed initial sync with backing file\n");
 }
 DPRINTK("Reset read backing file\n");
 }
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] nvram: at24c: prevent segfault by checking "rom-size"

[Wolfram Sang]

The value for "rom-size" is used as a divisor, so it must not be 0 or it
will segfault. A size of 0 wouldn't make sense as well.

Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 8507516b7e..d82710e1df 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -120,6 +120,11 @@ int at24c_eeprom_init(I2CSlave *i2c)
 {
 EEPROMState *ee = AT24C_EE(i2c);
 
+if (!ee->rsize) {
+ERR("rom-size not allowed to be 0\n");
+exit(1);
+}
+
 ee->mem = g_malloc0(ee->rsize);
 
 if (ee->blk) {
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] nvram: at24c: use a sane default for "rom-size"

[Wolfram Sang]

0 as "rom-size" doesn't make much sense, let's use the smallest 24cXX
which has 128 byte.

Signed-off-by: Wolfram Sang 
---
 hw/nvram/eeprom_at24c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index d82710e1df..de988f8d07 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -168,7 +168,7 @@ void at24c_eeprom_reset(DeviceState *state)
 }
 
 static Property at24c_eeprom_props[] = {
-DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 0),
+DEFINE_PROP_UINT32("rom-size", EEPROMState, rsize, 128),
 DEFINE_PROP_BOOL("writable", EEPROMState, writable, true),
 DEFINE_PROP_DRIVE("drive", EEPROMState, blk),
 DEFINE_PROP_END_OF_LIST()
-- 
2.11.0

[Qemu-devel] [PATCH 0/3] nvram: at24c: fix problems related to "rom-size"

[Wolfram Sang]

I used this driver as a template for a custom one. While hacking on my own, I
noticed some problems in this driver, too. This series fixes the first set of
them, mostly related to "rom-size". It fixes a segfault.

Wolfram Sang (3):
  nvram: at24c: remove doubled prefix for ERR
  nvram: at24c: prevent segfault by checking "rom-size"
  nvram: at24c: use a sane default for "rom-size"

 hw/nvram/eeprom_at24c.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
2.11.0