Patchwork rtc-ds1302: handle write protection

login
register
mail settings
Submitter Sergey Yanovich
Date May 20, 2013, 11:21 p.m.
Message ID <1369092090-5384-1-git-send-email-ynvich@gmail.com>
Download mbox | patch
Permalink /patch/245140/
State New
Headers show

Comments

Sergey Yanovich - May 20, 2013, 11:21 p.m.
This chip has a control register and can prevent altering saved clock.
Without this patch we could have:
----8<----
(arm)root@pac14:~# date
Tue May 21 03:08:27 MSK 2013
(arm)root@pac14:~# /etc/init.d/hwclock.sh show
Tue May 21 11:13:58 2013  -0.067322 seconds
(arm)root@pac14:~# /etc/init.d/hwclock.sh stop
[info] Saving the system clock.
[info] Hardware Clock updated to Tue May 21 03:09:01 MSK 2013.
(arm)root@pac14:~# /etc/init.d/hwclock.sh show
Tue May 21 11:14:15 2013  -0.624272 seconds
----8<----

Signed-off-by: Sergey Yanovich <ynvich@gmail.com>
---
 drivers/rtc/rtc-ds1302.c |    6 ++++++
 1 file changed, 6 insertions(+)
Andrew Morton - May 29, 2013, 10:53 p.m.
On Tue, 21 May 2013 03:21:30 +0400 Sergey Yanovich <ynvich@gmail.com> wrote:

> This chip has a control register and can prevent altering saved clock.
> Without this patch we could have:
> ----8<----
> (arm)root@pac14:~# date
> Tue May 21 03:08:27 MSK 2013
> (arm)root@pac14:~# /etc/init.d/hwclock.sh show
> Tue May 21 11:13:58 2013  -0.067322 seconds
> (arm)root@pac14:~# /etc/init.d/hwclock.sh stop
> [info] Saving the system clock.
> [info] Hardware Clock updated to Tue May 21 03:09:01 MSK 2013.
> (arm)root@pac14:~# /etc/init.d/hwclock.sh show
> Tue May 21 11:14:15 2013  -0.624272 seconds
> ----8<----
>
> ...
>
> --- a/drivers/rtc/rtc-ds1302.c
> +++ b/drivers/rtc/rtc-ds1302.c
> @@ -23,8 +23,12 @@
>  #define	RTC_CMD_READ	0x81		/* Read command */
>  #define	RTC_CMD_WRITE	0x80		/* Write command */
>  
> +#define	RTC_CMD_WRITE_ENABLE	0x00		/* Write enable */
> +#define	RTC_CMD_WRITE_DISABLE	0x80		/* Write disable */
> +
>  #define RTC_ADDR_RAM0	0x20		/* Address of RAM0 */
>  #define RTC_ADDR_TCR	0x08		/* Address of trickle charge register */
> +#define	RTC_ADDR_CTRL	0x07		/* Address of control register */
>  #define	RTC_ADDR_YEAR	0x06		/* Address of year register */
>  #define	RTC_ADDR_DAY	0x05		/* Address of day of week register */
>  #define	RTC_ADDR_MON	0x04		/* Address of month register */
> @@ -313,6 +317,7 @@ static int __init ds1302_rtc_probe(struct platform_device *pdev)
>  		return PTR_ERR(rtc);
>  
>  	platform_set_drvdata(pdev, rtc);
> +	ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_ENABLE);
>  
>  	return 0;
>  }
> @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device *pdev)
>  {
>  	struct rtc_device *rtc = platform_get_drvdata(pdev);
>  
> +	ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE);
>  	rtc_device_unregister(rtc);
>  	platform_set_drvdata(pdev, NULL);

ds1302_rtc_remove() no longer exists in my tree - it got whittled away
to nothing by
http://ozlabs.org/~akpm/mmots/broken-out/rtc-rtc-ds1302-remove-unnecessary-platform_set_drvdata.patch
and
http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-ds1302c-remove-empty-function.patch

