All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] rtc: remove VLA usage
@ 2018-03-07 20:11 Gustavo A. R. Silva
  2018-03-07 21:25 ` Alexandre Belloni
  0 siblings, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-07 20:11 UTC (permalink / raw)
  To: Alessandro Zummo, Alexandre Belloni
  Cc: linux-rtc, linux-kernel, Gustavo A. R. Silva

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

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/rtc/rtc-bq32k.c  | 2 +-
 drivers/rtc/rtc-mcp795.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
index e8698e9..e4b234a 100644
--- a/drivers/rtc/rtc-bq32k.c
+++ b/drivers/rtc/rtc-bq32k.c
@@ -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);
diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
index 79e24ea..00e11c1 100644
--- a/drivers/rtc/rtc-mcp795.c
+++ b/drivers/rtc/rtc-mcp795.c
@@ -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;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 20:11 [PATCH] rtc: remove VLA usage Gustavo A. R. Silva
@ 2018-03-07 21:25 ` Alexandre Belloni
  2018-03-07 22:39   ` Gustavo A. R. Silva
  2018-03-07 22:40   ` Gustavo A. R. Silva
  0 siblings, 2 replies; 8+ messages in thread
From: Alexandre Belloni @ 2018-03-07 21:25 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Alessandro Zummo, linux-rtc, linux-kernel, Gustavo A. R. Silva

On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
> 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.

> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/rtc/rtc-bq32k.c  | 2 +-
>  drivers/rtc/rtc-mcp795.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
> index e8698e9..e4b234a 100644
> --- a/drivers/rtc/rtc-bq32k.c
> +++ b/drivers/rtc/rtc-bq32k.c
> @@ -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.

>  	buffer[0] = off;
>  	memcpy(&buffer[1], data, len);
> diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
> index 79e24ea..00e11c1 100644
> --- a/drivers/rtc/rtc-mcp795.c
> +++ b/drivers/rtc/rtc-mcp795.c
> @@ -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;
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 21:25 ` Alexandre Belloni
@ 2018-03-07 22:39   ` Gustavo A. R. Silva
  2018-03-07 23:01     ` Alexandre Belloni
  2018-03-07 22:40   ` Gustavo A. R. Silva
  1 sibling, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-07 22:39 UTC (permalink / raw)
  To: Alexandre Belloni, Gustavo A. R. Silva
  Cc: Alessandro Zummo, linux-rtc, linux-kernel

Hi Alexandre,

On 03/07/2018 03:25 PM, Alexandre Belloni wrote:
> On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
>> 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.

>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>   drivers/rtc/rtc-bq32k.c  | 2 +-
>>   drivers/rtc/rtc-mcp795.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
>> index e8698e9..e4b234a 100644
>> --- a/drivers/rtc/rtc-bq32k.c
>> +++ b/drivers/rtc/rtc-bq32k.c
>> @@ -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.

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


>>   	buffer[0] = off;
>>   	memcpy(&buffer[1], data, len);
>> diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
>> index 79e24ea..00e11c1 100644
>> --- a/drivers/rtc/rtc-mcp795.c
>> +++ b/drivers/rtc/rtc-mcp795.c
>> @@ -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

>>   
>>   	tx[0] = MCP795_WRITE;
>>   	tx[1] = addr;
>> -- 
>> 2.7.4
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 21:25 ` Alexandre Belloni
  2018-03-07 22:39   ` Gustavo A. R. Silva
