[PATCH v2 0/3] leds-pwm: Defer PWM calls if PWM can sleep

[PATCH v2 0/3] leds-pwm: Defer PWM calls if PWM can sleep

[Florian Vaussard]

Hello,

When using the leds-pwm module with external PWM chips connected through
I2C, the kernel will panic when settings a trigger. In this case, PWM
calls can sleep, and should be deferred.

Patch 1 and 2 add the necessary API to the PWM subsystem, and update
matching drivers. Patch 3 updates leds-pwm to use a worker if the PWM
chip can sleep, or performs direct calls otherwise.

This patchset is based on the for-next branch of the led subsystem [1],
as patch 3 depends on the recent work of Peter for DT bindings in leds-pwm.
The pwm patches should probably follow the same path.

Tested on Overo (OMAP3 + TWL4030) with device tree.

Best regards,

Florian

[1] git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git


Changes from v1:
* pwm_can_sleep -> pwm_cansleep
* avoid duplicated code between the worker and the direct call
* cache the value of pwm_cansleep inside leds-pwm to avoid API calls


Florian Vaussard (3):
  pwm: Add pwm_cansleep() as exported API to users
  pwm: Add can_sleep property to drivers
  leds: leds-pwm: Defer led_pwm_set() if PWM can sleep

 drivers/leds/leds-pwm.c   |   50 +++++++++++++++++++++++++++++++++++++-------
 drivers/pwm/core.c        |   12 ++++++++++
 drivers/pwm/pwm-twl-led.c |    1 +
 drivers/pwm/pwm-twl.c     |    1 +
 include/linux/pwm.h       |   10 +++++++++
 5 files changed, 66 insertions(+), 8 deletions(-)

-- 
1.7.5.4

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Florian Vaussard]

Calls to some external PWM chips can sleep. To help users,
add pwm_cansleep() API.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/pwm/core.c  |   12 ++++++++++++
 include/linux/pwm.h |   10 ++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4a13da4..e737f5f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(devm_pwm_put);
 
