netdev - Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for GPIO configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <659c4254-d0b7-52dc-dd9b-3921cd2f20c0@gmail.com>
Date:   Thu, 28 Sep 2017 10:45:03 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Brandon Streiff <brandon.streiff@...com>, netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...oirfairelinux.com>,
        Richard Cochran <richardcochran@...il.com>,
        Erik Hons <erik.hons@...com>
Subject: Re: [PATCH net-next RFC 3/9] net: dsa: mv88e6xxx: add support for
 GPIO configuration

On 09/28/2017 08:25 AM, Brandon Streiff wrote:
> The Scratch/Misc register is a windowed interface that provides access
> to the GPIO configuration. Provide a new method for configuration of
> GPIO functions.
> 
> Signed-off-by: Brandon Streiff <brandon.streiff@...com>
> ---

> +/* Offset 0x1A: Scratch and Misc. Register */
> +static int mv88e6xxx_g2_scratch_reg_read(struct mv88e6xxx_chip *chip,
> +					 int reg, u8 *data)
> +{
> +	int err;
> +	u16 value;
> +
> +	err = mv88e6xxx_g2_write(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC,
> +				 reg << 8);
> +	if (err)
> +		return err;
> +
> +	err = mv88e6xxx_g2_read(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, &value);
> +	if (err)
> +		return err;
> +
> +	*data = (value & MV88E6XXX_G2_SCRATCH_MISC_DATA_MASK);
> +
> +	return 0;
> +}

With the write and read acquiring and then releasing the lock
immediately, is no there room for this sequence to be interrupted in the
middle and end-up returning inconsistent reads?

> +
> +static int mv88e6xxx_g2_scratch_reg_write(struct mv88e6xxx_chip *chip,
> +					  int reg, u8 data)
> +{
> +	u16 value = (reg << 8) | data;
> +
> +	return mv88e6xxx_g2_update(chip, MV88E6XXX_G2_SCRATCH_MISC_MISC, value);
> +}
> +
> +/* Configures the specified pin for the specified function. This function
> + * does not unset other pins configured for the same function. If multiple
> + * pins are configured for the same function, the lower-index pin gets
> + * that function and the higher-index pin goes back to being GPIO.
> + */
> +int mv88e6xxx_g2_set_gpio_config(struct mv88e6xxx_chip *chip, int pin,
> +				 int func, int dir)
> +{
> +	int mode_reg = MV88E6XXX_G2_SCRATCH_GPIO_MODE(pin);
> +	int dir_reg = MV88E6XXX_G2_SCRATCH_GPIO_DIR(pin);
> +	int err;
> +	u8 val;
> +
> +	if (pin < 0 || pin >= mv88e6xxx_num_gpio(chip))
> +		return -ERANGE;
> +
> +	/* Set function first */
> +	err = mv88e6xxx_g2_scratch_reg_read(chip, mode_reg, &val);
> +	if (err)
> +		return err;
> +
> +	/* Zero bits in the field for this GPIO and OR in new config */
> +	val &= ~MV88E6XXX_G2_SCRATCH_GPIO_MODE_MASK(pin);
> +	val |= (func << MV88E6XXX_G2_SCRATCH_GPIO_MODE_OFFSET(pin));
> +
> +	err = mv88e6xxx_g2_scratch_reg_write(chip, mode_reg, val);
> +	if (err)
> +		return err;
> +
> +	/* Set direction */
> +	err = mv88e6xxx_g2_scratch_reg_read(chip, dir_reg, &val);
> +	if (err)
> +		return err;
> +
> +	/* Zero bits in the field for this GPIO and OR in new config */
> +	val &= ~MV88E6XXX_G2_SCRATCH_GPIO_DIR_MASK(pin);
> +	val |= (dir << MV88E6XXX_G2_SCRATCH_GPIO_DIR_OFFSET(pin));
> +
> +	return mv88e6xxx_g2_scratch_reg_write(chip, dir_reg, val);
> +}

Would there be any value in implementing a proper gpiochip structure
here such that other pieces of SW can see this GPIO controller as a
provider and you can reference it from e.g: Device Tree using GPIO
descriptors?
-- 
Florian