Commit 9cc5f232 authored by Sven Van Asbroeck's avatar Sven Van Asbroeck Committed by Thierry Reding

pwm: pca9685: Fix PWM/GPIO inter-operation

This driver allows pwms to be requested as gpios via gpiolib. Obviously,
it should not be allowed to request a GPIO when its corresponding PWM is
already requested (and vice versa). So it requires some exclusion code.

Given that the PWMm and GPIO cores are not synchronized with respect to
each other, this exclusion code will also require proper
synchronization.

Such a mechanism was in place, but was inadvertently removed by Uwe's
clean-up in commit e926b12c ("pwm: Clear chip_data in pwm_put()").

Upon revisiting the synchronization mechanism, we found that
theoretically, it could allow two threads to successfully request
conflicting PWMs/GPIOs.

Replace with a bitmap which tracks PWMs in-use, plus a mutex. As long as
PWM and GPIO's respective request/free functions modify the in-use
bitmap while holding the mutex, proper synchronization will be
guaranteed.
Reported-by: default avatarYueHaibing <yuehaibing@huawei.com>
Fixes: e926b12c ("pwm: Clear chip_data in pwm_put()")
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: YueHaibing <yuehaibing@huawei.com>
Link: https://lkml.org/lkml/2019/5/31/963Signed-off-by: default avatarSven Van Asbroeck <TheSven73@gmail.com>
Reviewed-by: default avatarMika Westerberg <mika.westerberg@linux.intel.com>
[cg: Tested on an i.MX6Q board with two NXP PCA9685 chips]
Tested-by: default avatarClemens Gruber <clemens.gruber@pqgruber.com>
Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com> # cg's rebase
Link: https://lore.kernel.org/lkml/20200330160238.GD2817345@ulmo/Signed-off-by: default avatarThierry Reding <thierry.reding@gmail.com>
parent 374c1104
...@@ -20,6 +20,7 @@ ...@@ -20,6 +20,7 @@
#include <linux/slab.h> #include <linux/slab.h>
#include <linux/delay.h> #include <linux/delay.h>
#include <linux/pm_runtime.h> #include <linux/pm_runtime.h>
#include <linux/bitmap.h>
/* /*
* Because the PCA9685 has only one prescaler per chip, changing the period of * Because the PCA9685 has only one prescaler per chip, changing the period of
...@@ -73,6 +74,7 @@ struct pca9685 { ...@@ -73,6 +74,7 @@ struct pca9685 {
#if IS_ENABLED(CONFIG_GPIOLIB) #if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock; struct mutex lock;
struct gpio_chip gpio; struct gpio_chip gpio;
DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
#endif #endif
}; };
...@@ -82,51 +84,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) ...@@ -82,51 +84,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
} }
#if IS_ENABLED(CONFIG_GPIOLIB) #if IS_ENABLED(CONFIG_GPIOLIB)
static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset) static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
{ {
struct pca9685 *pca = gpiochip_get_data(gpio); bool is_inuse;
struct pwm_device *pwm;
mutex_lock(&pca->lock); mutex_lock(&pca->lock);
if (pwm_idx >= PCA9685_MAXCHAN) {
pwm = &pca->chip.pwms[offset]; /*
* "all LEDs" channel:
if (pwm->flags & (PWMF_REQUESTED | PWMF_EXPORTED)) { * pretend already in use if any of the PWMs are requested
mutex_unlock(&pca->lock); */
return -EBUSY; if (!bitmap_empty(pca->pwms_inuse, PCA9685_MAXCHAN)) {
is_inuse = true;
goto out;
} }
} else {
pwm_set_chip_data(pwm, (void *)1); /*
* regular channel:
* pretend already in use if the "all LEDs" channel is requested
*/
if (test_bit(PCA9685_MAXCHAN, pca->pwms_inuse)) {
is_inuse = true;
goto out;
}
}
is_inuse = test_and_set_bit(pwm_idx, pca->pwms_inuse);
out:
mutex_unlock(&pca->lock); mutex_unlock(&pca->lock);
pm_runtime_get_sync(pca->chip.dev); return is_inuse;
return 0;
} }
static bool pca9685_pwm_is_gpio(struct pca9685 *pca, struct pwm_device *pwm) static void pca9685_pwm_clear_inuse(struct pca9685 *pca, int pwm_idx)
{ {
bool is_gpio = false;
mutex_lock(&pca->lock); mutex_lock(&pca->lock);
clear_bit(pwm_idx, pca->pwms_inuse);
mutex_unlock(&pca->lock);
}
if (pwm->hwpwm >= PCA9685_MAXCHAN) { static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
unsigned int i; {
struct pca9685 *pca = gpiochip_get_data(gpio);
/*
* Check if any of the GPIOs are requested and in that case
* prevent using the "all LEDs" channel.
*/
for (i = 0; i < pca->gpio.ngpio; i++)
if (gpiochip_is_requested(&pca->gpio, i)) {
is_gpio = true;
break;
}
} else if (pwm_get_chip_data(pwm)) {
is_gpio = true;
}
mutex_unlock(&pca->lock); if (pca9685_pwm_test_and_set_inuse(pca, offset))
return is_gpio; return -EBUSY;
pm_runtime_get_sync(pca->chip.dev);
return 0;
} }
static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset) static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
...@@ -161,6 +163,7 @@ static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset) ...@@ -161,6 +163,7 @@ static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
pca9685_pwm_gpio_set(gpio, offset, 0); pca9685_pwm_gpio_set(gpio, offset, 0);
pm_runtime_put(pca->chip.dev); pm_runtime_put(pca->chip.dev);
pca9685_pwm_clear_inuse(pca, offset);
} }
static int pca9685_pwm_gpio_get_direction(struct gpio_chip *chip, static int pca9685_pwm_gpio_get_direction(struct gpio_chip *chip,
...@@ -212,12 +215,17 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca) ...@@ -212,12 +215,17 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca)
return devm_gpiochip_add_data(dev, &pca->gpio, pca); return devm_gpiochip_add_data(dev, &pca->gpio, pca);
} }
#else #else
static inline bool pca9685_pwm_is_gpio(struct pca9685 *pca, static inline bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca,
struct pwm_device *pwm) int pwm_idx)
{ {
return false; return false;
} }
static inline void
pca9685_pwm_clear_inuse(struct pca9685 *pca, int pwm_idx)
{
}
static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca) static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca)
{ {
return 0; return 0;
...@@ -399,7 +407,7 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) ...@@ -399,7 +407,7 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{ {
struct pca9685 *pca = to_pca(chip); struct pca9685 *pca = to_pca(chip);
if (pca9685_pwm_is_gpio(pca, pwm)) if (pca9685_pwm_test_and_set_inuse(pca, pwm->hwpwm))
return -EBUSY; return -EBUSY;
pm_runtime_get_sync(chip->dev); pm_runtime_get_sync(chip->dev);
...@@ -408,8 +416,11 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) ...@@ -408,8 +416,11 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{ {
struct pca9685 *pca = to_pca(chip);
pca9685_pwm_disable(chip, pwm); pca9685_pwm_disable(chip, pwm);
pm_runtime_put(chip->dev); pm_runtime_put(chip->dev);
pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
} }
static const struct pwm_ops pca9685_pwm_ops = { static const struct pwm_ops pca9685_pwm_ops = {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment