[net-next,v2,7/7] net: dsa: mv88e6xxx: Add per port devlink regions

Add a devlink region to return the per port registers.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx/devlink.c | 109 +++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 4 deletions(-)

On Sat, Sep 26, 2020 at 11:06:32PM +0200, Andrew Lunn wrote:
> Add a devlink region to return the per port registers.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/mv88e6xxx/devlink.c | 109 +++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/devlink.c b/drivers/net/dsa/mv88e6xxx/devlink.c
> index 81e1560db206..ed74b075de84 100644
> --- a/drivers/net/dsa/mv88e6xxx/devlink.c
> +++ b/drivers/net/dsa/mv88e6xxx/devlink.c
> @@ -415,6 +415,36 @@ static int mv88e6xxx_region_atu_snapshot(struct devlink *dl,
>  	return err;
>  }
>  
> +static int mv88e6xxx_region_port_snapshot(struct devlink_port *devlink_port,
> +					  const struct devlink_port_region_ops *ops,
> +					  struct netlink_ext_ack *extack,
> +					  u8 **data)
> +{
> +	struct dsa_switch *ds = dsa_devlink_port_to_ds(devlink_port);
> +	int port = dsa_devlink_port_to_port(devlink_port);
> +	struct mv88e6xxx_chip *chip = ds->priv;

> +	u16 *registers;

I was meaning to ask this since the global regions patchset, but I
forgot.

Do we not expect to see, under the same circumstances, the same region
snapshot on a big endian and on a little endian system?

> +	int i, err;
> +
> +	registers = kmalloc_array(32, sizeof(u16), GFP_KERNEL);
> +	if (!registers)
> +		return -ENOMEM;
> +
> +	mv88e6xxx_reg_lock(chip);
> +	for (i = 0; i < 32; i++) {
> +		err = mv88e6xxx_port_read(chip, port, i, &registers[i]);
> +		if (err) {
> +			kfree(registers);
> +			goto out;
> +		}
> +	}
> +	*data = (u8 *)registers;
> +out:
> +	mv88e6xxx_reg_unlock(chip);
> +
> +	return err;
> +}
> +
>  static struct mv88e6xxx_region_priv mv88e6xxx_region_global1_priv = {
>  	.id = MV88E6XXX_REGION_GLOBAL1,
>  };
> @@ -443,6 +473,12 @@ static struct devlink_region_ops mv88e6xxx_region_atu_ops = {
>  	.destructor = kfree,
>  };
>  
> +static const struct devlink_port_region_ops mv88e6xxx_region_port_ops = {
> +	.name = "port",
> +	.snapshot = mv88e6xxx_region_port_snapshot,
> +	.destructor = kfree,
> +};
> +
>  struct mv88e6xxx_region {
>  	struct devlink_region_ops *ops;
>  	u64 size;
> @@ -471,11 +507,59 @@ mv88e6xxx_teardown_devlink_regions_global(struct mv88e6xxx_chip *chip)
>  		dsa_devlink_region_destroy(chip->regions[i]);
>  }
>  
> -void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
> +static void
> +mv88e6xxx_teardown_devlink_regions_port(struct mv88e6xxx_chip *chip,
> +					int port)
>  {
> -	struct mv88e6xxx_chip *chip = ds->priv;
> +	dsa_devlink_region_destroy(chip->ports[port].region);
> +}
>  
> -	mv88e6xxx_teardown_devlink_regions_global(chip);
> +static int mv88e6xxx_setup_devlink_regions_port(struct dsa_switch *ds,
> +						struct mv88e6xxx_chip *chip,
> +						int port)
> +{
> +	struct devlink_region *region;
> +
> +	region = dsa_devlink_port_region_create(ds,
> +						port,
> +						&mv88e6xxx_region_port_ops, 1,
> +						32 * sizeof(u16));
> +	if (IS_ERR(region))
> +		return PTR_ERR(region);
> +
> +	chip->ports[port].region = region;
> +
> +	return 0;
> +}
> +
> +static void
> +mv88e6xxx_teardown_devlink_regions_ports(struct mv88e6xxx_chip *chip)
> +{
> +	int port;
> +
> +	for (port = 0; port < mv88e6xxx_num_ports(chip); port++)
> +		mv88e6xxx_teardown_devlink_regions_port(chip, port);
> +}
> +
> +static int mv88e6xxx_setup_devlink_regions_ports(struct dsa_switch *ds,
> +						 struct mv88e6xxx_chip *chip)
> +{
> +	int port, port_err;
> +	int err;
> +
> +	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
> +		err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
> +		if (err)
> +			goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	for (port_err = 0; port_err < port; port_err++)

"while (port-- >= 0)" should do the trick. Also, maybe it would be
overall more aesthetic if you wouldn't use the goto and have 2 exit
points in such a small function, but a simple break here. Like this:

	int err;

	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
		err = mv88e6xxx_setup_devlink_regions_port(ds, chip, port);
		if (err) {
			while (port-- >= 0)
				mv88e6xxx_teardown_devlink_regions_port(chip,
									port);
			break;
		}
	}

	return err;

