Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] watchdog: da9062: make restart handler atomic safe
@ 2020-01-15 16:23 Marco Felsch
  2020-01-15 16:54 ` Guenter Roeck
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Marco Felsch @ 2020-01-15 16:23 UTC (permalink / raw)
  To: support.opensource, linux, contact, Adam.Thomson.Opensource
  Cc: linux-watchdog, kernel, linux-kernel

The restart handler is executed during the shutdown phase which is
atomic/irq-less. The i2c framework supports atomic transfers since
commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic
transfers") to address this use case. Using regmap within an atomic
context is allowed only if the regmap type is MMIO and the cache type
'flat' or no cache is used. Using the i2c_smbus_write_byte_data()
function can be done without additional tests because:
 1) the DA9062 is an i2c-only device and
 2) the i2c framework emulates the smbus protocol if the host adapter
    does not support smbus_xfer by using the master_xfer.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Hi,

This patch is based on Stefan Lengfeld's RFC Patch [1].

[1] https://patchwork.ozlabs.org/patch/1085942/

Changes:

v2:
- adapt commit message
- add comment
- make use of i2c_smbus_write_byte_data()
---
 drivers/watchdog/da9062_wdt.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
index c9b9d6394525..77b6b5336067 100644
--- a/drivers/watchdog/da9062_wdt.c
+++ b/drivers/watchdog/da9062_wdt.c
@@ -11,6 +11,7 @@
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
+#include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/jiffies.h>
 #include <linux/mfd/da9062/registers.h>
@@ -149,12 +150,13 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
 			      void *data)
 {
 	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
+	struct i2c_client *client = to_i2c_client(wdt->hw->dev);
 	int ret;
 
-	ret = regmap_write(wdt->hw->regmap,
-			   DA9062AA_CONTROL_F,
-			   DA9062AA_SHUTDOWN_MASK);
-	if (ret)
+	/* Don't use regmap because it is not atomic safe */
+	ret = i2c_smbus_write_byte_data(client, DA9062AA_CONTROL_F,
+					DA9062AA_SHUTDOWN_MASK);
+	if (ret < 0)
 		dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n",
 			  ret);
 
-- 
2.20.1


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

* Re: [PATCH v2] watchdog: da9062: make restart handler atomic safe
  2020-01-15 16:23 [PATCH v2] watchdog: da9062: make restart handler atomic safe Marco Felsch
@ 2020-01-15 16:54 ` Guenter Roeck
  2020-01-16  7:23 ` Stefan Lengfeld
  2020-01-23 15:03 ` Adam Thomson
  2 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2020-01-15 16:54 UTC (permalink / raw)
  To: Marco Felsch
  Cc: support.opensource, contact, Adam.Thomson.Opensource,
	linux-watchdog, kernel, linux-kernel

On Wed, Jan 15, 2020 at 05:23:07PM +0100, Marco Felsch wrote:
> The restart handler is executed during the shutdown phase which is
> atomic/irq-less. The i2c framework supports atomic transfers since
> commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic
> transfers") to address this use case. Using regmap within an atomic
> context is allowed only if the regmap type is MMIO and the cache type
> 'flat' or no cache is used. Using the i2c_smbus_write_byte_data()
> function can be done without additional tests because:
>  1) the DA9062 is an i2c-only device and
>  2) the i2c framework emulates the smbus protocol if the host adapter
>     does not support smbus_xfer by using the master_xfer.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Hi,
> 
> This patch is based on Stefan Lengfeld's RFC Patch [1].
> 
> [1] https://patchwork.ozlabs.org/patch/1085942/
> 
> Changes:
> 
> v2:
> - adapt commit message
> - add comment
> - make use of i2c_smbus_write_byte_data()
> ---
>  drivers/watchdog/da9062_wdt.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index c9b9d6394525..77b6b5336067 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -11,6 +11,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> +#include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/jiffies.h>
>  #include <linux/mfd/da9062/registers.h>
> @@ -149,12 +150,13 @@ static int da9062_wdt_restart(struct watchdog_device *wdd, unsigned long action,
>  			      void *data)
>  {
>  	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	struct i2c_client *client = to_i2c_client(wdt->hw->dev);
>  	int ret;
>  
> -	ret = regmap_write(wdt->hw->regmap,
> -			   DA9062AA_CONTROL_F,
> -			   DA9062AA_SHUTDOWN_MASK);
> -	if (ret)
> +	/* Don't use regmap because it is not atomic safe */
> +	ret = i2c_smbus_write_byte_data(client, DA9062AA_CONTROL_F,
> +					DA9062AA_SHUTDOWN_MASK);
> +	if (ret < 0)
>  		dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n",
>  			  ret);
>  
> -- 
> 2.20.1
> 

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

