Message-ID: <YyHVoihMuKNRFXAS@orome>
Date:   Wed, 14 Sep 2022 15:22:42 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Prathamesh Shete <pshete@...dia.com>
Cc:     linus.walleij@...aro.org, bgolaszewski@...libre.com,
        linux-gpio@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org, jonathanh@...dia.com,
        smangipudi@...dia.com, kyarlagadda@...dia.com,
        Manish Bhardwaj <mbhardwaj@...dia.com>
Subject: Re: [PATCH] gpio: tegra186: Check GPIO pin permission before access.

On Wed, Sep 14, 2022 at 05:51:10PM +0530, Prathamesh Shete wrote:
> This change checks if we have the necessary permission to
> access the GPIO. For devices that have support for virtualisation
> we need to check both the TEGRA186_GPIO_VM_REG and the
> TEGRA186_GPIO_SCR_REG registers. For device that do not have
> virtualisation support for GPIOs we only need to check the
> TEGRA186_GPIO_SCR_REG register.
> 
> Signed-off-by: Manish Bhardwaj <mbhardwaj@...dia.com>
> Signed-off-by: Prathamesh Shete <pshete@...dia.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 71 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)

I like where this is heading, however I think there's a little more room
for improvement, see below.

> 
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index 54d9fa7da9c1..e6fc3c9b1e9f 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -26,6 +26,22 @@
>  
>  #define TEGRA186_GPIO_INT_ROUTE_MAPPING(p, x) (0x14 + (p) * 0x20 + (x) * 4)
>  
> +#define  TEGRA186_GPIO_VM_REG			0x00

I'd leave out the _REG suffix. It's redundant.

> +#define  TEGRA186_GPIO_VM_RW_MASK		0x03
> +#define  TEGRA186_GPIO_SCR_REG			0x04

Same here.

> +#define  TEGRA186_GPIO_SCR_DIFF			0x08

Maybe name this something like: TEGRA186_GPIO_SCR_PIN_SIZE to be a
little more specific. The VM and SCR registers above are both per-pin,
so the size of the per-pin window is 8 bytes.

> +#define  TEGRA186_GPIO_SCR_BASE_DIFF		0x40

And then this would equivalently be TEGRA186_GPIO_SCR_PORT_SIZE. This is
the per-port window, where each port can have a maximum of 8 pins, so 8
* 8 = 0x40 bytes.

> +#define  TEGRA186_GPIO_SCR_SEC_WEN		BIT(28)
> +#define  TEGRA186_GPIO_SCR_SEC_REN		BIT(27)
> +#define  TEGRA186_GPIO_SCR_SEC_G1W		BIT(9)
> +#define  TEGRA186_GPIO_SCR_SEC_G1R		BIT(1)
> +#define  TEGRA186_GPIO_FULL_ACCESS		(TEGRA186_GPIO_SCR_SEC_WEN | \
> +						 TEGRA186_GPIO_SCR_SEC_REN | \
> +						 TEGRA186_GPIO_SCR_SEC_G1R | \
> +						 TEGRA186_GPIO_SCR_SEC_G1W)

Maybe TEGRA186_GPIO_SCR_SEC_FULL_ACCESS for consistency? It's a bit of a
mouthful, but the single line where this is used still fits within the
100 characters limit, so seems fine.

> +#define  TEGRA186_GPIO_SCR_SEC_ENABLE		(TEGRA186_GPIO_SCR_SEC_WEN | \
> +						 TEGRA186_GPIO_SCR_SEC_REN)

I'd also put the _SIZE definitions after all the register field
definitions so they don't get mistaken for a register offset or field
definition.