> +		mv88e6xxx_teardown_devlink_regions_port(chip, port_err);
> +
> +	return err;
>  }
>  
>  static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
> @@ -511,8 +595,25 @@ static int mv88e6xxx_setup_devlink_regions_global(struct dsa_switch *ds,
>  int mv88e6xxx_setup_devlink_regions(struct dsa_switch *ds)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
> +	int err;
> +
> +	err = mv88e6xxx_setup_devlink_regions_global(ds, chip);
> +	if (err)
> +		return err;
> +
> +	err = mv88e6xxx_setup_devlink_regions_ports(ds, chip);
> +	if (err)
> +		mv88e6xxx_teardown_devlink_regions_global(chip);
>  
> -	return mv88e6xxx_setup_devlink_regions_global(ds, chip);
> +	return err;
> +}
> +
> +void mv88e6xxx_teardown_devlink_regions(struct dsa_switch *ds)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +
> +	mv88e6xxx_teardown_devlink_regions_ports(chip);
> +	mv88e6xxx_teardown_devlink_regions_global(chip);
>  }
>  
>  int mv88e6xxx_devlink_info_get(struct dsa_switch *ds,
> -- 
> 2.28.0
>

> I was meaning to ask this since the global regions patchset, but I
> forgot.
> 
> Do we not expect to see, under the same circumstances, the same region
> snapshot on a big endian and on a little endian system?

We have never had any issues with endinness with MDIO. PHY/DSA drivers
work with host endian. The MDIO bus controller does what it needs to
do when shifting the bits out, as required by class 22 or 45.

netlink in general assume host endian, as far as i know. So a big
endian and a little endian snapshot are going to be different.

Arnd did an interesting presentation for LPC. He basically shows that
big endian is going away, with the exception of IBM big iron. I don't
expect an IBM Z to have a DSA switch!

       Andrew

On Sun, Sep 27, 2020 at 03:03:00AM +0200, Andrew Lunn wrote:
> > I was meaning to ask this since the global regions patchset, but I
> > forgot.
> >
> > Do we not expect to see, under the same circumstances, the same region
> > snapshot on a big endian and on a little endian system?
>
> We have never had any issues with endinness with MDIO. PHY/DSA drivers
> work with host endian. The MDIO bus controller does what it needs to
> do when shifting the bits out, as required by class 22 or 45.
>
> netlink in general assume host endian, as far as i know. So a big
> endian and a little endian snapshot are going to be different.

If the binary form of the snapshot is supposed to be consumed and done
with immediately after the netlink communication is over, sure. But it
is presented to the user, and in fact this is even the way it is
presented by default, with devlink, there's no pretty-printing.

> Arnd did an interesting presentation for LPC. He basically shows that
> big endian is going away, with the exception of IBM big iron. I don't
> expect an IBM Z to have a DSA switch!

Tell that to my T1040 chip with an 8-port Seville switch driven by the
Ocelot driver.

Also, armv8 can also boot in big-endian mode.