+/**
+  * pwm_cansleep() - report whether pwm access will sleep
+  * @pwm: PWM device
+  *
+  * It returns nonzero if accessing the PWM can sleep.
+  */
+int pwm_cansleep(struct pwm_device *pwm)
+{
+	return pwm->chip->can_sleep;
+}
+EXPORT_SYMBOL_GPL(pwm_cansleep);
+
 #ifdef CONFIG_DEBUG_FS
 static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 {
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 70655a2..e2cb5c7 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -146,6 +146,8 @@ struct pwm_ops {
  * @base: number of first PWM controlled by this chip
  * @npwm: number of PWMs controlled by this chip
  * @pwms: array of PWM devices allocated by the framework
+ * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
+ *      as they must while accessing PWM chips over I2C or SPI
  */
 struct pwm_chip {
 	struct device		*dev;
@@ -159,6 +161,7 @@ struct pwm_chip {
 	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
 					    const struct of_phandle_args *args);
 	unsigned int		of_pwm_n_cells;
+	unsigned int		can_sleep:1;
 };
 
 #if IS_ENABLED(CONFIG_PWM)
@@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
 				   const char *con_id);
 void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
+
+int pwm_cansleep(struct pwm_device *pwm);
 #else
 static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
 {
@@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
 static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 {
 }
+
+static inline int pwm_cansleep(struct pwm_device *pwm)
+{
+	return 0;
+}
 #endif
 
 struct pwm_lookup {
-- 
1.7.5.4

[PATCH v2 2/3] pwm: Add can_sleep property to drivers

[Florian Vaussard]

Calls to PWM drivers connected through I2C can sleep.
Use the new can_sleep property.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/pwm/pwm-twl-led.c |    1 +
 drivers/pwm/pwm-twl.c     |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 9dfa0f3..6261193 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -300,6 +300,7 @@ static int twl_pwmled_probe(struct platform_device *pdev)
 
 	twl->chip.dev = &pdev->dev;
 	twl->chip.base = -1;
+	twl->chip.can_sleep = 1;
 
 	mutex_init(&twl->mutex);
 
diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
index e65db95..4e56cd8 100644
--- a/drivers/pwm/pwm-twl.c
+++ b/drivers/pwm/pwm-twl.c
@@ -315,6 +315,7 @@ static int twl_pwm_probe(struct platform_device *pdev)
 	twl->chip.dev = &pdev->dev;
 	twl->chip.base = -1;
 	twl->chip.npwm = 2;
+	twl->chip.can_sleep = 1;
 
 	mutex_init(&twl->mutex);
 
-- 
1.7.5.4

[PATCH v2 3/3] leds: leds-pwm: Defer led_pwm_set() if PWM can sleep

[Florian Vaussard]

Call to led_pwm_set() can happen inside atomic context, like triggers.
If the PWM call can sleep, defer using a worker.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/leds/leds-pwm.c |   50 +++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index a1ea5f6..70effa7 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -23,12 +23,16 @@
 #include <linux/pwm.h>
 #include <linux/leds_pwm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 
 struct led_pwm_data {
 	struct led_classdev	cdev;
 	struct pwm_device	*pwm;
+	struct work_struct	work;
 	unsigned int		active_low;
 	unsigned int		period;
+	int			duty;
+	unsigned		can_sleep:1;
 };
 
 struct led_pwm_priv {
@@ -36,6 +40,26 @@ struct led_pwm_priv {
 	struct led_pwm_data leds[0];
 };
 
+static void __led_pwm_set(struct led_pwm_data *led_dat)
+{
+	int new_duty = led_dat->duty;
+
+	pwm_config(led_dat->pwm, new_duty, led_dat->period);
+
+	if (new_duty == 0)
+		pwm_disable(led_dat->pwm);
+	else
+		pwm_enable(led_dat->pwm);
+}
+
+static void led_pwm_work(struct work_struct *work)
+{
+	struct led_pwm_data *led_dat =
+		container_of(work, struct led_pwm_data, work);
+
+	__led_pwm_set(led_dat);
+}
+
 static void led_pwm_set(struct led_classdev *led_cdev,
 	enum led_brightness brightness)
 {
@@ -44,13 +68,12 @@ static void led_pwm_set(struct led_classdev *led_cdev,
 	unsigned int max = led_dat->cdev.max_brightness;
 	unsigned int period =  led_dat->period;
 
-	if (brightness == 0) {
-		pwm_config(led_dat->pwm, 0, period);
-		pwm_disable(led_dat->pwm);
-	} else {
-		pwm_config(led_dat->pwm, brightness * period / max, period);
-		pwm_enable(led_dat->pwm);
-	}
+	led_dat->duty = brightness * period / max;
+
+	if (led_dat->can_sleep)
+		schedule_work(&led_dat->work);
+	else
+		__led_pwm_set(led_dat);
 }
 
 static inline size_t sizeof_pwm_leds_priv(int num_leds)
@@ -100,6 +123,10 @@ static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev)
 		led_dat->cdev.brightness = LED_OFF;
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 
+		led_dat->can_sleep = pwm_cansleep(led_dat->pwm);
+		if (led_dat->can_sleep)
+			INIT_WORK(&led_dat->work, led_pwm_work);
+
 		ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to register for %s\n",
@@ -153,6 +180,10 @@ static int led_pwm_probe(struct platform_device *pdev)
 			led_dat->cdev.max_brightness = cur_led->max_brightness;
 			led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 
+			led_dat->can_sleep = pwm_cansleep(led_dat->pwm);
+			if (led_dat->can_sleep)
+				INIT_WORK(&led_dat->work, led_pwm_work);
+
 			ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
 			if (ret < 0)
 				goto err;
@@ -180,8 +211,11 @@ static int led_pwm_remove(struct platform_device *pdev)
 	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
 	int i;
 
-	for (i = 0; i < priv->num_leds; i++)
+	for (i = 0; i < priv->num_leds; i++) {
 		led_classdev_unregister(&priv->leds[i].cdev);
+		if (priv->leds[i].can_sleep)
+			cancel_work_sync(&priv->leds[i].work);
+	}
 
 	return 0;
 }
-- 
1.7.5.4

[PATCH v2 2/3] pwm: Add can_sleep property to drivers

[Peter Ujfalusi]

On 01/25/2013 02:44 PM, Florian Vaussard wrote:
> Calls to PWM drivers connected through I2C can sleep.
> Use the new can_sleep property.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> ---
>  drivers/pwm/pwm-twl-led.c |    1 +
>  drivers/pwm/pwm-twl.c     |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
> index 9dfa0f3..6261193 100644
> --- a/drivers/pwm/pwm-twl-led.c
> +++ b/drivers/pwm/pwm-twl-led.c
> @@ -300,6 +300,7 @@ static int twl_pwmled_probe(struct platform_device *pdev)
>  
>  	twl->chip.dev = &pdev->dev;
>  	twl->chip.base = -1;
> +	twl->chip.can_sleep = 1;
>  
>  	mutex_init(&twl->mutex);
>  
> diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
> index e65db95..4e56cd8 100644
> --- a/drivers/pwm/pwm-twl.c
> +++ b/drivers/pwm/pwm-twl.c
> @@ -315,6 +315,7 @@ static int twl_pwm_probe(struct platform_device *pdev)
>  	twl->chip.dev = &pdev->dev;
>  	twl->chip.base = -1;
>  	twl->chip.npwm = 2;
> +	twl->chip.can_sleep = 1;
>  
>  	mutex_init(&twl->mutex);
>  
> 


-- 
P?ter

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Peter Ujfalusi]

On 01/25/2013 02:44 PM, Florian Vaussard wrote:
> Calls to some external PWM chips can sleep. To help users,
> add pwm_cansleep() API.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> ---
>  drivers/pwm/core.c  |   12 ++++++++++++
>  include/linux/pwm.h |   10 ++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4a13da4..e737f5f 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>  }
>  EXPORT_SYMBOL_GPL(devm_pwm_put);
>  
> +/**
> +  * pwm_cansleep() - report whether pwm access will sleep
> +  * @pwm: PWM device
> +  *
> +  * It returns nonzero if accessing the PWM can sleep.
> +  */
> +int pwm_cansleep(struct pwm_device *pwm)
> +{
> +	return pwm->chip->can_sleep;
> +}
> +EXPORT_SYMBOL_GPL(pwm_cansleep);
> +
>  #ifdef CONFIG_DEBUG_FS
>  static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  {
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 70655a2..e2cb5c7 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -146,6 +146,8 @@ struct pwm_ops {
>   * @base: number of first PWM controlled by this chip
>   * @npwm: number of PWMs controlled by this chip
>   * @pwms: array of PWM devices allocated by the framework
> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
> + *      as they must while accessing PWM chips over I2C or SPI
>   */
>  struct pwm_chip {
>  	struct device		*dev;
> @@ -159,6 +161,7 @@ struct pwm_chip {
>  	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>  					    const struct of_phandle_args *args);
>  	unsigned int		of_pwm_n_cells;
> +	unsigned int		can_sleep:1;
>  };
>  
>  #if IS_ENABLED(CONFIG_PWM)
> @@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
>  struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
>  				   const char *con_id);
>  void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
> +
> +int pwm_cansleep(struct pwm_device *pwm);
>  #else
>  static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
>  {
> @@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
>  static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>  {
>  }
> +
> +static inline int pwm_cansleep(struct pwm_device *pwm)
> +{
> +	return 0;
> +}
>  #endif
>  
>  struct pwm_lookup {
> 


-- 
P?ter

[PATCH v2 3/3] leds: leds-pwm: Defer led_pwm_set() if PWM can sleep

[Peter Ujfalusi]

On 01/25/2013 02:44 PM, Florian Vaussard wrote:
> Call to led_pwm_set() can happen inside atomic context, like triggers.
> If the PWM call can sleep, defer using a worker.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

> ---
>  drivers/leds/leds-pwm.c |   50 +++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index a1ea5f6..70effa7 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -23,12 +23,16 @@
>  #include <linux/pwm.h>
>  #include <linux/leds_pwm.h>
>  #include <linux/slab.h>
> +#include <linux/workqueue.h>
>  
>  struct led_pwm_data {
>  	struct led_classdev	cdev;
>  	struct pwm_device	*pwm;
> +	struct work_struct	work;
>  	unsigned int		active_low;
>  	unsigned int		period;
> +	int			duty;
> +	unsigned		can_sleep:1;
>  };
>  
>  struct led_pwm_priv {
> @@ -36,6 +40,26 @@ struct led_pwm_priv {
>  	struct led_pwm_data leds[0];
>  };
>  
> +static void __led_pwm_set(struct led_pwm_data *led_dat)
> +{
> +	int new_duty = led_dat->duty;
> +
> +	pwm_config(led_dat->pwm, new_duty, led_dat->period);
> +
> +	if (new_duty == 0)
> +		pwm_disable(led_dat->pwm);
> +	else
> +		pwm_enable(led_dat->pwm);
> +}
> +
> +static void led_pwm_work(struct work_struct *work)
> +{
> +	struct led_pwm_data *led_dat =
> +		container_of(work, struct led_pwm_data, work);
> +
> +	__led_pwm_set(led_dat);
> +}
> +
>  static void led_pwm_set(struct led_classdev *led_cdev,
>  	enum led_brightness brightness)
>  {
> @@ -44,13 +68,12 @@ static void led_pwm_set(struct led_classdev *led_cdev,
>  	unsigned int max = led_dat->cdev.max_brightness;
>  	unsigned int period =  led_dat->period;
>  
> -	if (brightness == 0) {
> -		pwm_config(led_dat->pwm, 0, period);
> -		pwm_disable(led_dat->pwm);
> -	} else {
> -		pwm_config(led_dat->pwm, brightness * period / max, period);
> -		pwm_enable(led_dat->pwm);
> -	}
> +	led_dat->duty = brightness * period / max;
> +
> +	if (led_dat->can_sleep)
> +		schedule_work(&led_dat->work);
> +	else
> +		__led_pwm_set(led_dat);
>  }
>  
>  static inline size_t sizeof_pwm_leds_priv(int num_leds)
> @@ -100,6 +123,10 @@ static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev)
>  		led_dat->cdev.brightness = LED_OFF;
>  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>  
> +		led_dat->can_sleep = pwm_cansleep(led_dat->pwm);
> +		if (led_dat->can_sleep)
> +			INIT_WORK(&led_dat->work, led_pwm_work);
> +
>  		ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "failed to register for %s\n",
> @@ -153,6 +180,10 @@ static int led_pwm_probe(struct platform_device *pdev)
>  			led_dat->cdev.max_brightness = cur_led->max_brightness;
>  			led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>  
> +			led_dat->can_sleep = pwm_cansleep(led_dat->pwm);
> +			if (led_dat->can_sleep)
> +				INIT_WORK(&led_dat->work, led_pwm_work);
> +
>  			ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
>  			if (ret < 0)
>  				goto err;
> @@ -180,8 +211,11 @@ static int led_pwm_remove(struct platform_device *pdev)
>  	struct led_pwm_priv *priv = platform_get_drvdata(pdev);
>  	int i;
>  
> -	for (i = 0; i < priv->num_leds; i++)
> +	for (i = 0; i < priv->num_leds; i++) {
>  		led_classdev_unregister(&priv->leds[i].cdev);
> +		if (priv->leds[i].can_sleep)
> +			cancel_work_sync(&priv->leds[i].work);
> +	}
>  
>  	return 0;
>  }
> 


-- 
P?ter

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Thierry Reding]

On Fri, Jan 25, 2013 at 02:44:29PM +0100, Florian Vaussard wrote:
> Calls to some external PWM chips can sleep. To help users,
> add pwm_cansleep() API.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
> ---
>  drivers/pwm/core.c  |   12 ++++++++++++
>  include/linux/pwm.h |   10 ++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4a13da4..e737f5f 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>  }
>  EXPORT_SYMBOL_GPL(devm_pwm_put);
>  
> +/**
> +  * pwm_cansleep() - report whether pwm access will sleep

"... whether PWM access..." please.

> +  * @pwm: PWM device
> +  *
> +  * It returns nonzero if accessing the PWM can sleep.
> +  */
> +int pwm_cansleep(struct pwm_device *pwm)

I actually liked pwm_can_sleep() better. I find it to be more consistent
with the naming of other function names. It would furthermore match the
field name.

> +{
> +	return pwm->chip->can_sleep;
> +}
> +EXPORT_SYMBOL_GPL(pwm_cansleep);

Would it make sense to check for NULL pointers here? I guess that
passing NULL into the function could be considered a programming error
and an oops would be okay, but in that case there's no point in making
the function return an int. Also see my next comment.

> +
>  #ifdef CONFIG_DEBUG_FS
>  static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  {
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 70655a2..e2cb5c7 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -146,6 +146,8 @@ struct pwm_ops {
>   * @base: number of first PWM controlled by this chip
>   * @npwm: number of PWMs controlled by this chip
>   * @pwms: array of PWM devices allocated by the framework
> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
> + *      as they must while accessing PWM chips over I2C or SPI
>   */
>  struct pwm_chip {
>  	struct device		*dev;
> @@ -159,6 +161,7 @@ struct pwm_chip {
>  	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>  					    const struct of_phandle_args *args);
>  	unsigned int		of_pwm_n_cells;
> +	unsigned int		can_sleep:1;

What's the reason for making this a bitfield? Couldn't we just use a
bool instead?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130126/dc808821/attachment.sig>

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Peter Ujfalusi]

hi Thierry,

On 01/26/2013 06:40 AM, Thierry Reding wrote:
>> +  * @pwm: PWM device
>> +  *
>> +  * It returns nonzero if accessing the PWM can sleep.
>> +  */
>> +int pwm_cansleep(struct pwm_device *pwm)
> 
> I actually liked pwm_can_sleep() better. I find it to be more consistent
> with the naming of other function names. It would furthermore match the
> field name.

I was looking at the GPIO API to suggest this name change, but you are right
we should be consistent with the PWM API here.
Sorry Florian.

> 
>> +{
>> +	return pwm->chip->can_sleep;
>> +}
>> +EXPORT_SYMBOL_GPL(pwm_cansleep);
> 
> Would it make sense to check for NULL pointers here? I guess that
> passing NULL into the function could be considered a programming error
> and an oops would be okay, but in that case there's no point in making
> the function return an int. Also see my next comment.

While it is unlikely to happen it is better to be safe, something like this
will do:

return pwm ? pwm->chip->can_sleep : 0;

> 
>> +
>>  #ifdef CONFIG_DEBUG_FS
>>  static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>>  {
>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>> index 70655a2..e2cb5c7 100644
>> --- a/include/linux/pwm.h
>> +++ b/include/linux/pwm.h
>> @@ -146,6 +146,8 @@ struct pwm_ops {
>>   * @base: number of first PWM controlled by this chip
>>   * @npwm: number of PWMs controlled by this chip
>>   * @pwms: array of PWM devices allocated by the framework
>> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
>> + *      as they must while accessing PWM chips over I2C or SPI
>>   */
>>  struct pwm_chip {
>>  	struct device		*dev;
>> @@ -159,6 +161,7 @@ struct pwm_chip {
>>  	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>>  					    const struct of_phandle_args *args);
>>  	unsigned int		of_pwm_n_cells;
>> +	unsigned int		can_sleep:1;
> 
> What's the reason for making this a bitfield? Couldn't we just use a
> bool instead?

I have also overlooked this. In my version I had the can_sleep as bool also.

-- 
P?ter

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Florian Vaussard]

Hello,

Le 28/01/2013 09:45, Peter Ujfalusi a ?crit :
> hi Thierry,
>
> On 01/26/2013 06:40 AM, Thierry Reding wrote:
>>> +  * @pwm: PWM device
>>> +  *
>>> +  * It returns nonzero if accessing the PWM can sleep.
>>> +  */
>>> +int pwm_cansleep(struct pwm_device *pwm)
>>
>> I actually liked pwm_can_sleep() better. I find it to be more consistent
>> with the naming of other function names. It would furthermore match the
>> field name.
>
> I was looking at the GPIO API to suggest this name change, but you are right
> we should be consistent with the PWM API here.
> Sorry Florian.
>

No problem, I agree with the PWM API consistency.

>>
>>> +{
>>> +	return pwm->chip->can_sleep;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pwm_cansleep);
>>
>> Would it make sense to check for NULL pointers here? I guess that
>> passing NULL into the function could be considered a programming error
>> and an oops would be okay, but in that case there's no point in making
>> the function return an int. Also see my next comment.
>
> While it is unlikely to happen it is better to be safe, something like this
> will do:
>
> return pwm ? pwm->chip->can_sleep : 0;
>

Ok. And what about:

BUG_ON(pwm == NULL);
return pwm->chip->can_sleep;

>>
>>> +
>>>   #ifdef CONFIG_DEBUG_FS
>>>   static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>>>   {
>>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>>> index 70655a2..e2cb5c7 100644
>>> --- a/include/linux/pwm.h
>>> +++ b/include/linux/pwm.h
>>> @@ -146,6 +146,8 @@ struct pwm_ops {
>>>    * @base: number of first PWM controlled by this chip
>>>    * @npwm: number of PWMs controlled by this chip
>>>    * @pwms: array of PWM devices allocated by the framework
>>> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
>>> + *      as they must while accessing PWM chips over I2C or SPI
>>>    */
>>>   struct pwm_chip {
>>>   	struct device		*dev;
>>> @@ -159,6 +161,7 @@ struct pwm_chip {
>>>   	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>>>   					    const struct of_phandle_args *args);
>>>   	unsigned int		of_pwm_n_cells;
>>> +	unsigned int		can_sleep:1;
>>
>> What's the reason for making this a bitfield? Couldn't we just use a
>> bool instead?
>
> I have also overlooked this. In my version I had the can_sleep as bool also.
>

Ok for a bool.

Thank you for your reviews. I will send a v3 sometimes today.

Cheers,
Florian

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Thierry Reding]

On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote:
> Hello,
> 
> Le 28/01/2013 09:45, Peter Ujfalusi a ?crit :
> >hi Thierry,
> >
> >On 01/26/2013 06:40 AM, Thierry Reding wrote:
[...]
> >>>+{
> >>>+	return pwm->chip->can_sleep;
> >>>+}
> >>>+EXPORT_SYMBOL_GPL(pwm_cansleep);
> >>
> >>Would it make sense to check for NULL pointers here? I guess that
> >>passing NULL into the function could be considered a programming error
> >>and an oops would be okay, but in that case there's no point in making
> >>the function return an int. Also see my next comment.
> >
> >While it is unlikely to happen it is better to be safe, something like this
> >will do:
> >
> >return pwm ? pwm->chip->can_sleep : 0;
> >
> 
> Ok. And what about:
> 
> BUG_ON(pwm == NULL);
> return pwm->chip->can_sleep;

I don't think we need that. In case pwm == NULL, dereferencing it will
oops anyway. So either we make it safe and return an error code, or we
let it oops without explicit BUG_ON().

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130128/4f4439e5/attachment-0001.sig>

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Florian Vaussard]

Le 28/01/2013 10:57, Thierry Reding a ?crit :
> On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote:
>> Hello,
>>
>> Le 28/01/2013 09:45, Peter Ujfalusi a ?crit :
>>> hi Thierry,
>>>
>>> On 01/26/2013 06:40 AM, Thierry Reding wrote:
> [...]
>>>>> +{
>>>>> +	return pwm->chip->can_sleep;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(pwm_cansleep);
>>>>
>>>> Would it make sense to check for NULL pointers here? I guess that
>>>> passing NULL into the function could be considered a programming error
>>>> and an oops would be okay, but in that case there's no point in making
>>>> the function return an int. Also see my next comment.
>>>
>>> While it is unlikely to happen it is better to be safe, something like this
>>> will do:
>>>
>>> return pwm ? pwm->chip->can_sleep : 0;
>>>
>>
>> Ok. And what about:
>>
>> BUG_ON(pwm == NULL);
>> return pwm->chip->can_sleep;
>
> I don't think we need that. In case pwm == NULL, dereferencing it will
> oops anyway. So either we make it safe and return an error code, or we
> let it oops without explicit BUG_ON().
>

Calling this function with a NULL pointer is a programming error, so there
is no error codes for such errors. I propose to return bool, and let it
oops if such case happens.

Regards,

Florian

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Thierry Reding]

On Mon, Jan 28, 2013 at 11:57:39AM +0100, Florian Vaussard wrote:
> Le 28/01/2013 10:57, Thierry Reding a ?crit :
> >On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote:
> >>Hello,
> >>
> >>Le 28/01/2013 09:45, Peter Ujfalusi a ?crit :
> >>>hi Thierry,
> >>>
> >>>On 01/26/2013 06:40 AM, Thierry Reding wrote:
> >[...]
> >>>>>+{
> >>>>>+	return pwm->chip->can_sleep;
> >>>>>+}
> >>>>>+EXPORT_SYMBOL_GPL(pwm_cansleep);
> >>>>
> >>>>Would it make sense to check for NULL pointers here? I guess that
> >>>>passing NULL into the function could be considered a programming error
> >>>>and an oops would be okay, but in that case there's no point in making
> >>>>the function return an int. Also see my next comment.
> >>>
> >>>While it is unlikely to happen it is better to be safe, something like this
> >>>will do:
> >>>
> >>>return pwm ? pwm->chip->can_sleep : 0;
> >>>
> >>
> >>Ok. And what about:
> >>
> >>BUG_ON(pwm == NULL);
> >>return pwm->chip->can_sleep;
> >
> >I don't think we need that. In case pwm == NULL, dereferencing it will
> >oops anyway. So either we make it safe and return an error code, or we
> >let it oops without explicit BUG_ON().
> >
> 
> Calling this function with a NULL pointer is a programming error, so there
> is no error codes for such errors.

You could return -EINVAL if pwm == NULL.

> I propose to return bool, and let it oops if such case happens.

My point was that it will oops even if you don't use BUG_ON() so there
isn't so much point in using it explicitly.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130128/a79cde7f/attachment.sig>

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Russell King - ARM Linux]

On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote:
> Hello,
>
> Le 28/01/2013 09:45, Peter Ujfalusi a ?crit :
>> hi Thierry,
>>
>> On 01/26/2013 06:40 AM, Thierry Reding wrote:
>>>> +{
>>>> +	return pwm->chip->can_sleep;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(pwm_cansleep);
>>>
>>> Would it make sense to check for NULL pointers here? I guess that
>>> passing NULL into the function could be considered a programming error
>>> and an oops would be okay, but in that case there's no point in making
>>> the function return an int. Also see my next comment.
>>
>> While it is unlikely to happen it is better to be safe, something like this
>> will do:
>>
>> return pwm ? pwm->chip->can_sleep : 0;
>>
>
> Ok. And what about:
>
> BUG_ON(pwm == NULL);
> return pwm->chip->can_sleep;

Let's get something straight.

1. Don't use BUG_ON() as some kind of willy nilly assert() replacement.
   Linus refused to have assert() in the kernel because assert() gets not
   only over-used, but also gets inappropriately used too.

   _Only_ _ever_ use BUG_ON() if continuing is going to cause user
   noticable data loss which is not reportable to userspace.  In other
   words, block device queue corruption or the like - where bringing the
   system down is going to _save_ the system from itself.

   Otherwise, return an error and/or use WARN_ON().

2. If you want a slow kernel, then by all means check your arguments to
   your functions.  While you're at it, why not check that strings which
   are passed contain only the characters you expect them to?  And, if
   you're bothering to check against a NULL pointer, what about NULL+1
   pointers which are also invalid?  Why not invent some function to
   ensure that the pointer is a valid kernel pointer.  Maybe you'll have
   to interate the vmalloc lists too - yay, more code to be executed!
   That must be good!

In your example, if you're going to check that pwm is non-NULL, what
if pwm->chip is non-NULL?  How far do you take this?

Or... just like most of the core kernel does, it does _not_ verify on
function entry that the pointer is "correct" unless it is explicitly
defined that the function may take a NULL pointer (like kfree()).
Everything else just goes right on and does the dereference - and if
the pointer was wrong, we hope that the MMU faults and we get a kernel
oops.

Have a read through the code in fs/ or kernel/ and see how many functions
you can spot in there which validate their pointers which aren't dealing
with data from userland.

You'll find almost no function checking that an inode pointer is not NULL.
Or a struct file pointer.  Or a struct path pointer... etc.

Yet, you come to ARM code, and it seems "popular" that pointer arguments
need to be verified on every single function call.  Why is this?

I don't know if Andrew would like to inject something here (I've added
him) on this subject...

[PATCH v2 1/3] pwm: Add pwm_cansleep() as exported API to users

[Florian Vaussard]

Hello,

Le 28/01/2013 16:01, Russell King - ARM Linux a ?crit :
> On Mon, Jan 28, 2013 at 10:36:07AM +0100, Florian Vaussard wrote:
>> Hello,
>>
>> Le 28/01/2013 09:45, Peter Ujfalusi a ?crit :
>>> hi Thierry,
>>>
>>> On 01/26/2013 06:40 AM, Thierry Reding wrote:
>>>>> +{
>>>>> +	return pwm->chip->can_sleep;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(pwm_cansleep);
>>>>
>>>> Would it make sense to check for NULL pointers here? I guess that
>>>> passing NULL into the function could be considered a programming error
>>>> and an oops would be okay, but in that case there's no point in making
>>>> the function return an int. Also see my next comment.
>>>
>>> While it is unlikely to happen it is better to be safe, something like this
>>> will do:
>>>
>>> return pwm ? pwm->chip->can_sleep : 0;
>>>
>>
>> Ok. And what about:
>>
>> BUG_ON(pwm == NULL);
>> return pwm->chip->can_sleep;
>
> Let's get something straight.
>
> 1. Don't use BUG_ON() as some kind of willy nilly assert() replacement.
>     Linus refused to have assert() in the kernel because assert() gets not
>     only over-used, but also gets inappropriately used too.
>
>     _Only_ _ever_ use BUG_ON() if continuing is going to cause user
>     noticable data loss which is not reportable to userspace.  In other
>     words, block device queue corruption or the like - where bringing the
>     system down is going to _save_ the system from itself.
>
>     Otherwise, return an error and/or use WARN_ON().
>
> 2. If you want a slow kernel, then by all means check your arguments to
>     your functions.  While you're at it, why not check that strings which
>     are passed contain only the characters you expect them to?  And, if
>     you're bothering to check against a NULL pointer, what about NULL+1
>     pointers which are also invalid?  Why not invent some function to
>     ensure that the pointer is a valid kernel pointer.  Maybe you'll have
>     to interate the vmalloc lists too - yay, more code to be executed!
>     That must be good!
>
> In your example, if you're going to check that pwm is non-NULL, what
> if pwm->chip is non-NULL?  How far do you take this?
>
> Or... just like most of the core kernel does, it does _not_ verify on
> function entry that the pointer is "correct" unless it is explicitly
> defined that the function may take a NULL pointer (like kfree()).
> Everything else just goes right on and does the dereference - and if
> the pointer was wrong, we hope that the MMU faults and we get a kernel
> oops.
>
> Have a read through the code in fs/ or kernel/ and see how many functions
> you can spot in there which validate their pointers which aren't dealing
> with data from userland.
>
> You'll find almost no function checking that an inode pointer is not NULL.
> Or a struct file pointer.  Or a struct path pointer... etc.
>
> Yet, you come to ARM code, and it seems "popular" that pointer arguments
> need to be verified on every single function call.  Why is this?
>
> I don't know if Andrew would like to inject something here (I've added
> him) on this subject...
>

The v3 does not contain the check.

Thank you,

Florian