Skip to contentSkip to conversation

rtc: remove VLA usage

PATCH In review Gustavo A. R. Silva 3 participants 8 messages Mar 7, 2018 Real time clock (RTC)

lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 rtc: remove VLA usage

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/rtc/rtc-bq32k.c  | 2 +-
 drivers/rtc/rtc-mcp795.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
 static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];
 
 	buffer[0] = off;
 	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +1 -1
index 79e24ea..00e11c1 100644
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];
 
 	tx[0] = MCP795_WRITE;
 	tx[1] = addr;
Signature · 1 line
2.7.4
maintainer
#1 lore raw
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Quoted 11 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/rtc/rtc-bq32k.c  | 2 +-
 drivers/rtc/rtc-mcp795.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/rtc/rtc-bq32k.c +0 -0
index e8698e9..e4b234a 100644

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 78

Quoted 6 lines
drivers/rtc/rtc-bq32k.c +1 -1
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
 static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but this is way too much for this rtc, it only has 10 consecutive registers.

Quoted 18 lines
drivers/rtc/rtc-bq32k.c +0 -0
 	buffer[0] = off;
 	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +1 -1
index 79e24ea..00e11c1 100644
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
 {
 	struct spi_device *spi = to_spi_device(dev);
 	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];
 
 	tx[0] = MCP795_WRITE;
 	tx[1] = addr;
Signature · 1 line
2.7.4
Signature · 3 lines
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#2 lore raw

Hi Alexandre,

Quoted 5 lines · Alexandre Belloni
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Sure. I can elaborate a little bit more.

Quoted 6 lines
Quoted 6 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/rtc/rtc-bq32k.c  | 2 +-
  drivers/rtc/rtc-mcp795.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
Quoted 12 lines

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 78

Quoted 11 lines
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
  static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
  {
  	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but

Correct.

Quoted 1 line

this is way too much for this rtc, it only has 10 consecutive registers.

In that case probably the best solution is to add the following line to the module:

#define MAX_LEN 10

and update the rest of the code as follows:

uint8_t buffer[MAX_LEN + 1];

Quoted 4 lines
Quoted 4 lines
drivers/rtc/rtc-bq32k.c +0 -0
  	buffer[0] = off;
  	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +0 -0
index 79e24ea..00e11c1 100644
Quoted 8 lines

drivers/rtc/rtc-mcp795.c static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count) · line 85

Quoted 8 lines
drivers/rtc/rtc-mcp795.c +1 -1
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
  {
  	struct spi_device *spi = to_spi_device(dev);
  	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];

For this particular case it seems to me that the following works just fine:

#define MAX_COUNT 7

u8 tx[MAX_COUNT + 2];

What do you think?

Thanks for your feedback.
--
Gustavo

Quoted 6 lines
Quoted 6 lines
drivers/rtc/rtc-mcp795.c +0 -0
  	tx[0] = MCP795_WRITE;
  	tx[1] = addr;
Signature · 1 line
2.7.4
maintainer
#3 lore raw
Quoted 33 lines · Gustavo A. R. Silva

Hi Alexandre,

Quoted 5 lines · Alexandre Belloni
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Sure. I can elaborate a little bit more.

Quoted 6 lines
Quoted 6 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/rtc/rtc-bq32k.c  | 2 +-
  drivers/rtc/rtc-mcp795.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
Quoted 11 lines

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 77

Quoted 10 lines
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
  static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
  {
  	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but

Correct.

Quoted 1 line

this is way too much for this rtc, it only has 10 consecutive registers.

Quoted 9 lines

In that case probably the best solution is to add the following line to the module:

#define MAX_LEN 10

and update the rest of the code as follows:

uint8_t buffer[MAX_LEN + 1];

Seems better, yes.

Quoted 16 lines
Quoted 13 lines

drivers/rtc/rtc-mcp795.c static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count) · line 85

