Message ID | 20190601035709.85379-1-yuehaibing@huawei.com |
---|---|
State | Rejected |
Headers | show |
Series | [-next] pwm: pca9685: Remove set but not used variable 'pwm' | expand |
Hi YueHaibing, On Fri, May 31, 2019 at 11:49 PM YueHaibing <yuehaibing@huawei.com> wrote: > > mutex_lock(&pca->lock); > - pwm = &pca->chip.pwms[offset]; > mutex_unlock(&pca->lock); Thanks for noticing this issue. However it should be fixed differently. This was introduced by Uwe's clean-up patch: commit e926b12c611c2095c79 ("pwm: Clear chip_data in pwm_put()") But Uwe did not realize that in this case, the pwm chip_data is used as a synchronization mechanism between pwm and gpio. Moving the chip_data clear out of the mutex breaks this mechanism. I think the following would restore the mechanism: > mutex_lock(&pca->lock); > pwm = &pca->chip.pwms[offset]; > + pwm_set_chip_data(pwm, NULL); > mutex_unlock(&pca->lock); This would of course clear the pwm chip_data twice, once in the driver and once in the core, but that's not a problem. I'd like to hear Mika Westerberg's opinion, because he introduced this synchronization mechanism back in 2016. [Adding Mika] Sven
Hello Sven, On Sat, Jun 01, 2019 at 09:03:09AM -0400, Sven Van Asbroeck wrote: > Hi YueHaibing, > > On Fri, May 31, 2019 at 11:49 PM YueHaibing <yuehaibing@huawei.com> wrote: > > > > mutex_lock(&pca->lock); > > - pwm = &pca->chip.pwms[offset]; > > mutex_unlock(&pca->lock); > > Thanks for noticing this issue. However it should be fixed differently. > > This was introduced by Uwe's clean-up patch: > commit e926b12c611c2095c79 ("pwm: Clear chip_data in pwm_put()") > > But Uwe did not realize that in this case, the pwm chip_data is used as a > synchronization mechanism between pwm and gpio. Moving the chip_data > clear out of the mutex breaks this mechanism. > > I think the following would restore the mechanism: > > > mutex_lock(&pca->lock); > > pwm = &pca->chip.pwms[offset]; > > + pwm_set_chip_data(pwm, NULL); > > mutex_unlock(&pca->lock); I didn't look into the driver to try to understand that, but the definitely needs a comment to explain for the next person to think they can do a cleanup here. Best regards Uwe
On Sat, Jun 1, 2019 at 12:05 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > I didn't look into the driver to try to understand that, but the > definitely needs a comment to explain for the next person to think they > can do a cleanup here. Certainly. But if we do restore the old behaviour, there may still be problems. I'm unsure if the old synchronization was working correctly. See the example at the end of this email. An intuitive way forward would be to use a simple bitfield in struct pca9685 to track if a specific pwm is in use by either pwm or gpio. Protected by a mutex. But it could be that the old behaviour is 'good enough' for the driver's users (I am no longer one of them). =========================================== Let's take the example of two threads, racing to grab a pwm and gpio, respectively. Assume the gpio is backed by the pwm, so they conflict. [Thread 1] drivers/pwm/core.c: if (pwm->chip->ops->request) { err = pwm->chip->ops->request(pwm->chip, pwm); [this calls pca9685_pwm_request()] [which calls pca9685_pwm_is_gpio()] [checks pwm chip_data, is NULL, pwm is free] [returns 0 - pwm request ok] if (err) { module_put(pwm->chip->ops->owner); return err; } } [CONTEXT SWITCH to Thread 2] 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]; [pwm->flags do not (yet) have PWMF_REQUESTED] if (pwm->flags & (PWMF_REQUESTED | PWMF_EXPORTED)) { 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; } [CONTEXT SWITCH back to Thread 1, still in pwm/core.c] set_bit(PWMF_REQUESTED, &pwm->flags); pwm->label = label;
On Sun, Jun 02, 2019 at 10:18:15AM -0400, Sven Van Asbroeck wrote: > On Sat, Jun 1, 2019 at 12:05 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > I didn't look into the driver to try to understand that, but the > > definitely needs a comment to explain for the next person to think they > > can do a cleanup here. > > Certainly. I agree. > But if we do restore the old behaviour, there may still be problems. > I'm unsure if the old synchronization was working correctly. > See the example at the end of this email. I think you are right. pca9685_pwm_request() should take the mutex as long as it is requesting PWM. > An intuitive way forward would be to use a simple bitfield in > struct pca9685 to track if a specific pwm is in use by either > pwm or gpio. Protected by a mutex. A flag would probably be easier to understand than the magic we have now. Or then wrap it inside function with an explanation comment: static inline void pca9685_pwm_set_as_gpio(struct pwm_device *pwm) { /* * We use ->chip_data to convoy the fact that the PWM channel is * being used as GPIO instead of PWM. */ pwm_set_chip_data(pwm, (void *)1) } static inline void pca9685_pwm_set_as_pwm(struct pwm_device *pwm) { pwm_set_chip_data(pwm, NULL); }
On Mon, Jun 3, 2019 at 7:40 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > I think you are right. pca9685_pwm_request() should take the mutex as > long as it is requesting PWM. Yes, but things get hairy because pca9685_pwm_request() will have to give up the mutex when it returns. I cannot see a way to keep holding this mutex while the in-use flag is set by the pwm core ? Alternatively, we could set (void *)1 pwm_data inside the pwm_request, wrapped inside the mutex. But then things get 'messy'. > A flag would probably be easier to understand than the magic we have > now. I have the feeling that a flag (plus a mutex) would be the clearest and safest way forward. I'll post a patch soon, you guys tell me what you think. Unfortunately, I no longer have any test hardware. The project that required this chip is long dead.
On Mon, Jun 03, 2019 at 11:08:06AM -0400, Sven Van Asbroeck wrote: > On Mon, Jun 3, 2019 at 7:40 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > > I think you are right. pca9685_pwm_request() should take the mutex as > > long as it is requesting PWM. > > Yes, but things get hairy because pca9685_pwm_request() will have to > give up the mutex when it returns. I cannot see a way to keep holding > this mutex while the in-use flag is set by the pwm core ? Right, I did not notice it's the PWM core that sets the flag. > Alternatively, we could set (void *)1 pwm_data inside the pwm_request, > wrapped inside the mutex. > But then things get 'messy'. > > > A flag would probably be easier to understand than the magic we have > > now. > > I have the feeling that a flag (plus a mutex) would be the clearest and > safest way forward. I'll post a patch soon, you guys tell me what you > think. Sounds good thanks!
On Mon, Jun 03, 2019 at 11:08:06AM -0400, Sven Van Asbroeck wrote: > On Mon, Jun 3, 2019 at 7:40 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > Unfortunately, I no longer have any test hardware. The project that > required this chip is long dead. Anyone in possession of Intel Galileo Gen 2 can test this.
I was able to test the patch [1] exclusion mechanism without access to actual hardware - by giving it a dummy regmap. See patch below. Test cases (all via sysfs): 1. verify requested pwm cannot be requested as gpio 2. verify requested gpio cannot be requested as pwm 3. verify pwm "all LEDs" cannot be used if pwms/gpios in use 4. verify pwms/gpios cannot be requested if pwm "all LEDs" in use All test cases ok. Obviously, I could not test multi-threaded correctness. [1] https://lkml.org/lkml/2019/6/4/1039 --- drivers/pwm/pwm-pca9685.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 259fd58812ae..c059da5f86f4 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -83,6 +83,7 @@ struct pca9685 { struct regmap *regmap; int duty_ns; int period_ns; + u8 regs[PCA9685_NUMREGS]; #if IS_ENABLED(CONFIG_GPIOLIB) struct mutex lock; struct gpio_chip gpio; @@ -446,11 +447,31 @@ static const struct pwm_ops pca9685_pwm_ops = { .owner = THIS_MODULE, }; +static int read_reg_dummy(void *context, unsigned int reg, + unsigned int *val) +{ + struct pca9685 *pca = context; + + *val = pca->regs[reg]; + return 0; +} + +static int write_reg_dummy(void *context, unsigned int reg, + unsigned int val) +{ + struct pca9685 *pca = context; + + pca->regs[reg] = val; + return 0; +} + static const struct regmap_config pca9685_regmap_i2c_config = { .reg_bits = 8, .val_bits = 8, .max_register = PCA9685_NUMREGS, .cache_type = REGCACHE_NONE, + .reg_read = read_reg_dummy, + .reg_write = write_reg_dummy, }; static int pca9685_pwm_probe(struct i2c_client *client, @@ -464,7 +485,8 @@ static int pca9685_pwm_probe(struct i2c_client *client, if (!pca) return -ENOMEM; - pca->regmap = devm_regmap_init_i2c(client, &pca9685_regmap_i2c_config); + pca->regmap = devm_regmap_init(&client->dev, NULL, pca, + &pca9685_regmap_i2c_config); if (IS_ERR(pca->regmap)) { ret = PTR_ERR(pca->regmap); dev_err(&client->dev, "Failed to initialize register map: %d\n",
On Thu, Jun 06, 2019 at 11:11:11AM -0400, Sven Van Asbroeck wrote: > I was able to test the patch [1] exclusion mechanism without access to actual > hardware - by giving it a dummy regmap. See patch below. > > Test cases (all via sysfs): > 1. verify requested pwm cannot be requested as gpio > 2. verify requested gpio cannot be requested as pwm > 3. verify pwm "all LEDs" cannot be used if pwms/gpios in use > 4. verify pwms/gpios cannot be requested if pwm "all LEDs" in use > > All test cases ok. > Obviously, I could not test multi-threaded correctness. > > [1] https://lkml.org/lkml/2019/6/4/1039 Is this patch still relevant? A patch similar to YueHaibing's one was merged in the meantime but I guess the underlying problem is still relevant. Sven, do you care enough to recheck and create a patch on top of a more recent tree? Best regards Uwe
Hi Uwe, On Sat, May 23, 2020 at 4:17 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Is this patch still relevant? A slightly different version of this patch has already landed in mainline. Clemens Gruber had relevant hardware available, and was able to test it out. See Linus's tree here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/pwm-pca9685.c?h=v5.7-rc6&id=9cc5f232a4b6a0ef6e9b57876d61b88f61bdd7c2 Have a great week-end, Sven
On Sat, May 23, 2020 at 08:24:23PM -0400, Sven Van Asbroeck wrote: > Hi Uwe, > > On Sat, May 23, 2020 at 4:17 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > Is this patch still relevant? > > A slightly different version of this patch has already landed in > mainline. Clemens Gruber had > relevant hardware available, and was able to test it out. > > See Linus's tree here: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/pwm/pwm-pca9685.c?h=v5.7-rc6&id=9cc5f232a4b6a0ef6e9b57876d61b88f61bdd7c2 Optimal, thanks for the feedback. So this thread can be closed. Thanks Uwe
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 567f5e2771c4..d16215c276bd 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -170,12 +170,10 @@ 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; pca9685_pwm_gpio_set(gpio, offset, 0); pm_runtime_put(pca->chip.dev); mutex_lock(&pca->lock); - pwm = &pca->chip.pwms[offset]; mutex_unlock(&pca->lock); }
Fixes gcc '-Wunused-but-set-variable' warning: drivers/pwm/pwm-pca9685.c: In function 'pca9685_pwm_gpio_free': drivers/pwm/pwm-pca9685.c:173:21: warning: variable 'pwm' set but not used [-Wunused-but-set-variable] It's not used since commit e926b12c611c ("pwm: Clear chip_data in pwm_put()") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/pwm/pwm-pca9685.c | 2 -- 1 file changed, 2 deletions(-)