@ 2018-03-07 22:40   ` Gustavo A. R. Silva
  1 sibling, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-07 22:40 UTC (permalink / raw)
  To: Alexandre Belloni, Gustavo A. R. Silva
  Cc: Alessandro Zummo, linux-rtc, linux-kernel

Hi Alexandre,

On 03/07/2018 03:25 PM, Alexandre Belloni wrote:
> On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
>> 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.

>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>   drivers/rtc/rtc-bq32k.c  | 2 +-
>>   drivers/rtc/rtc-mcp795.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
>> index e8698e9..e4b234a 100644
>> --- a/drivers/rtc/rtc-bq32k.c
>> +++ b/drivers/rtc/rtc-bq32k.c
>> @@ -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.

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


>>   	buffer[0] = off;
>>   	memcpy(&buffer[1], data, len);
>> diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
>> index 79e24ea..00e11c1 100644
>> --- a/drivers/rtc/rtc-mcp795.c
>> +++ b/drivers/rtc/rtc-mcp795.c
>> @@ -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

>>   
>>   	tx[0] = MCP795_WRITE;
>>   	tx[1] = addr;
>> -- 
>> 2.7.4
>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 22:39   ` Gustavo A. R. Silva
@ 2018-03-07 23:01     ` Alexandre Belloni
  2018-03-07 23:09       ` Gustavo A. R. Silva
  0 siblings, 1 reply; 8+ messages in thread
From: Alexandre Belloni @ 2018-03-07 23:01 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Gustavo A. R. Silva, Alessandro Zummo, linux-rtc, linux-kernel

On 07/03/2018 at 16:39:51 -0600, Gustavo A. R. Silva wrote:
> Hi Alexandre,
> 
> On 03/07/2018 03:25 PM, Alexandre Belloni wrote:
> > On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
> > > 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.
> 
> > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > > ---
> > >   drivers/rtc/rtc-bq32k.c  | 2 +-
> > >   drivers/rtc/rtc-mcp795.c | 2 +-
> > >   2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
> > > index e8698e9..e4b234a 100644
> > > --- a/drivers/rtc/rtc-bq32k.c
> > > +++ b/drivers/rtc/rtc-bq32k.c
> > > @@ -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.
> 
> > 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];
> 

Seems better, yes.

> 
> > >   	buffer[0] = off;
> > >   	memcpy(&buffer[1], data, len);
> > > diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
> > > index 79e24ea..00e11c1 100644
> > > --- a/drivers/rtc/rtc-mcp795.c
> > > +++ b/drivers/rtc/rtc-mcp795.c
> > > @@ -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?
> 

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


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 23:01     ` Alexandre Belloni
@ 2018-03-07 23:09       ` Gustavo A. R. Silva
  2018-03-07 23:12         ` Alexandre Belloni
  0 siblings, 1 reply; 8+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-07 23:09 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Gustavo A. R. Silva, Alessandro Zummo, linux-rtc, linux-kernel



On 03/07/2018 05:01 PM, Alexandre Belloni wrote:
> On 07/03/2018 at 16:39:51 -0600, Gustavo A. R. Silva wrote:
>> Hi Alexandre,
>>
>> On 03/07/2018 03:25 PM, Alexandre Belloni wrote:
>>> On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
>>>> 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.
>>
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>> ---
>>>>    drivers/rtc/rtc-bq32k.c  | 2 +-
>>>>    drivers/rtc/rtc-mcp795.c | 2 +-
>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
>>>> index e8698e9..e4b234a 100644
>>>> --- a/drivers/rtc/rtc-bq32k.c
>>>> +++ b/drivers/rtc/rtc-bq32k.c
>>>> @@ -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.
>>
>>> 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];
>>
> Seems better, yes.
>
>>>>    	buffer[0] = off;
>>>>    	memcpy(&buffer[1], data, len);
>>>> diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
>>>> index 79e24ea..00e11c1 100644
>>>> --- a/drivers/rtc/rtc-mcp795.c
>>>> +++ b/drivers/rtc/rtc-mcp795.c
>>>> @@ -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?
>>
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 23:09       ` Gustavo A. R. Silva
@ 2018-03-07 23:12         ` Alexandre Belloni
  2018-03-07 23:14           ` Gustavo A. R. Silva
  0 siblings, 1 reply; 8+ messages in thread
From: Alexandre Belloni @ 2018-03-07 23:12 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Gustavo A. R. Silva, Alessandro Zummo, linux-rtc, linux-kernel