Quoted 13 lines
drivers/rtc/rtc-bq32k.c +0 -0
  	buffer[0] = off;
  	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +1 -1
index 79e24ea..00e11c1 100644
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
  {
  	struct spi_device *spi = to_spi_device(dev);
  	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];

For this particular case it seems to me that the following works just fine:

Quoted 6 lines

#define MAX_COUNT 7

u8 tx[MAX_COUNT + 2];

What do you think?

Nope, that RTC has a section of 255 bytes that could be read at once so 257 is the correct value.

Signature · 3 lines
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#4 lore raw
Quoted 56 lines · Alexandre Belloni
Quoted 30 lines · Gustavo A. R. Silva

Hi Alexandre,

Quoted 5 lines · Alexandre Belloni
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Sure. I can elaborate a little bit more.

Quoted 6 lines
Quoted 6 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
   drivers/rtc/rtc-bq32k.c  | 2 +-
   drivers/rtc/rtc-mcp795.c | 2 +-
   2 files changed, 2 insertions(+), 2 deletions(-)
Quoted 11 lines

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 77

Quoted 10 lines
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
   static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
   {
   	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but

Correct.

Quoted 1 line

this is way too much for this rtc, it only has 10 consecutive registers.

Quoted 9 lines

In that case probably the best solution is to add the following line to the module:

#define MAX_LEN 10

and update the rest of the code as follows:

uint8_t buffer[MAX_LEN + 1];

Seems better, yes.

Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
drivers/rtc/rtc-bq32k.c +0 -0
   	buffer[0] = off;
   	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +0 -0
index 79e24ea..00e11c1 100644
Quoted 8 lines

drivers/rtc/rtc-mcp795.c static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count) · line 85

Quoted 8 lines
drivers/rtc/rtc-mcp795.c +1 -1
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
   {
   	struct spi_device *spi = to_spi_device(dev);
   	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];

For this particular case it seems to me that the following works just fine:

Quoted 9 lines
Quoted 6 lines

#define MAX_COUNT 7

u8 tx[MAX_COUNT + 2];

What do you think?

Nope, that RTC has a section of 255 bytes that could be read at once so 257 is the correct value.

I see...

I was looking into this piece of code drivers/rtc/rtc-mcp795.c:302:

        tmp[0] = (tmp[0] & 0x80) | bin2bcd(alm->time.tm_sec);
        tmp[1] = (tmp[1] & 0x80) | bin2bcd(alm->time.tm_min);
        tmp[2] = (tmp[2] & 0xE0) | bin2bcd(alm->time.tm_hour);
        tmp[3] = (tmp[3] & 0x80) | bin2bcd(alm->time.tm_wday + 1);

        /* set alarm match: seconds, minutes, hour, day, date and month */ tmp[3] |= (MCP795_ALM0C2_BIT | MCP795_ALM0C1_BIT | MCP795_ALM0C0_BIT); tmp[4] = (tmp[4] & 0xC0) | bin2bcd(alm->time.tm_mday); tmp[5] = (tmp[5] & 0xE0) | bin2bcd(alm->time.tm_mon + 1);

        ret = mcp795_rtcc_write(dev, MCP795_REG_ALM0_SECONDS, tmp, sizeof(tmp));

OK. I'll work on v2 of this patch. I'll send it shortly.

Thanks
--
Gustavo

maintainer
#5 lore raw
Quoted 77 lines · Gustavo A. R. Silva
Quoted 56 lines · Alexandre Belloni
Quoted 30 lines · Gustavo A. R. Silva

Hi Alexandre,

Quoted 5 lines · Alexandre Belloni
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Sure. I can elaborate a little bit more.

Quoted 6 lines
Quoted 6 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
   drivers/rtc/rtc-bq32k.c  | 2 +-
   drivers/rtc/rtc-mcp795.c | 2 +-
   2 files changed, 2 insertions(+), 2 deletions(-)
Quoted 11 lines

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 77

