linux-kernel - Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 8 Apr 2019 11:51:15 +0300
From:   Nandor Han <nandor.han@...sala.com>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] rtc: ds3232: get SRAM access using NVMEM Framework

On 4/6/19 12:44 AM, Alexandre Belloni wrote:
> Hi,
> 
> On 05/04/2019 11:14:35+0000, Han Nandor wrote:

<snip>

>> `
>> # hexdump -n 10 -C /sys/bus/nvmem/devices/ds3232_sram0/nvmem
>> 00000000  74 65 73 74 69 6e 67 0a  00 00                    |testing...|
>> 0000000a
>> `
>>
> 
> Thanks for that nice description!
> 

Glad that you like it. :)


>>   drivers/rtc/rtc-ds3232.c | 41 ++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rtc/rtc-ds3232.c b/drivers/rtc/rtc-ds3232.c
>> index 7184e5145f12..fe615aedaa9a 100644
>> --- a/drivers/rtc/rtc-ds3232.c
>> +++ b/drivers/rtc/rtc-ds3232.c
>> @@ -24,6 +24,8 @@
>>   #include <linux/regmap.h>
>>   #include <linux/hwmon.h>
>>   
>> +#include "rtc-core.h"
>> +
> 
> The drivers should not have to include that header, see
> fd5cd21d995e67f87b3eb4adf938be85fe83ef4b (from 4.16).
> 

Ahh...right. I did the implementation on 4.14 and it was needed there.
I will change the implementation but I'll not be able to fully test it.

<snip>

>>   struct ds3232 {
>>   	struct device *dev;
>>   	struct regmap *regmap;
>>   	int irq;
>>   	struct rtc_device *rtc;
>> +	struct nvmem_config	nvmem_cfg;
>>   
> 
> You don't actually need to keep that structure for the whole life of the
> device as it is copied as soon as it is registered. I usually prefer to
> have it on the stack.
> 

Very true. Done

<snip>

>> +	ds3232->nvmem_cfg.stride = 1;
>> +	ds3232->nvmem_cfg.size = DS3232_REG_SRAM_SIZE;
>> +	ds3232->nvmem_cfg.word_size = 1;
>> +	ds3232->nvmem_cfg.reg_read = ds3232_nvmem_read;
>> +	ds3232->nvmem_cfg.reg_write = ds3232_nvmem_write;
>> +	ds3232->nvmem_cfg.priv = ds3232;
> 
> Please also set the type (battery backed).
> 

Done.

<snip>

>>   
>> +	ds3232->rtc->dev.of_node = dev->of_node;
> 
> Please don't mess with rtc->dev.
> 

In my use-case I do use the device tree to declare a nvmem cell in this 
RTC node, which is used by another driver. Without this change finding 
the cell (using `devm_nvmem_cell_get` function) was failing because of 
missing `of_node` (again this was on 4.14).

I can do a bit of digging to see if something was changed, but I will 
really appreciate your advice, because I'm not be able to test the fix 
on linux-next.

>> +	ds3232->rtc->nvmem_config = &ds3232->nvmem_cfg;
>> +	rtc_nvmem_register(ds3232->rtc);
> 
> This part will not compile on v5.1-rc1.
> 

Sorry. I will fix this, and I will at least see that it builds on 
linux-next.

Thanks for review,
Nandor