* Re: [PATCH v2] watchdog: da9062: make restart handler atomic safe
  2020-01-15 16:23 [PATCH v2] watchdog: da9062: make restart handler atomic safe Marco Felsch
  2020-01-15 16:54 ` Guenter Roeck
@ 2020-01-16  7:23 ` Stefan Lengfeld
  2020-01-19 11:34   ` Stefan Lengfeld
  2020-01-23 15:03 ` Adam Thomson
  2 siblings, 1 reply; 5+ messages in thread
From: Stefan Lengfeld @ 2020-01-16  7:23 UTC (permalink / raw)
  To: Marco Felsch
  Cc: support.opensource, linux, Adam.Thomson.Opensource,
	linux-watchdog, kernel, linux-kernel

Hi Marco,

On Wed, Jan 15, 2020 at 05:23:07PM +0100, Marco Felsch wrote:
> The restart handler is executed during the shutdown phase which is
> atomic/irq-less. The i2c framework supports atomic transfers since
> commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic
> transfers") to address this use case. Using regmap within an atomic
> context is allowed only if the regmap type is MMIO and the cache type
> 'flat' or no cache is used. Using the i2c_smbus_write_byte_data()
> function can be done without additional tests because:
>  1) the DA9062 is an i2c-only device and
>  2) the i2c framework emulates the smbus protocol if the host adapter
>     does not support smbus_xfer by using the master_xfer.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu>


On Wed, Marco Flesch wrote [1]:
> I will send a v2 to cover Guenter's suggestion. Can I keep your reviewed
> by tag?

Yes, you can keep it. See above. I also checked by reading the code that
'i2c_smbus_write_byte_data' behaves the same as 'i2c_transfer'. Just
with some indirections.

Kind regards,
    Stefan

[1]: https://www.spinics.net/lists/linux-watchdog/msg17194.html

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

* Re: [PATCH v2] watchdog: da9062: make restart handler atomic safe
  2020-01-16  7:23 ` Stefan Lengfeld
@ 2020-01-19 11:34   ` Stefan Lengfeld
  0 siblings, 0 replies; 5+ messages in thread
From: Stefan Lengfeld @ 2020-01-19 11:34 UTC (permalink / raw)
  To: Marco Felsch
  Cc: support.opensource, linux, Adam.Thomson.Opensource,
	linux-watchdog, kernel, linux-kernel, Stefan Riedmueller

Hi Marco,

On Thu, Jan 16, 2020 at 08:23:20AM +0100, Stefan Lengfeld wrote:
> On Wed, Jan 15, 2020 at 05:23:07PM +0100, Marco Felsch wrote:
> > The restart handler is executed during the shutdown phase which is
> > atomic/irq-less. The i2c framework supports atomic transfers since
> > commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic
> > transfers") to address this use case. Using regmap within an atomic
> > context is allowed only if the regmap type is MMIO and the cache type
> > 'flat' or no cache is used. Using the i2c_smbus_write_byte_data()
> > function can be done without additional tests because:
> >  1) the DA9062 is an i2c-only device and
> >  2) the i2c framework emulates the smbus protocol if the host adapter
> >     does not support smbus_xfer by using the master_xfer.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> Reviewed-by: Stefan Lengfeld <contact@stefanchrist.eu>