Quoted 10 lines
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
   static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
   {
   	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but

Correct.

Quoted 1 line

this is way too much for this rtc, it only has 10 consecutive registers.

Quoted 9 lines

In that case probably the best solution is to add the following line to the module:

#define MAX_LEN 10

and update the rest of the code as follows:

uint8_t buffer[MAX_LEN + 1];

Seems better, yes.

Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
drivers/rtc/rtc-bq32k.c +0 -0
   	buffer[0] = off;
   	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +0 -0
index 79e24ea..00e11c1 100644
Quoted 8 lines

drivers/rtc/rtc-mcp795.c static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count) · line 85

Quoted 8 lines
drivers/rtc/rtc-mcp795.c +1 -1
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
   {
   	struct spi_device *spi = to_spi_device(dev);
   	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];

For this particular case it seems to me that the following works just fine:

Quoted 9 lines
Quoted 6 lines

#define MAX_COUNT 7

u8 tx[MAX_COUNT + 2];

What do you think?

Nope, that RTC has a section of 255 bytes that could be read at once so 257 is the correct value.

I see...

I was looking into this piece of code drivers/rtc/rtc-mcp795.c:302:

        tmp[0] = (tmp[0] & 0x80) | bin2bcd(alm->time.tm_sec);
        tmp[1] = (tmp[1] & 0x80) | bin2bcd(alm->time.tm_min);
        tmp[2] = (tmp[2] & 0xE0) | bin2bcd(alm->time.tm_hour);
        tmp[3] = (tmp[3] & 0x80) | bin2bcd(alm->time.tm_wday + 1);
Quoted 9 lines

        /* set alarm match: seconds, minutes, hour, day, date and month */ tmp[3] |= (MCP795_ALM0C2_BIT | MCP795_ALM0C1_BIT | MCP795_ALM0C0_BIT);
        tmp[4] = (tmp[4] & 0xC0) | bin2bcd(alm->time.tm_mday); tmp[5] = (tmp[5] & 0xE0) | bin2bcd(alm->time.tm_mon + 1);

        ret = mcp795_rtcc_write(dev, MCP795_REG_ALM0_SECONDS, tmp, sizeof(tmp));

Yeah, the 128 or 255 bytes NVRAM is not yet supported but this is on my todo list

Signature · 3 lines
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#6 lore raw
Quoted 82 lines · Alexandre Belloni
Quoted 81 lines · Gustavo A. R. Silva
Quoted 56 lines · Alexandre Belloni
Quoted 30 lines · Gustavo A. R. Silva

Hi Alexandre,

Quoted 5 lines · Alexandre Belloni
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Sure. I can elaborate a little bit more.

Quoted 6 lines
Quoted 6 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
    drivers/rtc/rtc-bq32k.c  | 2 +-
    drivers/rtc/rtc-mcp795.c | 2 +-
    2 files changed, 2 insertions(+), 2 deletions(-)
Quoted 11 lines

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 77

Quoted 10 lines
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
    static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
    {
    	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but

Correct.

Quoted 1 line

this is way too much for this rtc, it only has 10 consecutive registers.

Quoted 9 lines

In that case probably the best solution is to add the following line to the module:

#define MAX_LEN 10

and update the rest of the code as follows:

uint8_t buffer[MAX_LEN + 1];

Seems better, yes.

Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
drivers/rtc/rtc-bq32k.c +0 -0
    	buffer[0] = off;
    	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +0 -0
index 79e24ea..00e11c1 100644
Quoted 8 lines

drivers/rtc/rtc-mcp795.c static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count) · line 85

Quoted 8 lines
drivers/rtc/rtc-mcp795.c +1 -1
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
    {
    	struct spi_device *spi = to_spi_device(dev);
    	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];

For this particular case it seems to me that the following works just fine:

Quoted 9 lines
Quoted 6 lines

#define MAX_COUNT 7

u8 tx[MAX_COUNT + 2];

What do you think?

Nope, that RTC has a section of 255 bytes that could be read at once so 257 is the correct value.