On 07/03/2018 at 17:09:22 -0600, Gustavo A. R. Silva wrote:
> 
> 
> On 03/07/2018 05:01 PM, Alexandre Belloni wrote:
> > On 07/03/2018 at 16:39:51 -0600, Gustavo A. R. Silva wrote:
> > > Hi Alexandre,
> > > 
> > > On 03/07/2018 03:25 PM, Alexandre Belloni wrote:
> > > > On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
> > > > > 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.
> > > 
> > > > > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > > > > ---
> > > > >    drivers/rtc/rtc-bq32k.c  | 2 +-
> > > > >    drivers/rtc/rtc-mcp795.c | 2 +-
> > > > >    2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
> > > > > index e8698e9..e4b234a 100644
> > > > > --- a/drivers/rtc/rtc-bq32k.c
> > > > > +++ b/drivers/rtc/rtc-bq32k.c
> > > > > @@ -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.
> > > 
> > > > 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];
> > > 
> > Seems better, yes.
> > 
> > > > >    	buffer[0] = off;
> > > > >    	memcpy(&buffer[1], data, len);
> > > > > diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
> > > > > index 79e24ea..00e11c1 100644
> > > > > --- a/drivers/rtc/rtc-mcp795.c
> > > > > +++ b/drivers/rtc/rtc-mcp795.c
> > > > > @@ -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?
> > > 
> > 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));
> 

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

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] rtc: remove VLA usage
  2018-03-07 23:12         ` Alexandre Belloni
@ 2018-03-07 23:14           ` Gustavo A. R. Silva
  0 siblings, 0 replies; 8+ messages in thread
From: Gustavo A. R. Silva @ 2018-03-07 23:14 UTC (permalink / raw)
  To: Alexandre Belloni
  Cc: Gustavo A. R. Silva, Alessandro Zummo, linux-rtc, linux-kernel



On 03/07/2018 05:12 PM, Alexandre Belloni wrote:
> On 07/03/2018 at 17:09:22 -0600, Gustavo A. R. Silva wrote:
>>
>> On 03/07/2018 05:01 PM, Alexandre Belloni wrote:
>>> On 07/03/2018 at 16:39:51 -0600, Gustavo A. R. Silva wrote:
>>>> Hi Alexandre,
>>>>
>>>> On 03/07/2018 03:25 PM, Alexandre Belloni wrote:
>>>>> On 07/03/2018 at 14:11:33 -0600, Gustavo A. R. Silva wrote:
>>>>>> 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.
>>>>
>>>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>>>> ---
>>>>>>     drivers/rtc/rtc-bq32k.c  | 2 +-
>>>>>>     drivers/rtc/rtc-mcp795.c | 2 +-
>>>>>>     2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/rtc/rtc-bq32k.c b/drivers/rtc/rtc-bq32k.c
>>>>>> index e8698e9..e4b234a 100644
>>>>>> --- a/drivers/rtc/rtc-bq32k.c
>>>>>> +++ b/drivers/rtc/rtc-bq32k.c
>>>>>> @@ -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.
>>>>
>>>>> 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];
>>>>
>>> Seems better, yes.
>>>
>>>>>>     	buffer[0] = off;
>>>>>>     	memcpy(&buffer[1], data, len);
>>>>>> diff --git a/drivers/rtc/rtc-mcp795.c b/drivers/rtc/rtc-mcp795.c
>>>>>> index 79e24ea..00e11c1 100644
>>>>>> --- a/drivers/rtc/rtc-mcp795.c
>>>>>> +++ b/drivers/rtc/rtc-mcp795.c
>>>>>> @@ -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?
>>>>
>>> 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));
>>
> Yeah, the 128 or 255 bytes NVRAM is not yet supported but this is on my
> todo list
>

I got it.

Thanks
--
Gustavo

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-03-07 23:39 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-07 20:11 [PATCH] rtc: remove VLA usage Gustavo A. R. Silva
2018-03-07 21:25 ` Alexandre Belloni
2018-03-07 22:39   ` Gustavo A. R. Silva
2018-03-07 23:01     ` Alexandre Belloni
2018-03-07 23:09       ` Gustavo A. R. Silva
2018-03-07 23:12         ` Alexandre Belloni
2018-03-07 23:14           ` Gustavo A. R. Silva
2018-03-07 22:40   ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.