> +
>  /* control registers */
>  #define TEGRA186_GPIO_ENABLE_CONFIG 0x00
>  #define  TEGRA186_GPIO_ENABLE_CONFIG_ENABLE BIT(0)
> @@ -77,6 +93,7 @@ struct tegra_gpio_soc {
>  	unsigned int num_irqs_per_bank;
>  
>  	const struct tegra186_pin_range *pin_ranges;
> +	bool has_vm_support;

I had hoped that we could perhaps avoid this flag. So according to the
register documentation, the AON variants of the controller have a single
64 KiB page that contains both SCR and GPIO registers, whereas the VM-
capable variants (i.e. MAIN) contain SCR and GPIO registers in separate
64 KiB pages.

Now, unfortunately we've "abused" the "security" entry of the "reg"
property in DT to workaround the slight quirk that the GPIO registers
are offset by 4 KiB into the single 64 KiB page on AON. That's nifty on
one hand because it allows the driver to function in the same way as the
MAIN variant, but it's also not entirely accurate from a hardware
description point of view.

So while we currently have this in DT:

	gpio@...0000 {
		compatible = "nvidia,tegra234-gpio-aon";
		reg-names = "security", "gpio";
		reg = <0x0c2f0000 0x1000>;
		      <0x0c2f1000 0x1000>;
		...
	};

We should really have:

	gpio@...0000 {
		compatible = "nvidia,tegra234-gpio-aon";
		reg-names = "gpio";
		reg = <0x0c2f0000 0x10000>;
		...
	};

We could then, based on the absence of the "security" register region
derive in the driver that all "gpio" region accesses need to be offset
by that 4 KiB region.

That's a little difficult to do because of backwards-compatibility
requirements, so I'm tempted to just stick with what we have right now.
Alternatively we could also try to derive from the "security" region
size whether it's a full VM set of security registers, or whether its
the limited AON set.

The has_vm_support flag isn't all that bad, though, so I don't have a
strong objection here.

>  	unsigned int num_pin_ranges;
>  	const char *pinmux;
>  	bool has_gte;
> @@ -129,6 +146,45 @@ static void __iomem *tegra186_gpio_get_base(struct tegra_gpio *gpio,
>  	return gpio->base + offset + pin * 0x20;
>  }
>  
> +static void __iomem *tegra186_gpio_get_secure_base(struct tegra_gpio *gpio,
> +					    unsigned int pin)
> +{
> +	const struct tegra_gpio_port *port;
> +	unsigned int offset;
> +
> +	port = tegra186_gpio_get_port(gpio, &pin);
> +	if (!port)
> +		return NULL;
> +
> +	offset = port->bank * 0x1000 + port->port * TEGRA186_GPIO_SCR_BASE_DIFF;
> +
> +	return gpio->secure + offset + pin * TEGRA186_GPIO_SCR_DIFF;
> +}
> +
> +static inline bool tegra186_gpio_is_accessible(struct tegra_gpio *gpio, u32 pin)
> +{
> +	void __iomem *secure;
> +	u32 val;
> +
> +	secure = tegra186_gpio_get_secure_base(gpio, pin);
> +
> +	if (gpio->soc->has_vm_support) {
> +		val = readl(secure + TEGRA186_GPIO_VM_REG);
> +		if ((val & TEGRA186_GPIO_VM_RW_MASK) != TEGRA186_GPIO_VM_RW_MASK)
> +			return false;
> +	}
> +
> +	val = __raw_readl(secure + TEGRA186_GPIO_SCR_REG);
> +
> +	if ((val & TEGRA186_GPIO_SCR_SEC_ENABLE) == 0)
> +		return true;
> +
> +	if ((val & TEGRA186_GPIO_FULL_ACCESS) == TEGRA186_GPIO_FULL_ACCESS)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int tegra186_gpio_get_direction(struct gpio_chip *chip,
>  				       unsigned int offset)
>  {
> @@ -136,6 +192,9 @@ static int tegra186_gpio_get_direction(struct gpio_chip *chip,
>  	void __iomem *base;
>  	u32 value;
>  
> +	if (!tegra186_gpio_is_accessible(gpio, offset))
> +		return -EPERM;

It shouldn't be necessary to do this for every accessor function. In
general it should be enough to make sure the GPIO request fails for an
inaccessible GPIO. Interestingly there's already a feature built into
gpiolib that allows us to do exactly that. The gpiochip can implement
the ->init_valid_mask() callback, which can be used to mark certain pins
as invalid. See the gpio-aspeed-sgpio.c and gpio-bd71815.c for examples,
although the former seems to be completely redundant (the mask is all-
ones by default already) and the latter is quite simple.

For Tegra specifically I think what we want is to loop over all pins,
call tegra186_gpio_is_accessible() and if that returns false, call
bitmap_clear() for that specific pin.

With that the changes in the accessors here and below should not be
needed anymore and the gpiolib code should take care of everything.

> +
>  	base = tegra186_gpio_get_base(gpio, offset);
>  	if (WARN_ON(base == NULL))
>  		return -ENODEV;
> @@ -154,6 +213,9 @@ static int tegra186_gpio_direction_input(struct gpio_chip *chip,
>  	void __iomem *base;
>  	u32 value;
>  
> +	if (!tegra186_gpio_is_accessible(gpio, offset))
> +		return -EPERM;
> +
>  	base = tegra186_gpio_get_base(gpio, offset);
>  	if (WARN_ON(base == NULL))
>  		return -ENODEV;
> @@ -177,6 +239,9 @@ static int tegra186_gpio_direction_output(struct gpio_chip *chip,
>  	void __iomem *base;
>  	u32 value;
>  
> +	if (!tegra186_gpio_is_accessible(gpio, offset))
> +		return -EPERM;
> +
>  	/* configure output level first */
>  	chip->set(chip, offset, level);
>  
> @@ -293,6 +358,10 @@ static void tegra186_gpio_set(struct gpio_chip *chip, unsigned int offset,
>  	void __iomem *base;
>  	u32 value;
>  
> +	if (!tegra186_gpio_is_accessible(gpio, offset)){
> +		pr_err("GPIO not accessible\n");
> +		return;
> +	}
>  	base = tegra186_gpio_get_base(gpio, offset);
>  	if (WARN_ON(base == NULL))
>  		return;
> @@ -1042,6 +1111,7 @@ static const struct tegra_gpio_soc tegra194_main_soc = {
>  	.num_pin_ranges = ARRAY_SIZE(tegra194_main_pin_ranges),
>  	.pin_ranges = tegra194_main_pin_ranges,
>  	.pinmux = "nvidia,tegra194-pinmux",
> +	.has_vm_support = true,
>  };
>  
>  #define TEGRA194_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
> @@ -1067,6 +1137,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
>  	.instance = 1,
>  	.num_irqs_per_bank = 8,
>  	.has_gte = true,
> +	.has_vm_support = false,
>  };

Don't we need to set this for Tegra186 and Tegra234 as well?

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists