| Message ID | 9aec322678417753fe4022691f4bfdbe@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | protected pins and debugfs | expand |
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
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;
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
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
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.
--- 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");
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 } } -Thanks, Prasad