protected pins and debugfs

protected pins and debugfs

[Sodagudi Prasad]

Hi All,

This is regarding the protected pins configuration reading and printing 
from non-secure operating systems.
GPIO framework is checking whether pin is in use(flag FLAG_REQUESTED) or 
not in gpiolib_dbg_show().

If GPIO chip drivers are overriding the dbg_show callback, drivers are 
not checking whether a pin is really in use or not to print 
configuration details.
     if (chip->dbg_show)
                 chip->dbg_show(s, chip);
         else
                 gpiolib_dbg_show(s, gdev);

I think, reserved-gpio-ranges solution may move the problem to dts 
settings maintenance as they change from platform to platform.
https://lore.kernel.org/patchwork/patch/878107/
https://lore.kernel.org/patchwork/patch/878106/
https://lore.kernel.org/patchwork/patch/878109/

Can we use a simple/common solution like below?  It will check whether a 
pin is in use or not before printing configuration data with the help of 
gpiochip_is_requested().

index fef0970..2ca1440 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -579,16 +579,20 @@ static void msm_gpio_dbg_show_one(struct seq_file 
*s,
         seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", 
func);
         seq_printf(s, " %dmA", msm_regval_to_drive(drive));
         seq_printf(s, " %s", pulls[pull]);
+       seq_puts(s, "\n");
  }

  static void msm_gpio_dbg_show(struct seq_file *s, struct gpio_chip 
*chip)
  {
         unsigned gpio = chip->base;
         unsigned i;
+       const char *label;

         for (i = 0; i < chip->ngpio; i++, gpio++) {
+               label = gpiochip_is_requested(chip, i);
+               if (!label)
+                       continue;
                 msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
-               seq_puts(s, "\n");
         }
  }

-Thanks, Prasad

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: protected pins and debugfs

[Linus Walleij]

On Wed, Oct 3, 2018 at 2:38 PM Sodagudi Prasad <psodagud@codeaurora.org> wrote:

> This is regarding the protected pins configuration reading and printing
> from non-secure operating systems.

I do not think anyone with security in mind should have debugfs
enabled. But maybe that is beside the point.

> GPIO framework is checking whether pin is in use(flag FLAG_REQUESTED) or
> not in gpiolib_dbg_show().
>
> If GPIO chip drivers are overriding the dbg_show callback, drivers are
> not checking whether a pin is really in use or not to print
> configuration details.
>      if (chip->dbg_show)
>                  chip->dbg_show(s, chip);
>          else
>                  gpiolib_dbg_show(s, gdev);

Ah that is right. Because some drivers can inspect all pins whether they
are requested or not.

> Can we use a simple/common solution like below?  It will check whether a
> pin is in use or not before printing configuration data with the help of
> gpiochip_is_requested().

In the msm case I think maybe you want to inspect the valid_mask
instead, so you display debugfs info for all pins you can inspect
in hardware but avoid the "invalid" ones which I half-guess is used
by ACPI in your case.

Yours,
Linus Walleij

Re: protected pins and debugfs

[Stephen Boyd]

Quoting Sodagudi Prasad (2018-10-03 05:38:24)
> 
>          for (i = 0; i < chip->ngpio; i++, gpio++) {
> +               label = gpiochip_is_requested(chip, i);
> +               if (!label)
> +                       continue;
>                  msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> -               seq_puts(s, "\n");
>          }
>   }
> 

Does something not work with the following code in
msm_gpio_dbg_show_one()?


        if (!gpiochip_line_is_valid(chip, offset))
		return;

Re: protected pins and debugfs

[Sodagudi Prasad]

On 2018-10-07 23:04, Stephen Boyd wrote:
> Quoting Sodagudi Prasad (2018-10-03 05:38:24)
>> 
>>          for (i = 0; i < chip->ngpio; i++, gpio++) {
>> +               label = gpiochip_is_requested(chip, i);
>> +               if (!label)
>> +                       continue;
>>                  msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
>> -               seq_puts(s, "\n");
>>          }
>>   }
>> 
> 
> Does something not work with the following code in
> msm_gpio_dbg_show_one()?
> 
> 
>         if (!gpiochip_line_is_valid(chip, offset))
> 		return;

Hi Stephen,
I didnt realize that these changes are merged on tip. I was testing on 
4.14 kernel.

https://lore.kernel.org/patchwork/patch/878107/
https://lore.kernel.org/patchwork/patch/878106/
https://lore.kernel.org/patchwork/patch/878109/

