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

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(-)

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