Perhaps it should be re-added for this?
Sergey Yanovich - May 30, 2013, 10:14 a.m.
On Wed, 2013-05-29 at 15:53 -0700, Andrew Morton wrote:
> On Tue, 21 May 2013 03:21:30 +0400 Sergey Yanovich <ynvich@gmail.com> wrote:
> @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device *pdev)
> >  {
> >  	struct rtc_device *rtc = platform_get_drvdata(pdev);
> >  
> > +	ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE);
> >  	rtc_device_unregister(rtc);
> >  	platform_set_drvdata(pdev, NULL);
> 
> ds1302_rtc_remove() no longer exists in my tree - it got whittled away
> to nothing by
> http://ozlabs.org/~akpm/mmots/broken-out/rtc-rtc-ds1302-remove-unnecessary-platform_set_drvdata.patch
> and
> http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-ds1302c-remove-empty-function.patch
> 
> Perhaps it should be re-added for this?

There are 2 options. I would be happy with either.

1. I've chosen 'probe/remove' to enable/disable write access.

2. Another option is to wrap enable/disable around
ds1302_rtc_set_time().

IIUC, the former saves a few bytes of memory. However, now, when
ds1302_rtc_remove() is gone, the latter looks better. So I could rewrite
the patch either way.
Marc Zyngier - May 30, 2013, 10:20 a.m.
On Thu, 30 May 2013 14:14:42 +0400, Sergey Yanovich <ynvich@gmail.com>
wrote:
> On Wed, 2013-05-29 at 15:53 -0700, Andrew Morton wrote:
>> On Tue, 21 May 2013 03:21:30 +0400 Sergey Yanovich <ynvich@gmail.com>
>> wrote:
>> @@ -321,6 +326,7 @@ static int ds1302_rtc_remove(struct platform_device
>> *pdev)
>> >  {
>> >  	struct rtc_device *rtc = platform_get_drvdata(pdev);
>> >  
>> > +	ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE);
>> >  	rtc_device_unregister(rtc);
>> >  	platform_set_drvdata(pdev, NULL);
>> 
>> ds1302_rtc_remove() no longer exists in my tree - it got whittled away
>> to nothing by
>>
http://ozlabs.org/~akpm/mmots/broken-out/rtc-rtc-ds1302-remove-unnecessary-platform_set_drvdata.patch
>> and
>>
http://ozlabs.org/~akpm/mmots/broken-out/drivers-rtc-rtc-ds1302c-remove-empty-function.patch
>> 
>> Perhaps it should be re-added for this?
> 
> There are 2 options. I would be happy with either.
> 
> 1. I've chosen 'probe/remove' to enable/disable write access.
> 
> 2. Another option is to wrap enable/disable around
> ds1302_rtc_set_time().
> 
> IIUC, the former saves a few bytes of memory. However, now, when
> ds1302_rtc_remove() is gone, the latter looks better. So I could rewrite
> the patch either way.

Option two looks actually safer to me, as it ensures that an unexpected
reboot outside of the set_time section doesn't leave write access enabled.
You never know what firmware could do while you're not looking...

         M.

Patch

diff --git a/drivers/rtc/rtc-ds1302.c b/drivers/rtc/rtc-ds1302.c
index 73eafb9..626aec2 100644
--- a/drivers/rtc/rtc-ds1302.c
+++ b/drivers/rtc/rtc-ds1302.c
@@ -23,8 +23,12 @@ 
 #define	RTC_CMD_READ	0x81		/* Read command */
 #define	RTC_CMD_WRITE	0x80		/* Write command */
 
+#define	RTC_CMD_WRITE_ENABLE	0x00		/* Write enable */
+#define	RTC_CMD_WRITE_DISABLE	0x80		/* Write disable */
+
 #define RTC_ADDR_RAM0	0x20		/* Address of RAM0 */
 #define RTC_ADDR_TCR	0x08		/* Address of trickle charge register */
+#define	RTC_ADDR_CTRL	0x07		/* Address of control register */
 #define	RTC_ADDR_YEAR	0x06		/* Address of year register */
 #define	RTC_ADDR_DAY	0x05		/* Address of day of week register */
 #define	RTC_ADDR_MON	0x04		/* Address of month register */
@@ -313,6 +317,7 @@  static int __init ds1302_rtc_probe(struct platform_device *pdev)
 		return PTR_ERR(rtc);
 
 	platform_set_drvdata(pdev, rtc);
+	ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_ENABLE);
 
 	return 0;
 }
@@ -321,6 +326,7 @@  static int ds1302_rtc_remove(struct platform_device *pdev)
 {
 	struct rtc_device *rtc = platform_get_drvdata(pdev);
 
+	ds1302_writebyte(RTC_ADDR_CTRL, RTC_CMD_WRITE_DISABLE);
 	rtc_device_unregister(rtc);
 	platform_set_drvdata(pdev, NULL);