linux-kernel - Re: [PATCH] pinctrl: intel: save HOSTSW

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190402115836.GL9224@smile.fi.intel.com>
Date:   Tue, 2 Apr 2019 14:58:36 +0300
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Chris Chiu <chiu@...lessm.com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Daniel Drake <drake@...lessm.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:PIN CONTROL SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Linux Upstreaming Team <linux@...lessm.com>
Subject: Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over
 suspend/resume

On Tue, Apr 02, 2019 at 02:16:19PM +0800, Chris Chiu wrote:
> On Mon, Apr 1, 2019 at 8:23 PM Andy Shevchenko
> <andriy.shevchenko@...el.com> wrote:
> > On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:

> Thanks for the comment. My first version did mimic the logic of the interrupt
> mask restore but it was based on the DMI quirk. It saves HOSTSW_OWN
> for each padgroup and restores them all after resume if DMI info matched.
> 
> What really confused me is how to do this specifically for a requested GPIO
> pin. So here's my new proposed patch. Please suggests if there's any better
> idea. Thanks.

>  struct intel_community_context {
>         u32 *intmask;
> +       u32 *hostown;

This is okay.

>  };

> +#ifdef CONFIG_PM_SLEEP
> +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin);
> +#endif
> +

No need for this...

>         /* Disable TX buffer and enable RX (this will be input) */
>         __intel_gpio_set_direction(padcfg0, true);
> +#ifdef CONFIG_PM_SLEEP
> +       intel_save_hostown(pctrl, pin);
> +#endif

...and for this.
Just save all of them at ->suspend()

>         for (i = 0; i < pctrl->ncommunities; i++) {
>                 struct intel_community *community = &pctrl->communities[i];
> -               u32 *intmask;
> +               u32 *intmask, *hostown;
> 
>                 intmask = devm_kcalloc(pctrl->dev, community->ngpps,
>                                        sizeof(*intmask), GFP_KERNEL);
> @@ -1292,6 +1299,13 @@ static int intel_pinctrl_pm_init(struct
> intel_pinctrl *pctrl)
>                         return -ENOMEM;
> 
>                 communities[i].intmask = intmask;
> +
> +               hostown = devm_kcalloc(pctrl->dev, community->ngpps,
> +                                      sizeof(*hostown), GFP_KERNEL);
> +               if (!hostown)
> +                       return -ENOMEM;
> +
> +               communities[i].hostown= hostown;

This is good.

>         }

> +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> +       const struct intel_community *community;
> +       const struct intel_padgroup *padgrp;
> +       int i;
> +
> +       community = intel_get_community(pctrl, pin);
> +       if (!community)
> +               return;
> +       if (!community->hostown_offset)
> +               return;
> +
> +       padgrp = intel_community_get_padgroup(community, pin);
> +       if (!padgrp)
> +               return;
> +
> +       for (i = 0; i < pctrl->ncommunities; i++) {
> +               const struct intel_community *comm = &pctrl->communities[i];
> +               int j;
> +
> +               for (j = 0; j < comm->ngpps; j++) {
> +                       const struct intel_padgroup *pgrp = &comm->gpps[j];
> +
> +                       if (padgrp == pgrp) {
> +                               struct intel_community_context *communities;
> +                               void __iomem *base;
> +
> +                               communities = pctrl->context.communities;
> +                               base = community->regs +
> community->hostown_offset;
> +                               communities[i].hostown[j] = readl(base + j * 4);
> +                               break;
> +                       }
> +               }
> +       }

> +       return;

Useless.

> +}

This is too complicated. Just add

base = community->regs + community->hostown_offset;
for (gpp = 0; gpp < community->ngpps; gpp++)
	communities[i].hostown[gpp] = readl(base + gpp * 4);

into ->suspend() loop.

> +               base = community->regs + community->hostown_offset;
> +               for (gpp = 0; gpp < community->ngpps; gpp++) {
> +                       if (communities[i].hostown[gpp] &&
> +                           communities[i].hostown[gpp] != readl(base
> + gpp * 4)) {
> +                               writel(communities[i].hostown[gpp],
> base + gpp * 4);
> +                               dev_warn(dev, "hostown changed after resume\n");
> +                               dev_dbg(dev, "restored hostown %d/%u
> %#08x\n", i, gpp,
> +                                       readl(base + gpp * 4));
> +                       }
> +               }

Instead you may need to loop over each pin in the part of the group related to
one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the driver),
check if it's requested and break a loop. If loop index is off-by-one a limit,
nothing to do, otherwise restore hostown register.

More pedantic approach is to collect the mask inside the loop and apply it.

The check function name is gpiochip_is_requested().

(One of Intel's drivers which is using that at ->resume() is
 drivers/gpio/gpio-lynxpoint.c)

P.S. I prefer pedantic approach. The simplification one is showed in order to
give you an idea.

-- 
With Best Regards,
Andy Shevchenko