[PATCH] gpio: tegra186: Check GPIO pin permission before access.

[PATCH] gpio: tegra186: Check GPIO pin permission before access.

[Prathamesh Shete]

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@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 71 ++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

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
+#define  TEGRA186_GPIO_VM_RW_MASK		0x03
+#define  TEGRA186_GPIO_SCR_REG			0x04
+#define  TEGRA186_GPIO_SCR_DIFF			0x08
+#define  TEGRA186_GPIO_SCR_BASE_DIFF		0x40
+#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)
+#define  TEGRA186_GPIO_SCR_SEC_ENABLE		(TEGRA186_GPIO_SCR_SEC_WEN | \
+						 TEGRA186_GPIO_SCR_SEC_REN)
+
 /* 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;
 	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;
+
 	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,
 };
 
 #define TEGRA234_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
-- 
2.17.1

Re: [PATCH] gpio: tegra186: Check GPIO pin permission before access.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8674 bytes --]

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@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.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@c2f0000 {
		compatible = "nvidia,tegra234-gpio-aon";
		reg-names = "security", "gpio";
		reg = <0x0c2f0000 0x1000>;
		      <0x0c2f1000 0x1000>;
		...
	};

We should really have:

	gpio@c2f0000 {
		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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] gpio: tegra186: Check GPIO pin permission before access.

[Prathamesh Shete]

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@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 74 ++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 54d9fa7da9c1..34b6c287d608 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			0x00
+#define  TEGRA186_GPIO_VM_RW_MASK		0x03
+#define  TEGRA186_GPIO_SCR			0x04
+#define  TEGRA186_GPIO_SCR_PIN_SIZE		0x08
+#define  TEGRA186_GPIO_SCR_PORT_SIZE		0x40
+#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)
+#define  TEGRA186_GPIO_SCR_SEC_ENABLE		(TEGRA186_GPIO_SCR_SEC_WEN | \
+						 TEGRA186_GPIO_SCR_SEC_REN)
+
 /* 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;
 	unsigned int num_pin_ranges;
 	const char *pinmux;
 	bool has_gte;
@@ -129,6 +146,58 @@ 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_PORT_SIZE;
+
+	return gpio->secure + offset + pin * TEGRA186_GPIO_SCR_PIN_SIZE;
+}
+
+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);
+		if ((val & TEGRA186_GPIO_VM_RW_MASK) != TEGRA186_GPIO_VM_RW_MASK)
+			return false;
+	}
+
+	val = __raw_readl(secure + TEGRA186_GPIO_SCR);
+
+	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_init_valid_mask(struct gpio_chip *chip,
+		unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	int j;
+
+	for (j = 0; j < ngpios; j++) {
+		if (!tegra186_gpio_is_accessible(gpio, j))
+			clear_bit(j, valid_mask);
+	}
+	return 0;
+}
+
 static int tegra186_gpio_get_direction(struct gpio_chip *chip,
 				       unsigned int offset)
 {
@@ -763,6 +832,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->soc = device_get_match_data(&pdev->dev);
 	gpio->gpio.label = gpio->soc->name;
 	gpio->gpio.parent = &pdev->dev;
+	gpio->gpio.init_valid_mask = tegra186_init_valid_mask;
 
 	/* count the number of banks in the controller */
 	for (i = 0; i < gpio->soc->num_ports; i++)
@@ -1042,6 +1112,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 +1138,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
 	.instance = 1,
 	.num_irqs_per_bank = 8,
 	.has_gte = true,
+	.has_vm_support = false,
 };
 
 #define TEGRA234_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1111,6 +1183,7 @@ static const struct tegra_gpio_soc tegra234_main_soc = {
 	.name = "tegra234-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = true,
 };
 
 #define TEGRA234_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1136,6 +1209,7 @@ static const struct tegra_gpio_soc tegra234_aon_soc = {
 	.name = "tegra234-gpio-aon",
 	.instance = 1,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = false,
 };
 
 #define TEGRA241_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
-- 
2.17.1

Re: [PATCH v2] gpio: tegra186: Check GPIO pin permission before access.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6126 bytes --]

On Tue, Oct 04, 2022 at 01:18:45PM +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@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 74 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)

