Re: [net-next PATCH v3 03/11] net: pcs: Add subsystem

[Simon Horman <horms@kernel.org>]

On Thu, Apr 17, 2025 at 11:05:24AM -0400, Sean Anderson wrote:
> On 4/17/25 05:19, Simon Horman wrote:
> > On Tue, Apr 15, 2025 at 03:33:15PM -0400, Sean Anderson wrote:
> >> This adds support for getting PCS devices from the device tree. PCS
> >> drivers must first register with phylink_register_pcs. After that, MAC
> >> drivers may look up their PCS using phylink_get_pcs.
> >> 
> >> We wrap registered PCSs in another PCS. This wrapper PCS is refcounted
> >> and can outlive the wrapped PCS (such as if the wrapped PCS's driver is
> >> unbound). The wrapper forwards all PCS callbacks to the wrapped PCS,
> >> first checking to make sure the wrapped PCS still exists. This design
> >> was inspired by Bartosz Golaszewski's talk at LPC [1].
> >> 
> >> pcs_get_by_fwnode_compat is a bit hairy, but it's necessary for
> >> compatibility with existing drivers, which often attach to (devicetree)
> >> nodes directly. We use the devicetree changeset system instead of
> >> adding a (secondary) software node because mdio_bus_match calls
> >> of_driver_match_device to match devices, and that function only works on
> >> devicetree nodes.
> >> 
> >> [1] https://lpc.events/event/17/contributions/1627/
> >> 
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> > 
> > Hi Sean,
> > 
> > I noticed a few build problems after sending my previous email.
> > 
> > I was able to exercise them using variants of the following to
> > generate small configs. I include this here in case it is useful to you.
> > 
> > make tinyconfig
> > 
> > cat >> .config << __EOF__
> > CONFIG_MODULES=y
> > CONFIG_NET=y
> > CONFIG_NETDEVICES=y
> > CONFIG_PCS=y
> > CONFIG_PHYLIB=m
> > __EOF__
> > 
> > cat >> .config << __EOF__
> > CONFIG_OF=y
> > CONFIG_OF_UNITTEST=y
> > CONFIG_OF_DYNAMIC=y
> > __EOF__
> > 
> > yes "" | make oldconfig
> > 
> > ...
> > 
> >> diff --git a/drivers/net/pcs/core.c b/drivers/net/pcs/core.c
> > 
> > ...
> 
> Thanks, I was able to reproduce/fix these issues.
> 
> How did you find these? By inspection?
> 
> I often end up missing build issues like this because I mostly
> test with everything enabled.

Hi Sean,

The issue regarding fwnode_mdio_find_device and PHYLIB as a module
came up with an automated allmodconfig W=1 build.

I came across the other issue, regarding CONFIG_OF_DYNAMIC, while
investigating the PHYLIB issue.  Basically running allmodconfig takes ages
on my development machine.  So I wanted to try coming up with a small
config, based on tinyconfig, that would reproduce the PHYLIB issue. And on
the way there - as you can see artifacts of in the configuration commands
above - I hit the CONFIG_OF_DYNAMIC issue.

My take on this is that it's good to test allmodconfig, although the build
can take ages. Especially if there is any risk of link failures. And, as
for other cases, perhaps there are standard ones that are worth exercising,
like with/without IPv6 support (probably not relevant to this patchset).
But otherwise I think we get down to waiting for the bots (or someone on
the ML) to happen to come across a broken (random?) configuration.

...