Now also 

Tested-by: Stefan Lengfeld <contact@stefanchrist.eu>

on a phyCORE-i.MX6 Quad with baseboard phyBOARD-Mira.

I'm also cc'ing Stefan from phytec, since he is also interested in the
thread/patch [1].

Kind regards,
    Stefan

[1]: https://www.spinics.net/lists/linux-watchdog/msg17203.html

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

* RE: [PATCH v2] watchdog: da9062: make restart handler atomic safe
  2020-01-15 16:23 [PATCH v2] watchdog: da9062: make restart handler atomic safe Marco Felsch
  2020-01-15 16:54 ` Guenter Roeck
  2020-01-16  7:23 ` Stefan Lengfeld
@ 2020-01-23 15:03 ` Adam Thomson
  2 siblings, 0 replies; 5+ messages in thread
From: Adam Thomson @ 2020-01-23 15:03 UTC (permalink / raw)
  To: Marco Felsch, Support Opensource, linux, contact, Adam Thomson
  Cc: linux-watchdog, kernel, linux-kernel

On 15 January 2020 16:23, Marco Felsch wrote:

> The restart handler is executed during the shutdown phase which is
> atomic/irq-less. The i2c framework supports atomic transfers since
> commit 63b96983a5dd ("i2c: core: introduce callbacks for atomic
> transfers") to address this use case. Using regmap within an atomic
> context is allowed only if the regmap type is MMIO and the cache type
> 'flat' or no cache is used. Using the i2c_smbus_write_byte_data()
> function can be done without additional tests because:
>  1) the DA9062 is an i2c-only device and
>  2) the i2c framework emulates the smbus protocol if the host adapter
>     does not support smbus_xfer by using the master_xfer.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Reviewed-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>

> ---
> Hi,
> 
> This patch is based on Stefan Lengfeld's RFC Patch [1].
> 
> [1] https://patchwork.ozlabs.org/patch/1085942/
> 
> Changes:
> 
> v2:
> - adapt commit message
> - add comment
> - make use of i2c_smbus_write_byte_data()
> ---
>  drivers/watchdog/da9062_wdt.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/da9062_wdt.c b/drivers/watchdog/da9062_wdt.c
> index c9b9d6394525..77b6b5336067 100644
> --- a/drivers/watchdog/da9062_wdt.c
> +++ b/drivers/watchdog/da9062_wdt.c
> @@ -11,6 +11,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> +#include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/jiffies.h>
>  #include <linux/mfd/da9062/registers.h>
> @@ -149,12 +150,13 @@ static int da9062_wdt_restart(struct watchdog_device
> *wdd, unsigned long action,
>  			      void *data)
>  {
>  	struct da9062_watchdog *wdt = watchdog_get_drvdata(wdd);
> +	struct i2c_client *client = to_i2c_client(wdt->hw->dev);
>  	int ret;
> 
> -	ret = regmap_write(wdt->hw->regmap,
> -			   DA9062AA_CONTROL_F,
> -			   DA9062AA_SHUTDOWN_MASK);
> -	if (ret)
> +	/* Don't use regmap because it is not atomic safe */
> +	ret = i2c_smbus_write_byte_data(client, DA9062AA_CONTROL_F,
> +					DA9062AA_SHUTDOWN_MASK);
> +	if (ret < 0)
>  		dev_alert(wdt->hw->dev, "Failed to shutdown (err = %d)\n",
>  			  ret);
> 
> --
> 2.20.1


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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 16:23 [PATCH v2] watchdog: da9062: make restart handler atomic safe Marco Felsch
2020-01-15 16:54 ` Guenter Roeck
2020-01-16  7:23 ` Stefan Lengfeld
2020-01-19 11:34   ` Stefan Lengfeld
2020-01-23 15:03 ` Adam Thomson

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git