This looks mostly good. A few small comments, see below.

> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index 54d9fa7da9c1..34b6c287d608 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			0x00
> +#define  TEGRA186_GPIO_VM_RW_MASK		0x03
> +#define  TEGRA186_GPIO_SCR			0x04
> +#define  TEGRA186_GPIO_SCR_PIN_SIZE		0x08
> +#define  TEGRA186_GPIO_SCR_PORT_SIZE		0x40
> +#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)
> +#define  TEGRA186_GPIO_SCR_SEC_ENABLE		(TEGRA186_GPIO_SCR_SEC_WEN | \
> +						 TEGRA186_GPIO_SCR_SEC_REN)
> +
>  /* 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;

You've placed this right between two fields that clearly belong
together. Best to add new fields towards the end of the structure if
they are not related to any of the other fields.

>  	unsigned int num_pin_ranges;
>  	const char *pinmux;
>  	bool has_gte;
> @@ -129,6 +146,58 @@ 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_PORT_SIZE;
> +
> +	return gpio->secure + offset + pin * TEGRA186_GPIO_SCR_PIN_SIZE;
> +}
> +
> +static inline bool tegra186_gpio_is_accessible(struct tegra_gpio *gpio, u32 pin)

Pin offsets are "unsigned int" in the rest of the driver, so please
stick to that here as well.

> +{
> +	void __iomem *secure;
> +	u32 val;

The drivers uses "u32 value;" everywhere else, so best stick to that for
consistency.

> +
> +	secure = tegra186_gpio_get_secure_base(gpio, pin);
> +
> +	if (gpio->soc->has_vm_support) {
> +		val = readl(secure + TEGRA186_GPIO_VM);
> +		if ((val & TEGRA186_GPIO_VM_RW_MASK) != TEGRA186_GPIO_VM_RW_MASK)
> +			return false;
> +	}
> +
> +	val = __raw_readl(secure + TEGRA186_GPIO_SCR);
> +
> +	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_init_valid_mask(struct gpio_chip *chip,
> +		unsigned long *valid_mask, unsigned int ngpios)
> +{
> +	struct tegra_gpio *gpio = gpiochip_get_data(chip);
> +	int j;

Make that unsigned int to match the type of ngpios.

> +
> +	for (j = 0; j < ngpios; j++) {
> +		if (!tegra186_gpio_is_accessible(gpio, j))
> +			clear_bit(j, valid_mask);
> +	}
> +	return 0;
> +}
> +
>  static int tegra186_gpio_get_direction(struct gpio_chip *chip,
>  				       unsigned int offset)
>  {
> @@ -763,6 +832,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>  	gpio->soc = device_get_match_data(&pdev->dev);
>  	gpio->gpio.label = gpio->soc->name;
>  	gpio->gpio.parent = &pdev->dev;
> +	gpio->gpio.init_valid_mask = tegra186_init_valid_mask;

Maybe slot this in before the gpio->gpio.add_pin_ranges assignment
further below in this function to keep things ordered a bit better.

>  
>  	/* count the number of banks in the controller */
>  	for (i = 0; i < gpio->soc->num_ports; i++)
> @@ -1042,6 +1112,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 +1138,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
>  	.instance = 1,
>  	.num_irqs_per_bank = 8,
>  	.has_gte = true,
> +	.has_vm_support = false,
>  };
>  
>  #define TEGRA234_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
> @@ -1111,6 +1183,7 @@ static const struct tegra_gpio_soc tegra234_main_soc = {
>  	.name = "tegra234-gpio",
>  	.instance = 0,
>  	.num_irqs_per_bank = 8,
> +	.has_vm_support = true,
>  };
>  
>  #define TEGRA234_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
> @@ -1136,6 +1209,7 @@ static const struct tegra_gpio_soc tegra234_aon_soc = {
>  	.name = "tegra234-gpio-aon",
>  	.instance = 1,
>  	.num_irqs_per_bank = 8,
> +	.has_vm_support = false,

It's not technically necessary to initialize 0/false fields because
that's what they end up with if you don't initialize them at all. I do
like to be explicit, so no objection from me on this. However, since we
are being explicit, we should set this new field for Tegra186 and
Tegra241 as well.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[Prathamesh Shete]

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@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 78 ++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 54d9fa7da9c1..873b476ae8f1 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			0x00
+#define  TEGRA186_GPIO_VM_RW_MASK		0x03
+#define  TEGRA186_GPIO_SCR			0x04
+#define  TEGRA186_GPIO_SCR_PIN_SIZE		0x08
+#define  TEGRA186_GPIO_SCR_PORT_SIZE		0x40
+#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)
+#define  TEGRA186_GPIO_SCR_SEC_ENABLE		(TEGRA186_GPIO_SCR_SEC_WEN | \
+						 TEGRA186_GPIO_SCR_SEC_REN)
+
 /* control registers */
 #define TEGRA186_GPIO_ENABLE_CONFIG 0x00
 #define  TEGRA186_GPIO_ENABLE_CONFIG_ENABLE BIT(0)
@@ -80,6 +96,7 @@ struct tegra_gpio_soc {
 	unsigned int num_pin_ranges;
 	const char *pinmux;
 	bool has_gte;
+	bool has_vm_support;
 };
 
 struct tegra_gpio {
@@ -129,6 +146,58 @@ 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_PORT_SIZE;
+
+	return gpio->secure + offset + pin * TEGRA186_GPIO_SCR_PIN_SIZE;
+}
+
+static inline bool tegra186_gpio_is_accessible(struct tegra_gpio *gpio, unsigned int pin)
+{
+	void __iomem *secure;
+	u32 value;
+
+	secure = tegra186_gpio_get_secure_base(gpio, pin);
+
+	if (gpio->soc->has_vm_support) {
+		value = readl(secure + TEGRA186_GPIO_VM);
+		if ((value & TEGRA186_GPIO_VM_RW_MASK) != TEGRA186_GPIO_VM_RW_MASK)
+			return false;
+	}
+
+	value = __raw_readl(secure + TEGRA186_GPIO_SCR);
+
+	if ((value & TEGRA186_GPIO_SCR_SEC_ENABLE) == 0)
+		return true;
+
+	if ((value & TEGRA186_GPIO_FULL_ACCESS) == TEGRA186_GPIO_FULL_ACCESS)
+		return true;
+
+	return false;
+}
+
+static int tegra186_init_valid_mask(struct gpio_chip *chip,
+		unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	unsigned int j;
+
+	for (j = 0; j < ngpios; j++) {
+		if (!tegra186_gpio_is_accessible(gpio, j))
+			clear_bit(j, valid_mask);
+	}
+	return 0;
+}
+
 static int tegra186_gpio_get_direction(struct gpio_chip *chip,
 				       unsigned int offset)
 {
@@ -818,6 +887,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->gpio.set = tegra186_gpio_set;
 	gpio->gpio.set_config = tegra186_gpio_set_config;
 	gpio->gpio.add_pin_ranges = tegra186_gpio_add_pin_ranges;
+	gpio->gpio.init_valid_mask = tegra186_init_valid_mask;
 	if (gpio->soc->has_gte) {
 		gpio->gpio.en_hw_timestamp = tegra186_gpio_en_hw_ts;
 		gpio->gpio.dis_hw_timestamp = tegra186_gpio_dis_hw_ts;
@@ -960,6 +1030,7 @@ static const struct tegra_gpio_soc tegra186_main_soc = {
 	.name = "tegra186-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 1,
+	.has_vm_support = false,
 };
 
 #define TEGRA186_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -987,6 +1058,7 @@ static const struct tegra_gpio_soc tegra186_aon_soc = {
 	.name = "tegra186-gpio-aon",
 	.instance = 1,
 	.num_irqs_per_bank = 1,
+	.has_vm_support = false,
 };
 
 #define TEGRA194_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1042,6 +1114,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 +1140,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
 	.instance = 1,
 	.num_irqs_per_bank = 8,
 	.has_gte = true,
+	.has_vm_support = false,
 };
 
 #define TEGRA234_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1111,6 +1185,7 @@ static const struct tegra_gpio_soc tegra234_main_soc = {
 	.name = "tegra234-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = true,
 };
 
 #define TEGRA234_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1136,6 +1211,7 @@ static const struct tegra_gpio_soc tegra234_aon_soc = {
 	.name = "tegra234-gpio-aon",
 	.instance = 1,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = false,
 };
 
 #define TEGRA241_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1166,6 +1242,7 @@ static const struct tegra_gpio_soc tegra241_main_soc = {
 	.name = "tegra241-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = false,
 };
 
 #define TEGRA241_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1187,6 +1264,7 @@ static const struct tegra_gpio_soc tegra241_aon_soc = {
 	.name = "tegra241-gpio-aon",
 	.instance = 1,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = false,
 };
 
 static const struct of_device_id tegra186_gpio_of_match[] = {
-- 
2.17.1

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On Fri, Oct 07, 2022 at 11:29:36AM +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@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  drivers/gpio/gpio-tegra186.c | 78 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)

I like it, thanks!

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[Linus Walleij]

On Fri, Oct 7, 2022 at 7:59 AM Prathamesh Shete <pshete@nvidia.com> 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@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>

Very nice patch!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[Jon Hunter]

On 17/10/2022 10:31, Linus Walleij wrote:
> On Fri, Oct 7, 2022 at 7:59 AM Prathamesh Shete <pshete@nvidia.com> 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@nvidia.com>
>> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> 
> Very nice patch!
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>


I did not see this anywhere in the mainline/next. However, I also 
noticed that we don't have the correct email address for Bartosz (again).

Bartosz, let me know if you can pick this up? Thierry also ack'ed 
previously for Tegra too.

FWIW ...

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Thanks
Jon

-- 
nvpublic

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[Bartosz Golaszewski]

On Tue, May 23, 2023 at 8:22 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>
>
> On 17/10/2022 10:31, Linus Walleij wrote:
> > On Fri, Oct 7, 2022 at 7:59 AM Prathamesh Shete <pshete@nvidia.com> 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@nvidia.com>
> >> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> >
> > Very nice patch!
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
>
> I did not see this anywhere in the mainline/next. However, I also
> noticed that we don't have the correct email address for Bartosz (again).
>

I have only ever changed my address in MAINTAINERS once, so "again" is
not really the right term. And scripts/get_maintainer.pl should be
used anyway every time when submitting patches.

> Bartosz, let me know if you can pick this up? Thierry also ack'ed
> previously for Tegra too.
>
> FWIW ...
>
> Acked-by: Jon Hunter <jonathanh@nvidia.com>
>

This doesn't apply to v6.4-rc1. Prathmesh: could you rebase and resend?

Bart

> Thanks
> Jon
>
> --
> nvpublic

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[Jon Hunter]

On 23/05/2023 10:17, Bartosz Golaszewski wrote:
> On Tue, May 23, 2023 at 8:22 AM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> On 17/10/2022 10:31, Linus Walleij wrote:
>>> On Fri, Oct 7, 2022 at 7:59 AM Prathamesh Shete <pshete@nvidia.com> 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@nvidia.com>
>>>> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
>>>
>>> Very nice patch!
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>>
>> I did not see this anywhere in the mainline/next. However, I also
>> noticed that we don't have the correct email address for Bartosz (again).
>>
> 
> I have only ever changed my address in MAINTAINERS once, so "again" is
> not really the right term. And scripts/get_maintainer.pl should be
> used anyway every time when submitting patches.

Sorry I meant that WE did not send to the correct email AGAIN and not 
that you updated your email address :-)

Thanks
Jon

-- 
nvpublic

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[andy.shevchenko]

Tue, May 23, 2023 at 02:42:52PM +0100, Jon Hunter kirjoitti:
> On 23/05/2023 10:17, Bartosz Golaszewski wrote:
> > On Tue, May 23, 2023 at 8:22 AM Jon Hunter <jonathanh@nvidia.com> wrote:
> > > On 17/10/2022 10:31, Linus Walleij wrote:

...

> > > I did not see this anywhere in the mainline/next. However, I also
> > > noticed that we don't have the correct email address for Bartosz (again).
> > 
> > I have only ever changed my address in MAINTAINERS once, so "again" is
> > not really the right term. And scripts/get_maintainer.pl should be
> > used anyway every time when submitting patches.
> 
> Sorry I meant that WE did not send to the correct email AGAIN and not that
> you updated your email address :-)

