linux-kernel - Re: [PATCH -next] pwm: pca9685: Remove set but not used variable 'pwm'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGngYiXZM0QUdKE_zDK763J9iDuiKSbmFeTVA1PJ_4WvjntjQQ@mail.gmail.com>
Date:   Sat, 1 Jun 2019 09:03:09 -0400
From:   Sven Van Asbroeck <thesven73@...il.com>
To:     YueHaibing <yuehaibing@...wei.com>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        u.kleine-koenig@...gutronix.de,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-pwm@...r.kernel.org, kernel-janitors@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -next] pwm: pca9685: Remove set but not used variable 'pwm'

Hi YueHaibing,

On Fri, May 31, 2019 at 11:49 PM YueHaibing <yuehaibing@...wei.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