[-next] pwm: pca9685: Remove set but not used variable 'pwm'
diff mbox series

Message ID 20190601035709.85379-1-yuehaibing@huawei.com
State New
Headers show
Series
  • [-next] pwm: pca9685: Remove set but not used variable 'pwm'
Related show

Commit Message

YueHaibing June 1, 2019, 3:57 a.m. UTC
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(-)

Comments

Sven Van Asbroeck June 1, 2019, 1:03 p.m. UTC | #1
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
Uwe Kleine-König June 1, 2019, 4:04 p.m. UTC | #2
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
Sven Van Asbroeck June 2, 2019, 2:18 p.m. UTC | #3
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;
mika.westerberg@linux.intel.com June 3, 2019, 11:40 a.m. UTC | #4
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);
}
Sven Van Asbroeck June 3, 2019, 3:08 p.m. UTC | #5
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.
mika.westerberg@linux.intel.com June 3, 2019, 3:58 p.m. UTC | #6
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!
Andy Shevchenko June 4, 2019, 4:01 p.m. UTC | #7
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.
Sven Van Asbroeck June 6, 2019, 3:11 p.m. UTC | #8
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",

Patch
diff mbox series

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);
 }