FWIW, you may look into my "smart" script [1] which automatically fills the Cc
and To WRT MAINTAINERS records.

[1]: https://lore.kernel.org/linux-gpio/Yz62XmiH8YG3Dtsf@orome/T/#t

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: tegra186: Check GPIO pin permission before access.

[andy.shevchenko]

Tue, May 23, 2023 at 07:32:48PM +0300, andy.shevchenko@gmail.com kirjoitti:
> Tue, May 23, 2023 at 02:42:52PM +0100, Jon Hunter kirjoitti:
> > On 23/05/2023 10:17, Bartosz Golaszewski wrote:
> > > On Tue, May 23, 2023 at 8:22 AM Jon Hunter <jonathanh@nvidia.com> wrote:
> > > > On 17/10/2022 10:31, Linus Walleij wrote:

...

> > > > I did not see this anywhere in the mainline/next. However, I also
> > > > noticed that we don't have the correct email address for Bartosz (again).
> > > 
> > > I have only ever changed my address in MAINTAINERS once, so "again" is
> > > not really the right term. And scripts/get_maintainer.pl should be
> > > used anyway every time when submitting patches.
> > 
> > Sorry I meant that WE did not send to the correct email AGAIN and not that
> > you updated your email address :-)
> 
> FWIW, you may look into my "smart" script [1] which automatically fills the Cc
> and To WRT MAINTAINERS records.
> 
> [1]: https://lore.kernel.org/linux-gpio/Yz62XmiH8YG3Dtsf@orome/T/#t

Oops, wrong link, here we are:

https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4] gpio: tegra186: Check GPIO pin permission before access.

[Prathamesh Shete]

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@nvidia.com>
Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/gpio/gpio-tegra186.c | 78 ++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index b904de0b1784..464b0ea3b6f1 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -27,6 +27,22 @@
 
 #define TEGRA186_GPIO_INT_ROUTE_MAPPING(p, x) (0x14 + (p) * 0x20 + (x) * 4)
 
+#define  TEGRA186_GPIO_VM			0x00
+#define  TEGRA186_GPIO_VM_RW_MASK		0x03
+#define  TEGRA186_GPIO_SCR			0x04
+#define  TEGRA186_GPIO_SCR_PIN_SIZE		0x08
+#define  TEGRA186_GPIO_SCR_PORT_SIZE		0x40
+#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)
+#define  TEGRA186_GPIO_SCR_SEC_ENABLE		(TEGRA186_GPIO_SCR_SEC_WEN | \
+						 TEGRA186_GPIO_SCR_SEC_REN)
+
 /* control registers */
 #define TEGRA186_GPIO_ENABLE_CONFIG 0x00
 #define  TEGRA186_GPIO_ENABLE_CONFIG_ENABLE BIT(0)
