Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Oct 2022 13:05:02 +0200
From:   Thierry Reding <thierry.reding@gmail.com>
To:     Prathamesh Shete <pshete@nvidia.com>
Cc:     linus.walleij@linaro.org, bgolaszewski@baylibre.com,
        jonathanh@nvidia.com, linux-gpio@vger.kernel.org,
        linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
        smangipudi@nvidia.com, kyarlagadda@nvidia.com,
        Manish Bhardwaj <mbhardwaj@nvidia.com>
Subject: Re: [PATCH v2] gpio: tegra186: Check GPIO pin permission before
 access.
Message-ID: <Yz62XmiH8YG3Dtsf@orome>
References: <YyHVoihMuKNRFXAS@orome>
 <20221004074845.29583-1-pshete@nvidia.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="wIueMpqElCFwwyGl"
Content-Disposition: inline
In-Reply-To: <20221004074845.29583-1-pshete@nvidia.com>
User-Agent: Mutt/2.2.7 (2022-08-07)
Precedence: bulk


--wIueMpqElCFwwyGl
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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 @@
> =20
>  #define TEGRA186_GPIO_INT_ROUTE_MAPPING(p, x) (0x14 + (p) * 0x20 + (x) *=
 4)
> =20
> +#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;
> =20
>  	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 t=
egra_gpio *gpio,
>  	return gpio->base + offset + pin * 0x20;
>  }
> =20
> +static void __iomem *tegra186_gpio_get_secure_base(struct tegra_gpio *gp=
io,
> +					    unsigned int pin)
> +{
> +	const struct tegra_gpio_port *port;
> +	unsigned int offset;
> +
> +	port =3D tegra186_gpio_get_port(gpio, &pin);
> +	if (!port)
> +		return NULL;
> +
> +	offset =3D port->bank * 0x1000 + port->port * TEGRA186_GPIO_SCR_PORT_SI=
ZE;
> +
> +	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 =3D tegra186_gpio_get_secure_base(gpio, pin);
> +
> +	if (gpio->soc->has_vm_support) {
> +		val =3D readl(secure + TEGRA186_GPIO_VM);
> +		if ((val & TEGRA186_GPIO_VM_RW_MASK) !=3D TEGRA186_GPIO_VM_RW_MASK)
> +			return false;
> +	}
> +
> +	val =3D __raw_readl(secure + TEGRA186_GPIO_SCR);
> +
> +	if ((val & TEGRA186_GPIO_SCR_SEC_ENABLE) =3D=3D 0)
> +		return true;
> +
> +	if ((val & TEGRA186_GPIO_FULL_ACCESS) =3D=3D 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 =3D gpiochip_get_data(chip);
> +	int j;

Make that unsigned int to match the type of ngpios.

> +
> +	for (j =3D 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 =3D device_get_match_data(&pdev->dev);
>  	gpio->gpio.label =3D gpio->soc->name;
>  	gpio->gpio.parent =3D &pdev->dev;
> +	gpio->gpio.init_valid_mask =3D 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.

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

--wIueMpqElCFwwyGl
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmM+tlwACgkQ3SOs138+
s6Ez+w/9FpeFqssyODdvFPPaSJVoQhx8Avcx4WK/cItqBnM+tB1fygrgpl6A2sh5
XvZKOzhET0TCtFLpftfonlG+3elynFYdOEqugG9vcFTcS7vffxXt9dj5tJX62k7X
7dbonYK9jvNhy26oEjOe2NOaRk6BOR4KNVC8TYwXZcJizq4Z6m/8apk8hiYLIzd8
vonhilY/oeA7ac4QtL2JOtN1zmIzUKc0Gmz55ZwOMAhJAZag3zJpsE1dBLDQc96k
ncDV+v99WFsghH9hdwWGLXf9rvvPkoQFWzayeE6kKMlFVLRnTZE5sYomAAfJfFF1
TfDcW6P3aj32VCCBq6Jtrlq0Z3LLgbBZyPTJKqTMj1JK+7a7ruDa9RObMCrifPcC
C4hb1R/F3t7NF/FJOjA3DnWilMwNZV8Ez8ux6FTiVWVMPPyBKnLns7YbFYpOyWd/
LjD2yKuWhEfAUA0te8GlRBvO4fgm8VxaJqbfK3aOwRZT+n1ZiSCrR1PQMih9GSbE
PzunC2qRpyr28AoUpII1IqCAYzs/Xz3FBh7XipIQSH1vEQPSzJwwdQXGyhmZTKvl
aMPL6CAngZsnntATEEMtatiZPnGcJHl63J74cxYXkMrAY9AbEZCaxrjKInOhnIcL
hEWwUhEXlVozq1FlSZSicZ2YpLNb8tQcF5Sr4N1S/Quu3tVuuR0=
=wN8V
-----END PGP SIGNATURE-----

--wIueMpqElCFwwyGl--