| Message ID | 20200908005155.3267736-1-andrew@lunn.ch |
|---|---|
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add devlink regions support | expand |
On 9/7/2020 5:51 PM, Andrew Lunn wrote: > Make use of devlink regions to allow read access to some of the > internal of the switches. The switch itself will never trigger a > region snapshot, it is assumed it is performed from user space as > needed. > > v2: > Remove left of debug print > Comment ATU format is generic to mv88e6xxx > Combine declaration and the assignment on a single line. Andrew, can you run scripts/get_maintainters.pl for your patch submissions and copy the various DSA maintainers as Vladimir who gives valuable feedback? Thanks > > Andrew Lunn (7): > net: dsa: Add helper to convert from devlink to ds > net: dsa: Add devlink regions support to DSA > net: dsa: mv88e6xxx: Move devlink code into its own file > net: dsa: mv88e6xxx: Create helper for FIDs in use > net: dsa: mv88e6xxx: Add devlink regions > net: dsa: wire up devlink info get > net: dsa: mv88e6xxx: Implement devlink info get callback > > drivers/net/dsa/mv88e6xxx/Makefile | 1 + > drivers/net/dsa/mv88e6xxx/chip.c | 290 ++---------- > drivers/net/dsa/mv88e6xxx/chip.h | 14 + > drivers/net/dsa/mv88e6xxx/devlink.c | 686 ++++++++++++++++++++++++++++ > drivers/net/dsa/mv88e6xxx/devlink.h | 21 + > include/net/dsa.h | 13 +- > net/dsa/dsa.c | 36 +- > net/dsa/dsa2.c | 19 +- > 8 files changed, 807 insertions(+), 273 deletions(-) > create mode 100644 drivers/net/dsa/mv88e6xxx/devlink.c > create mode 100644 drivers/net/dsa/mv88e6xxx/devlink.h >
On Tue, Sep 08, 2020 at 12:01:06PM -0700, Jakub Kicinski wrote: > On Tue, 8 Sep 2020 02:51:53 +0200 Andrew Lunn wrote: > > Allow ports, the global registers, and the ATU to be snapshot via > > devlink regions. > > > > v2: > > Remove left over debug prints > > Comment ATU format is generic for mv88e6xxx, not wider > > > > Signed-off-by: Andrew Lunn <andrew@lunn.ch> > > Probably best CCing devlink maintainers on devlink patches. > > Also - it's always useful to include show command outputs in the commit > message for devlink patches. Hi Jakub root@rap:~# devlink region dump mdio_bus/gpio-0:00/port5 snapshot 42 0000000000000000 0f 10 03 00 00 00 01 39 7c 00 00 00 df 07 01 00 0000000000000010 80 20 01 00 00 80 20 00 00 00 00 00 00 00 00 91 0000000000000020 00 00 00 00 00 00 00 00 00 00 00 00 22 00 00 00 0000000000000030 00 00 00 00 c0 01 00 80 00 00 00 00 00 00 00 00 Not very informative. The whole point of devlink regions is that they are suppose to be specific to a device, and you need intimate knowledge of the device to decode it. > > +#define PORT_REGION_OPS(_X_) \ > > +static struct devlink_region_ops mv88e6xxx_region_port_ ## _X_ ## _ops = { \ > > + .name = "port" #_X_, \ > > + .snapshot = mv88e6xxx_region_port_ ## _X_ ## _snapshot, \ > > + .destructor = kfree, \ > > +} > > This is a little awkward, can we make devlink pass the region pointer > back to the callback instead? Plus perhaps an ability to allocate "priv" > data inside the region would also h Yes, this API is not easy to use. I suspect it is because it was developed to support 'core dump' of the firmware, and you only have one core to dump. > > +PORT_REGION_OPS(0); > > +PORT_REGION_OPS(1); > > +PORT_REGION_OPS(2); > > +PORT_REGION_OPS(3); > > +PORT_REGION_OPS(4); > > +PORT_REGION_OPS(5); > > +PORT_REGION_OPS(6); > > +PORT_REGION_OPS(7); > > +PORT_REGION_OPS(8); > > +PORT_REGION_OPS(9); > > +PORT_REGION_OPS(10); > > +PORT_REGION_OPS(11); > > + > > +static const struct devlink_region_ops *mv88e6xxx_region_port_ops[] = { > > + &mv88e6xxx_region_port_0_ops, > > + &mv88e6xxx_region_port_1_ops, > > + &mv88e6xxx_region_port_2_ops, > > + &mv88e6xxx_region_port_3_ops, > > + &mv88e6xxx_region_port_4_ops, > > + &mv88e6xxx_region_port_5_ops, > > + &mv88e6xxx_region_port_6_ops, > > + &mv88e6xxx_region_port_7_ops, > > + &mv88e6xxx_region_port_8_ops, > > + &mv88e6xxx_region_port_9_ops, > > + &mv88e6xxx_region_port_10_ops, > > + &mv88e6xxx_region_port_11_ops, > > +}; > > Ahh, seems like regions will get a per-port incarnation as some point as > well.. Again, i think this is back to the history of dumping firmware core. I guess the existing users don't have per port CPUs which could dump a core. Andrew