@@ -81,6 +97,7 @@ struct tegra_gpio_soc {
 	unsigned int num_pin_ranges;
 	const char *pinmux;
 	bool has_gte;
+	bool has_vm_support;
 };
 
 struct tegra_gpio {
@@ -130,6 +147,58 @@ 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_PORT_SIZE;
+
+	return gpio->secure + offset + pin * TEGRA186_GPIO_SCR_PIN_SIZE;
+}
+
+static inline bool tegra186_gpio_is_accessible(struct tegra_gpio *gpio, unsigned int pin)
+{
+	void __iomem *secure;
+	u32 value;
+
+	secure = tegra186_gpio_get_secure_base(gpio, pin);
+
+	if (gpio->soc->has_vm_support) {
+		value = readl(secure + TEGRA186_GPIO_VM);
+		if ((value & TEGRA186_GPIO_VM_RW_MASK) != TEGRA186_GPIO_VM_RW_MASK)
+			return false;
+	}
+
+	value = __raw_readl(secure + TEGRA186_GPIO_SCR);
+
+	if ((value & TEGRA186_GPIO_SCR_SEC_ENABLE) == 0)
+		return true;
+
+	if ((value & TEGRA186_GPIO_FULL_ACCESS) == TEGRA186_GPIO_FULL_ACCESS)
+		return true;
+
+	return false;
+}
+
+static int tegra186_init_valid_mask(struct gpio_chip *chip,
+				    unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct tegra_gpio *gpio = gpiochip_get_data(chip);
+	unsigned int j;
+
+	for (j = 0; j < ngpios; j++) {
+		if (!tegra186_gpio_is_accessible(gpio, j))
+			clear_bit(j, valid_mask);
+	}
+	return 0;
+}
+
 static int tegra186_gpio_get_direction(struct gpio_chip *chip,
 				       unsigned int offset)
 {
@@ -816,6 +885,7 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
 	gpio->gpio.set = tegra186_gpio_set;
 	gpio->gpio.set_config = tegra186_gpio_set_config;
 	gpio->gpio.add_pin_ranges = tegra186_gpio_add_pin_ranges;
+	gpio->gpio.init_valid_mask = tegra186_init_valid_mask;
 	if (gpio->soc->has_gte) {
 		gpio->gpio.en_hw_timestamp = tegra186_gpio_en_hw_ts;
 		gpio->gpio.dis_hw_timestamp = tegra186_gpio_dis_hw_ts;
@@ -958,6 +1028,7 @@ static const struct tegra_gpio_soc tegra186_main_soc = {
 	.name = "tegra186-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 1,
+	.has_vm_support = false,
 };
 
 #define TEGRA186_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -985,6 +1056,7 @@ static const struct tegra_gpio_soc tegra186_aon_soc = {
 	.name = "tegra186-gpio-aon",
 	.instance = 1,
 	.num_irqs_per_bank = 1,
+	.has_vm_support = false,
 };
 
 #define TEGRA194_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1040,6 +1112,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)	\
@@ -1065,6 +1138,7 @@ static const struct tegra_gpio_soc tegra194_aon_soc = {
 	.instance = 1,
 	.num_irqs_per_bank = 8,
 	.has_gte = true,
+	.has_vm_support = false,
 };
 
 #define TEGRA234_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1109,6 +1183,7 @@ static const struct tegra_gpio_soc tegra234_main_soc = {
 	.name = "tegra234-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = true,
 };
 
 #define TEGRA234_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1135,6 +1210,7 @@ static const struct tegra_gpio_soc tegra234_aon_soc = {
 	.instance = 1,
 	.num_irqs_per_bank = 8,
 	.has_gte = true,
+	.has_vm_support = false,
 };
 
 #define TEGRA241_MAIN_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1165,6 +1241,7 @@ static const struct tegra_gpio_soc tegra241_main_soc = {
 	.name = "tegra241-gpio",
 	.instance = 0,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = false,
 };
 
 #define TEGRA241_AON_GPIO_PORT(_name, _bank, _port, _pins)	\
@@ -1186,6 +1263,7 @@ static const struct tegra_gpio_soc tegra241_aon_soc = {
 	.name = "tegra241-gpio-aon",
 	.instance = 1,
 	.num_irqs_per_bank = 8,
+	.has_vm_support = false,
 };
 
 static const struct of_device_id tegra186_gpio_of_match[] = {
-- 
2.17.1

Re: [PATCH v4] gpio: tegra186: Check GPIO pin permission before access.

[Bartosz Golaszewski]

On Wed, May 24, 2023 at 12:50 PM Prathamesh Shete <pshete@nvidia.com> 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@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Acked-by: Jon Hunter <jonathanh@nvidia.com>
> ---

Applied, thanks!

Bart

Re: [PATCH] gpio: tegra186: Check GPIO pin permission before access.

[Linus Walleij]

On Wed, Sep 14, 2022 at 2:21 PM Prathamesh Shete <pshete@nvidia.com> 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@nvidia.com>
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>

Instead of doing this check in each and every single gpio callback,
set up the .init_valid_mask on struct gpio_chip, add a for-loop
looping over all GPIOs and fill in the .valid_mask.

This way the gpiolib core will do the check for you.

git grep init_valid_mask for a bunch of examples on how to use this.

Oh I see Thierry already said the same while I was typing :)

Yours,
Linus Walleij