netdev - Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Add helper for accessing port registers

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]

Date:   Mon, 19 Sep 2016 18:53:12 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>
Cc:     netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next 1/2] net: dsa: mv88e6xxx: Add helper for accessing port registers

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

> @@ -41,6 +41,11 @@ static void assert_reg_lock(struct mv88e6xxx_chip *chip)
>  	}
>  }
>  
> +static int mv88e6xxx_reg_port(struct mv88e6xxx_chip *chip, int port)
> +{
> +	return chip->info->port_base_addr + port;
> +}
> +

If we really want such helper, can you call it mv88e6xxx_port_addr()
instead, so that we respect an implicit mv88e6xxx_port_ namespace, and
use the correct "addr" term instead of erroneous "reg" one.

>  /* The switch ADDR[4:1] configuration pins define the chip SMI device address
>   * (ADDR[0] is always zero, thus only even SMI addresses can be strapped).
>   *
> @@ -216,6 +221,42 @@ int mv88e6xxx_write(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val)
>  	return 0;
>  }
>  
> +int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
> +			u16 *val)
> +{
> +	int addr = mv88e6xxx_reg_port(chip, port);
> +	int err;
> +
> +	assert_reg_lock(chip);
> +
> +	err = mv88e6xxx_smi_read(chip, addr, reg, val);
> +	if (err)
> +		return err;
> +
> +	dev_dbg(chip->dev, "<- port: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
> +		port, reg, *val);
> +
> +	return 0;
> +}
> +
> +int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,
> +			 u16 val)
> +{
> +	int addr = mv88e6xxx_reg_port(chip, port);
> +	int err;
> +
> +	assert_reg_lock(chip);
> +
> +	err = mv88e6xxx_smi_write(chip, addr, reg, val);
> +	if (err)
> +		return err;
> +
> +	dev_dbg(chip->dev, "-> port: 0x%.2x reg: 0x%.2x val: 0x%.4x\n",
> +		port, reg, val);
> +
> +	return 0;
> +}
> +

mv88e6xxx_{read,write} are already doing this (wrapping the assert, smi
op and debug message). Plus, we could access the port registers through
a different interface, like remote management frames.

So please don't duplicate and use the following, as the previous
mv88e6xxx_port_read() function was doing:

    static int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip,
                                   int port, int reg, u16 *val)
    {
            int addr = chip->info->port_base_addr + port;

            return mv88e6xxx_read(chip, addr, reg, val);
    }

    static int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip,
                                    int port, int reg, u16 val)
    {
            int addr = chip->info->port_base_addr + port;

            return mv88e6xxx_write(chip, addr, reg, val);
    }

Note: I don't really see a need for the mv88e6xxx_port_addr helper in
fact, the above code is quite clear. I'd suggest to drop it unless we
need a port address somewhere else than in mv88e6xxx_port_{read,write}.

>  static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
>  			      int reg, u16 *val)
>  {
> @@ -585,19 +626,19 @@ static void mv88e6xxx_adjust_link(struct dsa_switch *ds, int port,
>  				  struct phy_device *phydev)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
> -	u32 reg;
> -	int ret;
> +	u16 reg;
> +	int err;
>  
>  	if (!phy_is_pseudo_fixed_link(phydev))
>  		return;
>  
>  	mutex_lock(&chip->reg_lock);
>  
> -	ret = _mv88e6xxx_reg_read(chip, REG_PORT(port), PORT_PCS_CTRL);
> -	if (ret < 0)
> +	err = mv88e6xxx_port_read(chip, port, PORT_PCS_CTRL, &reg);
> +	if (err < 0)
>  		goto out;

Can you please get rid of the < 0 condition at the same time, if (err)
is enough.

(same for the rest of this patch).

Thanks,

        Vivien