I see...

I was looking into this piece of code drivers/rtc/rtc-mcp795.c:302:

         tmp[0] = (tmp[0] & 0x80) | bin2bcd(alm->time.tm_sec);
         tmp[1] = (tmp[1] & 0x80) | bin2bcd(alm->time.tm_min);
         tmp[2] = (tmp[2] & 0xE0) | bin2bcd(alm->time.tm_hour);
         tmp[3] = (tmp[3] & 0x80) | bin2bcd(alm->time.tm_wday + 1);

        /* set alarm match: seconds, minutes, hour, day, date and month */ tmp[3] |= (MCP795_ALM0C2_BIT | MCP795_ALM0C1_BIT |
MCP795_ALM0C0_BIT);
        tmp[4] = (tmp[4] & 0xC0) | bin2bcd(alm->time.tm_mday); tmp[5] = (tmp[5] & 0xE0) | bin2bcd(alm->time.tm_mon + 1);

Quoted 6 lines
Quoted 3 lines

        ret = mcp795_rtcc_write(dev, MCP795_REG_ALM0_SECONDS, tmp,
sizeof(tmp));

Yeah, the 128 or 255 bytes NVRAM is not yet supported but this is on my todo list

I got it.

Thanks
--
Gustavo

#7 lore raw
In reply to Alexandre Belloni

Hi Alexandre,

Quoted 5 lines · Alexandre Belloni
Quoted 3 lines · Gustavo A. R. Silva

In preparation to enabling -Wvla, remove VLA and replace it with a fixed-length array instead.

You should probably explain what VLA is and why this is important to do.

Sure. I can elaborate a little bit more.

Quoted 6 lines
Quoted 6 lines
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  drivers/rtc/rtc-bq32k.c  | 2 +-
  drivers/rtc/rtc-mcp795.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)
Quoted 12 lines

drivers/rtc/rtc-bq32k.c static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len) · line 78

Quoted 11 lines
drivers/rtc/rtc-bq32k.c +1 -1
index e8698e9..e4b234a 100644
@@ -74,7 +74,7 @@ static int bq32k_read(struct device *dev, void *data, uint8_t off, uint8_t len)
  static int bq32k_write(struct device *dev, void *data, uint8_t off, uint8_t len)
  {
  	struct i2c_client *client = to_i2c_client(dev);
-	uint8_t buffer[len + 1];
+	uint8_t buffer[256];

You chose to change len to 255, probably because this is a uint8_t but

Correct.

Quoted 1 line

this is way too much for this rtc, it only has 10 consecutive registers.

In that case probably the best solution is to add the following line to the module:

#define MAX_LEN 10

and update the rest of the code as follows:

uint8_t buffer[MAX_LEN + 1];

Quoted 4 lines
Quoted 4 lines
drivers/rtc/rtc-bq32k.c +0 -0
  	buffer[0] = off;
  	memcpy(&buffer[1], data, len);
drivers/rtc/rtc-mcp795.c +0 -0
index 79e24ea..00e11c1 100644
Quoted 8 lines

drivers/rtc/rtc-mcp795.c static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count) · line 85

Quoted 8 lines
drivers/rtc/rtc-mcp795.c +1 -1
@@ -82,7 +82,7 @@ static int mcp795_rtcc_write(struct device *dev, u8 addr, u8 *data, u8 count)
  {
  	struct spi_device *spi = to_spi_device(dev);
  	int ret;
-	u8 tx[2 + count];
+	u8 tx[257];

For this particular case it seems to me that the following works just fine:

#define MAX_COUNT 7

u8 tx[MAX_COUNT + 2];

What do you think?

Thanks for your feedback.
--
Gustavo

Quoted 6 lines
Quoted 6 lines
drivers/rtc/rtc-mcp795.c +0 -0
  	tx[0] = MCP795_WRITE;
  	tx[1] = addr;
Signature · 1 line
2.7.4