[PATCH 1/3] rtc: ds1685: fix crash caused by referencing wrong device struct

[PATCH 1/3] rtc: ds1685: fix crash caused by referencing wrong device struct

[Thomas Bogendoerfer]

sysfs entries added by rtc_add_group are called with the rtc device
as argument and not the underlying device. Fixed by using the dev->parent

Fixes: cfb74916e2ec ("rtc: ds1685: use rtc_add_group")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/rtc/rtc-ds1685.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 2710f2594c42..2f5194df239e 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -1004,7 +1004,7 @@ static ssize_t
 ds1685_rtc_sysfs_battery_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
-	struct ds1685_priv *rtc = dev_get_drvdata(dev);
+	struct ds1685_priv *rtc = dev_get_drvdata(dev->parent);
 	u8 ctrld;
 
 	ctrld = rtc->read(rtc, RTC_CTRL_D);
@@ -1024,7 +1024,7 @@ static ssize_t
 ds1685_rtc_sysfs_auxbatt_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
-	struct ds1685_priv *rtc = dev_get_drvdata(dev);
+	struct ds1685_priv *rtc = dev_get_drvdata(dev->parent);
 	u8 ctrl4a;
 
 	ds1685_rtc_switch_to_bank1(rtc);
@@ -1046,7 +1046,7 @@ static ssize_t
 ds1685_rtc_sysfs_serial_show(struct device *dev,
 			     struct device_attribute *attr, char *buf)
 {
-	struct ds1685_priv *rtc = dev_get_drvdata(dev);
+	struct ds1685_priv *rtc = dev_get_drvdata(dev->parent);
 	u8 ssn[8];
 
 	ds1685_rtc_switch_to_bank1(rtc);
-- 
2.13.7

[PATCH 2/3] rtc: ds1685: use correct device struct to get platform device struct

[Thomas Bogendoerfer]

Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/rtc/rtc-ds1685.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 2f5194df239e..929f28375b87 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -655,7 +655,7 @@ ds1685_rtc_work_queue(struct work_struct *work)
 {
 	struct ds1685_priv *rtc = container_of(work,
 					       struct ds1685_priv, work);
-	struct platform_device *pdev = to_platform_device(&rtc->dev->dev);
+	struct platform_device *pdev = to_platform_device(rtc->dev->dev.parent);
 	struct mutex *rtc_mutex = &rtc->dev->ops_lock;
 	u8 ctrl4a, ctrl4b;
 
-- 
2.13.7

[PATCH 3/3] rtc: ds1685: disable interrupts when moving work to work queue

[Thomas Bogendoerfer]

Handling of extended interrupts (kickstart, wake-up, ram-clear) is
moved off to a work queue, but the interrupts aren't acknowledged
in the interrupt handler. This leads to a deadlock, if driver
is used with interrupts. To fix this we now disable in irq handler
and re-enable it after work queue is done.

Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
---
 drivers/rtc/rtc-ds1685.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 929f28375b87..5dabfa57bd2a 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -635,6 +635,7 @@ ds1685_rtc_irq_handler(int irq, void *dev_id)
 			 * to be minimized.  Schedule them into a workqueue
 			 * and inform the RTC core that the IRQs were handled.
 			 */
+			disable_irq_nosync(rtc->irq_num);
 			spin_unlock(&rtc->lock);
 			schedule_work(&rtc->work);
 			rtc_update_irq(rtc->dev, 0, 0);
@@ -741,6 +742,7 @@ ds1685_rtc_work_queue(struct work_struct *work)
 	ds1685_rtc_switch_to_bank0(rtc);
 
 	mutex_unlock(rtc_mutex);
+	enable_irq(rtc->irq_num);
 }
 /* ----------------------------------------------------------------------- */
 
-- 
2.13.7

Re: [PATCH 2/3] rtc: ds1685: use correct device struct to get platform device struct

[Alexandre Belloni]

Hi,

Every patch need a commit message. Maybe you could indicate that this
never gave any issue because parent is the first member of struct
device.

On 11/04/2019 16:33:22+0200, Thomas Bogendoerfer wrote:
> Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
>  drivers/rtc/rtc-ds1685.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
> index 2f5194df239e..929f28375b87 100644
> --- a/drivers/rtc/rtc-ds1685.c
> +++ b/drivers/rtc/rtc-ds1685.c
> @@ -655,7 +655,7 @@ ds1685_rtc_work_queue(struct work_struct *work)
>  {
>  	struct ds1685_priv *rtc = container_of(work,
>  					       struct ds1685_priv, work);
> -	struct platform_device *pdev = to_platform_device(&rtc->dev->dev);
> +	struct platform_device *pdev = to_platform_device(rtc->dev->dev.parent);
>  	struct mutex *rtc_mutex = &rtc->dev->ops_lock;
>  	u8 ctrl4a, ctrl4b;
>  
> -- 
> 2.13.7
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/3] rtc: ds1685: disable interrupts when moving work to work queue

[Alexandre Belloni]

On 11/04/2019 16:33:23+0200, Thomas Bogendoerfer wrote:
> Handling of extended interrupts (kickstart, wake-up, ram-clear) is
> moved off to a work queue, but the interrupts aren't acknowledged
> in the interrupt handler. This leads to a deadlock, if driver
> is used with interrupts. To fix this we now disable in irq handler
> and re-enable it after work queue is done.
> 

The correct fix to that seems to switch to a threaded interrupt handler.
Can you do that?

> Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
>  drivers/rtc/rtc-ds1685.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
> index 929f28375b87..5dabfa57bd2a 100644
> --- a/drivers/rtc/rtc-ds1685.c
> +++ b/drivers/rtc/rtc-ds1685.c
> @@ -635,6 +635,7 @@ ds1685_rtc_irq_handler(int irq, void *dev_id)
>  			 * to be minimized.  Schedule them into a workqueue
>  			 * and inform the RTC core that the IRQs were handled.
>  			 */
> +			disable_irq_nosync(rtc->irq_num);
>  			spin_unlock(&rtc->lock);
>  			schedule_work(&rtc->work);
>  			rtc_update_irq(rtc->dev, 0, 0);
> @@ -741,6 +742,7 @@ ds1685_rtc_work_queue(struct work_struct *work)
>  	ds1685_rtc_switch_to_bank0(rtc);
>  
>  	mutex_unlock(rtc_mutex);
> +	enable_irq(rtc->irq_num);
>  }
>  /* ----------------------------------------------------------------------- */
>  
> -- 
> 2.13.7
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 1/3] rtc: ds1685: fix crash caused by referencing wrong device struct

