linux-kernel - Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active

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: <CAMRc=McGTizah7fPjWEer4mioQnOPZeFm-eBsrLxP0=7bM1-UQ@mail.gmail.com>
Date:   Wed, 27 Sep 2023 11:18:53 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Daniel Scally <djrscally@...il.com>,
        Mark Gross <markgross@...nel.org>, linux-gpio@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [RFT PATCH 0/4] platform/x86: int3472: don't use gpiod_toggle_active_low()

On Wed, Sep 27, 2023 at 11:02 AM Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi Bartosz,
>
> On 9/27/23 10:48, Bartosz Golaszewski wrote:
> > On Wed, Sep 27, 2023 at 10:38 AM Hans de Goede <hdegoede@...hat.com> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On 9/26/23 16:59, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> >>>
> >>> gpiod_toggle_active_low() is a badly designed API that should have never
> >>> been used elsewhere then in the MMC code. And even there we should find
> >>> a better solution.
> >>>
> >>> Replace the uses of it in the int3472 driver with the good old temporary
> >>> lookup table trick. This is not very pretty either but it's the lesser
> >>> evil.
> >>
> >> I saw your previous proposal which added a new api to directly set
> >> the active_low flag, rather then toggle it.
> >>
> >> I intended to reply to that thread to say that I liked that approach,
> >> but I don't remember if I actually did reply.
> >>
> >> I wonder what made you abandon the new function to directly set
> >> the active-low flag on a gpio_desc?
> >>
> >> For the int3472 code that would work pretty well and it would
> >> be much cleaner then the temp gpio-lookup approach.
> >>
> >
> > You did reply, yes. Under one of the other patches Linus W stated that
> > first: adding the ability for consumers to toggle the polarity was
> > added to handle the MMC slot quirk, then it was used unknowingly to
> > GPIO maintainers in other places (including this driver). I then
> > acknowledged the fact that it should have never existed in the first
> > place as this is HW description and should be defined in ACPI, DT or
> > lookup flags.
>
> I see and I understand.
>
> > I'm not sure why this information needs to be hard-coded in the driver
> > in int3472_get_func_and_polarity() but maybe it could be pulled into
> > gpiolib-acpi.c with other quirks?
>
> The problem is that for camera sensors Intel uses this special
> INT3472 ACPI device with a custom _DSM to list GPIOs, with the _DSM
> returning an u32 and one of the bits in the u32 is the polarity.
>
> We really do not want to deal with this Intel camera team hack
> inside gpiolib-acpi and I can understand why you and Linus W
> want to get rid of functions which allow drivers to meddle
> with a gpio_desc's active-low flag.
>
> So using a temporary gpio-lookup in the int3472 code as
> you are proposing is the best (least bad) thing to do
> here then.
>
> I'll try to make some time to test this sometime
> the coming days.
>
> Other then the discussion we just had is there any specific
> reason why this should be considered a RFC / why this would
> not be ready for merging?  (I still need to review these,
> but lets assume that goes well)
>

This is not an RFC but rather RFT - Request For Testing. I don't have
any HW to test those with so I only built it.

Bart