[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200908192231.GB3290129@lunn.ch>
Date: Tue, 8 Sep 2020 21:22:31 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jakub Kicinski <kuba@...nel.org>
Cc: David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>, Chris Healy <cphealy@...il.com>,
Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net-next v2 5/7] net: dsa: mv88e6xxx: Add devlink regions
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@...n.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
Powered by blists - more mailing lists