[Alexandre Belloni]

On 11/04/2019 16:33:21+0200, Thomas Bogendoerfer wrote:
> sysfs entries added by rtc_add_group are called with the rtc device
> as argument and not the underlying device. Fixed by using the dev->parent
> 
> Fixes: cfb74916e2ec ("rtc: ds1685: use rtc_add_group")
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
>  drivers/rtc/rtc-ds1685.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] rtc: ds1685: use correct device struct to get platform device struct

[Thomas Bogendoerfer]

On Fri, 12 Apr 2019 12:11:06 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> Every patch need a commit message. Maybe you could indicate that this
> never gave any issue because parent is the first member of struct
> device.

I'll update the commit message, I get a nice stacktrace because of that
bug, so the path from work_queue calling ds1685_rtc_poweroff never worked.

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/3] rtc: ds1685: disable interrupts when moving work to work queue

[Thomas Bogendoerfer]

On Fri, 12 Apr 2019 12:14:18 +0200
Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:

> On 11/04/2019 16:33:23+0200, Thomas Bogendoerfer wrote:
> > Handling of extended interrupts (kickstart, wake-up, ram-clear) is
> > moved off to a work queue, but the interrupts aren't acknowledged
> > in the interrupt handler. This leads to a deadlock, if driver
> > is used with interrupts. To fix this we now disable in irq handler
> > and re-enable it after work queue is done.
> > 
> 
> The correct fix to that seems to switch to a threaded interrupt handler.
> Can you do that?

sure, shouldn't be a big problem.

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/3] rtc: ds1685: use correct device struct to get platform device struct

[Joshua Kinard]

On 4/12/2019 07:44, Thomas Bogendoerfer wrote:
> On Fri, 12 Apr 2019 12:11:06 +0200
> Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> 
>> Every patch need a commit message. Maybe you could indicate that this
>> never gave any issue because parent is the first member of struct
>> device.
> 
> I'll update the commit message, I get a nice stacktrace because of that
> bug, so the path from work_queue calling ds1685_rtc_poweroff never worked.
> 
> Thomas.

I'll wager that's why the thing stopped powering off my Octane.  It *used*
to work when I wrote the driver, but stopped after some unidentified point,
and I never found the time to try and track it down.

Which machine are you testing on, out of curiosity?

-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
rsa6144/5C63F4E3F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943

"The past tempts us, the present confuses us, the future frightens us.  And
our lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [PATCH 2/3] rtc: ds1685: use correct device struct to get platform device struct

[Thomas Bogendoerfer]

On Sat, 13 Apr 2019 01:17:19 -0400
Joshua Kinard <kumba@gentoo.org> wrote:

> On 4/12/2019 07:44, Thomas Bogendoerfer wrote:
> > On Fri, 12 Apr 2019 12:11:06 +0200
> > Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> > 
> >> Every patch need a commit message. Maybe you could indicate that this
> >> never gave any issue because parent is the first member of struct
> >> device.
> > 
> > I'll update the commit message, I get a nice stacktrace because of that
> > bug, so the path from work_queue calling ds1685_rtc_poweroff never worked.
> > 
> > Thomas.
> 
> I'll wager that's why the thing stopped powering off my Octane.  It *used*
> to work when I wrote the driver, but stopped after some unidentified point,
> and I never found the time to try and track it down.

calling ds1685_rtc_poweroff with the correct platform device works, so the bug is
not in the poweroff function but in the work queue.

> Which machine are you testing on, out of curiosity?

SGI Octane but I'm not setting prepare_poweroff.

Thomas.

-- 
SUSE Linux GmbH
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)