Re: [PATCH AUTOSEL 5.15 41/46] net: dpaa2: publish MAC stringset to ethtool -S even if MAC is missing

[Sasha Levin]

On Mon, Dec 19, 2022 at 01:54:02PM +0200, Vladimir Oltean wrote:
> Hi Sasha,
>
> On Sun, Dec 18, 2022 at 11:12:39AM -0500, Sasha Levin wrote:
> > From: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> >
> > [ Upstream commit 29811d6e19d795efcf26644b66c4152abbac35a6 ]
> >
> > DPNIs and DPSW objects can connect and disconnect at runtime from DPMAC
> > objects on the same fsl-mc bus. The DPMAC object also holds "ethtool -S"
> > unstructured counters. Those counters are only shown for the entity
> > owning the netdev (DPNI, DPSW) if it's connected to a DPMAC.
> >
> > The ethtool stringset code path is split into multiple callbacks, but
> > currently, connecting and disconnecting the DPMAC takes the rtnl_lock().
> > This blocks the entire ethtool code path from running, see
> > ethnl_default_doit() -> rtnl_lock() -> ops->prepare_data() ->
> > strset_prepare_data().
> >
> > This is going to be a problem if we are going to no longer require
> > rtnl_lock() when connecting/disconnecting the DPMAC, because the DPMAC
> > could appear between ops->get_sset_count() and ops->get_strings().
> > If it appears out of the blue, we will provide a stringset into an array
> > that was dimensioned thinking the DPMAC wouldn't be there => array
> > accessed out of bounds.
> >
> > There isn't really a good way to work around that, and I don't want to
> > put too much pressure on the ethtool framework by playing locking games.
> > Just make the DPMAC counters be always available. They'll be zeroes if
> > the DPNI or DPSW isn't connected to a DPMAC.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> > Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> > Reviewed-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > Tested-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> > Signed-off-by: Paolo Abeni <pabeni@xxxxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > ---
>
> I think the algorithm has a problem in that it has a tendency to
> auto-pick preparatory patches which eliminate limitations that are
> preventing future development from taking place, rather than patches
> which fix present issues in the given code base.

Yeah, I'd agree. I think that the tricky part is that preperatory
patches usually resolve an issue, but it's not clear whether it's
something that affects users, or is just a theoretical limitation needed
by future patches.

> In this case, the patch is part of a larger series which was at the
> boundary between "next" work and "stable" work (patch 07/12 of this)
> https://patchwork.kernel.org/project/netdevbpf/cover/20221129141221.872653-1-vladimir.oltean@xxxxxxx/
>
> Due to the volume of that rework, I intended it to go to "next", even
> though backporting the entire series to "stable" could have its own
> merits. But picking just patch 07/12 out of that series is pointless,
> so please drop this patch from the queue for 5.15, 6.0 and 6.1, please.

Now dropped, thanks!

--
Thanks,
Sasha