| Message ID | 20190603151223.5311-1-TheSven73@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: pca9685: fix pwm/gpio inter-operation | expand |
On Mon, Jun 03, 2019 at 11:12:23AM -0400, Sven Van Asbroeck wrote: > 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 pwm 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 patch. > > 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 pwm 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: YueHaibing <yuehaibing@huawei.com> > Fixes: e926b12c611c ("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> > Link: https://lkml.org/lkml/2019/5/31/963 > Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> > --- > > This approach will also prevent the request of the "all" pwm channel, if any > other pwm channel is already in use. Is this correct behaviour? Sounds correct to me. > drivers/pwm/pwm-pca9685.c | 64 +++++++++++++++++++++------------------ > 1 file changed, 35 insertions(+), 29 deletions(-) > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > index 567f5e2771c4..f9927cd106d0 100644 > --- a/drivers/pwm/pwm-pca9685.c > +++ b/drivers/pwm/pwm-pca9685.c > @@ -31,6 +31,7 @@ > #include <linux/slab.h> > #include <linux/delay.h> > #include <linux/pm_runtime.h> > +#include <linux/bitmap.h> > > /* > * Because the PCA9685 has only one prescaler per chip, changing the period of > @@ -85,6 +86,7 @@ struct pca9685 { > #if IS_ENABLED(CONFIG_GPIOLIB) > struct mutex lock; > struct gpio_chip gpio; > + DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN); > #endif > }; > > @@ -97,48 +99,45 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset) > { > struct pca9685 *pca = gpiochip_get_data(gpio); > - struct pwm_device *pwm; > > mutex_lock(&pca->lock); > > - pwm = &pca->chip.pwms[offset]; > - > - if (pwm->flags & (PWMF_REQUESTED | PWMF_EXPORTED)) { > + if (test_and_set_bit(offset, pca->pwms_inuse)) { > mutex_unlock(&pca->lock); > return -EBUSY; > } > > - pwm_set_chip_data(pwm, (void *)1); > - > mutex_unlock(&pca->lock); > pm_runtime_get_sync(pca->chip.dev); > return 0; > } > > -static bool pca9685_pwm_is_gpio(struct pca9685 *pca, struct pwm_device *pwm) > +static bool > +pca9685_pwm_test_set_inuse(struct pca9685 *pca, struct pwm_device *pwm) Can we call it pca9685_pwm_test_and_set_inuse() following naming of test_and_set_bit()? > { > - bool is_gpio = false; > + bool is_inuse; > > mutex_lock(&pca->lock); > + /* > + * Check if any of the PWMs are requested and in that case > + * prevent using the "all LEDs" channel. > + */ > + if (pwm->hwpwm >= PCA9685_MAXCHAN && > + !bitmap_empty(pca->pwms_inuse, PCA9685_MAXCHAN)) > + is_inuse = true; > + else > + is_inuse = test_and_set_bit(pwm->hwpwm, pca->pwms_inuse); > + mutex_unlock(&pca->lock); > > - if (pwm->hwpwm >= PCA9685_MAXCHAN) { > - unsigned int i; > - > - /* > - * 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; > - } > + return is_inuse; > +} > > +static void pca9685_pwm_clear_inuse(struct pca9685 *pca, struct pwm_device *pwm) I think it might be better if you provide __pca9685_pwm_clear_inuse() that does not take the lock and then pca9685_pwm_clear_inuse() that just calls the former. Then -> > +{ > + mutex_lock(&pca->lock); > + if (pwm->hwpwm < PCA9685_MAXCHAN) > + clear_bit(pwm->hwpwm, pca->pwms_inuse); > mutex_unlock(&pca->lock); > - return is_gpio; > } > > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset) > @@ -170,12 +169,11 @@ static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset, > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset) > { > struct pca9685 *pca = gpiochip_get_data(gpio); > - struct pwm_device *pwm; > > + mutex_lock(&pca->lock); > pca9685_pwm_gpio_set(gpio, offset, 0); > pm_runtime_put(pca->chip.dev); > - mutex_lock(&pca->lock); > - pwm = &pca->chip.pwms[offset]; > + clear_bit(offset, pca->pwms_inuse); -> you can call __pca9685_pwm_clear_inuse() It feels more "consistent" wrt setting the bit. > mutex_unlock(&pca->lock); > } > > @@ -228,12 +226,17 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca) > return devm_gpiochip_add_data(dev, &pca->gpio, pca); > } > #else > -static inline bool pca9685_pwm_is_gpio(struct pca9685 *pca, > +static inline bool pca9685_pwm_test_set_inuse(struct pca9685 *pca, > struct pwm_device *pwm) > { > return false; > } > > +static inline void > +pca9685_pwm_clear_inuse(struct pca9685 *pca, struct pwm_device *pwm) > +{ > +} > + > static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca) > { > return 0; > @@ -417,7 +420,7 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct pca9685 *pca = to_pca(chip); > > - if (pca9685_pwm_is_gpio(pca, pwm)) > + if (pca9685_pwm_test_set_inuse(pca, pwm)) > return -EBUSY; > pm_runtime_get_sync(chip->dev); > > @@ -426,8 +429,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) > { > + struct pca9685 *pca = to_pca(chip); > + > pca9685_pwm_disable(chip, pwm); > pm_runtime_put(chip->dev); > + pca9685_pwm_clear_inuse(pca, pwm); > } > > static const struct pwm_ops pca9685_pwm_ops = { > -- > 2.17.1
Thank you for the review, Mika ! See comments below. On Tue, Jun 4, 2019 at 5:14 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > > This approach will also prevent the request of the "all" pwm channel, if any > > other pwm channel is already in use. Is this correct behaviour? > > Sounds correct to me. Something that occurred to me right after I pressed the send button: This patch will prevent a pwm 'all channels' request if at least one of the pwm's is in use. But it will not guard against the opposite: after the 'all channels' pwm is requested, it will still allow requests for other pwms/gpios ! This is identical to the old behaviour. But maybe this is an oversight and not a feature? Proposal: 1. prevent request of 'all channel' if any of the pwms/gpios are in use 2. prevent request of all other pwms/gpios if 'all channels' is in use > > Can we call it pca9685_pwm_test_and_set_inuse() following naming of > test_and_set_bit()? Sounds good to me. > > > > +static void pca9685_pwm_clear_inuse(struct pca9685 *pca, struct pwm_device *pwm) > > I think it might be better if you provide __pca9685_pwm_clear_inuse() > that does not take the lock and then pca9685_pwm_clear_inuse() that just > calls the former. Then -> I agree, this is much cleaner.
On Tue, Jun 04, 2019 at 09:34:46AM -0400, Sven Van Asbroeck wrote: > Thank you for the review, Mika ! See comments below. > > On Tue, Jun 4, 2019 at 5:14 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > > This approach will also prevent the request of the "all" pwm channel, if any > > > other pwm channel is already in use. Is this correct behaviour? > > > > Sounds correct to me. > > Something that occurred to me right after I pressed the send button: > > This patch will prevent a pwm 'all channels' request if at least one > of the pwm's is in use. But it will not guard against the opposite: > after the 'all channels' pwm is requested, it will still allow requests > for other pwms/gpios ! > > This is identical to the old behaviour. But maybe this is an oversight > and not a feature? Most probably an oversight. > Proposal: > 1. prevent request of 'all channel' if any of the pwms/gpios are in use > 2. prevent request of all other pwms/gpios if 'all channels' is in use Makes sense.
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 567f5e2771c4..f9927cd106d0 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -31,6 +31,7 @@ #include <linux/slab.h> #include <linux/delay.h> #include <linux/pm_runtime.h> +#include <linux/bitmap.h> /* * Because the PCA9685 has only one prescaler per chip, changing the period of @@ -85,6 +86,7 @@ struct pca9685 { #if IS_ENABLED(CONFIG_GPIOLIB) struct mutex lock; struct gpio_chip gpio; + DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN); #endif }; @@ -97,48 +99,45 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset) { struct pca9685 *pca = gpiochip_get_data(gpio); - struct pwm_device *pwm; mutex_lock(&pca->lock); - pwm = &pca->chip.pwms[offset]; - - if (pwm->flags & (PWMF_REQUESTED | PWMF_EXPORTED)) { + if (test_and_set_bit(offset, pca->pwms_inuse)) { mutex_unlock(&pca->lock); return -EBUSY; } - pwm_set_chip_data(pwm, (void *)1); - mutex_unlock(&pca->lock); pm_runtime_get_sync(pca->chip.dev); return 0; } -static bool pca9685_pwm_is_gpio(struct pca9685 *pca, struct pwm_device *pwm) +static bool +pca9685_pwm_test_set_inuse(struct pca9685 *pca, struct pwm_device *pwm) { - bool is_gpio = false; + bool is_inuse; mutex_lock(&pca->lock); + /* + * Check if any of the PWMs are requested and in that case + * prevent using the "all LEDs" channel. + */ + if (pwm->hwpwm >= PCA9685_MAXCHAN && + !bitmap_empty(pca->pwms_inuse, PCA9685_MAXCHAN)) + is_inuse = true; + else + is_inuse = test_and_set_bit(pwm->hwpwm, pca->pwms_inuse); + mutex_unlock(&pca->lock); - if (pwm->hwpwm >= PCA9685_MAXCHAN) { - unsigned int i; - - /* - * 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; - } + return is_inuse; +} +static void pca9685_pwm_clear_inuse(struct pca9685 *pca, struct pwm_device *pwm) +{ + mutex_lock(&pca->lock); + if (pwm->hwpwm < PCA9685_MAXCHAN) + clear_bit(pwm->hwpwm, pca->pwms_inuse); mutex_unlock(&pca->lock); - return is_gpio; } static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset) @@ -170,12 +169,11 @@ static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset, static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset) { struct pca9685 *pca = gpiochip_get_data(gpio); - struct pwm_device *pwm; + mutex_lock(&pca->lock); pca9685_pwm_gpio_set(gpio, offset, 0); pm_runtime_put(pca->chip.dev); - mutex_lock(&pca->lock); - pwm = &pca->chip.pwms[offset]; + clear_bit(offset, pca->pwms_inuse); mutex_unlock(&pca->lock); } @@ -228,12 +226,17 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca) return devm_gpiochip_add_data(dev, &pca->gpio, pca); } #else -static inline bool pca9685_pwm_is_gpio(struct pca9685 *pca, +static inline bool pca9685_pwm_test_set_inuse(struct pca9685 *pca, struct pwm_device *pwm) { return false; } +static inline void +pca9685_pwm_clear_inuse(struct pca9685 *pca, struct pwm_device *pwm) +{ +} + static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca) { return 0; @@ -417,7 +420,7 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) { struct pca9685 *pca = to_pca(chip); - if (pca9685_pwm_is_gpio(pca, pwm)) + if (pca9685_pwm_test_set_inuse(pca, pwm)) return -EBUSY; pm_runtime_get_sync(chip->dev); @@ -426,8 +429,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) { + struct pca9685 *pca = to_pca(chip); + pca9685_pwm_disable(chip, pwm); pm_runtime_put(chip->dev); + pca9685_pwm_clear_inuse(pca, pwm); } static const struct pwm_ops pca9685_pwm_ops = {
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 pwm 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 patch. 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 pwm 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: YueHaibing <yuehaibing@huawei.com> Fixes: e926b12c611c ("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> Link: https://lkml.org/lkml/2019/5/31/963 Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com> --- This approach will also prevent the request of the "all" pwm channel, if any other pwm channel is already in use. Is this correct behaviour? drivers/pwm/pwm-pca9685.c | 64 +++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 29 deletions(-)