I will add "gpio-reserved-ranges" to internal platforms and this issue 
should not be observed.

-thanks, Prasad

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: protected pins and debugfs

[Sodagudi Prasad]

On 2018-10-10 12:40, Sodagudi Prasad wrote:
> On 2018-10-07 23:04, Stephen Boyd wrote:
>> Quoting Sodagudi Prasad (2018-10-03 05:38:24)
>>> 
>>>          for (i = 0; i < chip->ngpio; i++, gpio++) {
>>> +               label = gpiochip_is_requested(chip, i);
>>> +               if (!label)
>>> +                       continue;
>>>                  msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
>>> -               seq_puts(s, "\n");
>>>          }
>>>   }
>>> 
>> 
>> Does something not work with the following code in
>> msm_gpio_dbg_show_one()?
>> 
>> 
>>         if (!gpiochip_line_is_valid(chip, offset))
>> 		return;
> 
> Hi Stephen,
> I didnt realize that these changes are merged on tip. I was testing on
> 4.14 kernel.
> 
> https://lore.kernel.org/patchwork/patch/878107/
> https://lore.kernel.org/patchwork/patch/878106/
> https://lore.kernel.org/patchwork/patch/878109/


Hi Stephen,

After checking this further, adding "gpio-reserved-ranges" in not good 
option. Because of the following reasons.
1) These gpio information changes from platform to platform. So need to 
maintain reserved-range properly for each platform.
2) Also some of the gpio can be changed to secure/protected gpio 
dynamically based on the use case.

It looks adding the "gpio-reserved-ranges" ranges is not good option for 
most of the platforms.

Can you please check the initial patch suggested in this thread? Please 
let me know if you have any other options for the above points.

-Thanks, Prasad

> 
> I will add "gpio-reserved-ranges" to internal platforms and this issue
> should not be observed.
> 
> -thanks, Prasad

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: protected pins and debugfs

[Stephen Boyd]

Quoting Sodagudi Prasad (2018-10-16 19:00:45)
> On 2018-10-10 12:40, Sodagudi Prasad wrote:
> > On 2018-10-07 23:04, Stephen Boyd wrote:
> >> Quoting Sodagudi Prasad (2018-10-03 05:38:24)
> >>> 
> >>>          for (i = 0; i < chip->ngpio; i++, gpio++) {
> >>> +               label = gpiochip_is_requested(chip, i);
> >>> +               if (!label)
> >>> +                       continue;
> >>>                  msm_gpio_dbg_show_one(s, NULL, chip, i, gpio);
> >>> -               seq_puts(s, "\n");
> >>>          }
> >>>   }
> >>> 
> >> 
> >> Does something not work with the following code in
> >> msm_gpio_dbg_show_one()?
> >> 
> >> 
> >>         if (!gpiochip_line_is_valid(chip, offset))
> >>              return;
> > 
> > Hi Stephen,
> > I didnt realize that these changes are merged on tip. I was testing on
> > 4.14 kernel.
> > 
> > https://lore.kernel.org/patchwork/patch/878107/
> > https://lore.kernel.org/patchwork/patch/878106/
> > https://lore.kernel.org/patchwork/patch/878109/
> 
> 
> Hi Stephen,
> 
> After checking this further, adding "gpio-reserved-ranges" in not good 
> option. Because of the following reasons.
> 1) These gpio information changes from platform to platform. So need to 
> maintain reserved-range properly for each platform.

This was known. The best approach was to populate this based on what the
firmware chooses to lock down, which didn't seem to change very often
from what I recall being told.

> 2) Also some of the gpio can be changed to secure/protected gpio 
> dynamically based on the use case.

This is new information. How exactly do these pins change dynamically?
Perhaps gpiolib needs to gain more support for the mask to change at
runtime instead of being populated once at boot time from DT.  Seems
like a patch could be written for gpiolib to handle that if it really is
something that needs to be done.

> 
> It looks adding the "gpio-reserved-ranges" ranges is not good option for 
> most of the platforms.
> 
> Can you please check the initial patch suggested in this thread? Please 
> let me know if you have any other options for the above points.
> 

The initial patch in this thread takes a Qualcomm centric approach to
the problem, which probably works just fine, but we would like to push
the solution to problem into the core framework so that other SoCs can
benefit from common code. Please look into handling the runtime swapping
in the gpiolib core so that we don't have a one-off solution for
Qualcomm here.