Re: [RFC net-next PATCH 05/16] net: phylink: Automatically attach PCS devices

[Sean Anderson]

On 10/7/21 6:23 AM, Russell King (Oracle) wrote:
> On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:
> > On 10/5/21 5:48 AM, Russell King (Oracle) wrote:
> > > On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:
> > > > This adds support for automatically attaching PCS devices when creating
> > > > a phylink. To do this, drivers must first register with
> > > > phylink_register_pcs. After that, new phylinks will attach the PCS
> > > > device specified by the "pcs" property.
> > > >
> > > > At the moment there is no support for specifying the interface used to
> > > > talk to the PCS. The MAC driver is expected to know how to talk to the
> > > > PCS. This is not a change, but it is perhaps an area for improvement.
> > > >
> > > > I believe this is mostly correct with regard to registering/
> > > > unregistering. However I am not too familiar with the guts of Linux's
> > > > device subsystem. It is possible (likely, even) that the current system
> > > > is insufficient to prevent removing PCS devices which are still in-use.
> > > > I would really appreciate any feedback, or suggestions of subsystems to
> > > > use as reference. In particular: do I need to manually create device
> > > > links? Should I instead add an entry to of_supplier_bindings? Do I need
> > > > a call to try_module_get?
> > >
> > > I think this is an area that needs to be thought about carefully.
> > > Things are not trivial here.
> > >
> > > The first mistake I see below is the use of device links. pl->dev is
> > > the "struct device" embedded within "struct net_device". This doesn't
> > > have a driver associated with it, and so using device links is likely
> > > ineffectual.

Ok, so the 'real' device is actually the parent of pl->netdev->dev?

> > So what can the device in net_device be used for?
>
> That is used for the class device that is commonly found in
> /sys/devices/$pathtothedevice/net/$interfacename

By the way, why don't we set pl->dev = config->dev->parent in
phylink_create() when config->type == PHYLINK_NETDEV?

> > > Even with the right device, I think careful thought is needed - we have
> > > network drivers where one "struct device" contains multiple network
> > > interfaces. Should the removal of a PCS from one network interface take
> > > out all of them?
> >
> > Well, it's more of the other way around. We need to prevent removing the
> > PCS while it is still in-use.
>
> devlinks don't do that - if the "producer" device goes away, they force
> the "consumer" device to be unbound.

Ah, I didn't realize that was the relationship being modeled.

> As I mention above, the "consumer" device, which would be the device
> providing the network interface(s) could have more than one interface
> and unbinding it could have drastic consequences for the platform.

Well, then don't unbind the PCS ;)

After reviewing several other subsystems, I think the correct way to
approach this is to add an entry to of_supplier_bindings, which will
help out with ordering, and get the module when looking up the PCS. That
is, something like

int phylink_get_pcs(fwnode, struct phylink_pcs **pcs)
{
	int ret;
	struct fwnode_reference_args ref;

	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle", NULL,
						 0, 0, &ref);
	if (ret)
		return ret;

	*pcs = phylink_find_pcs(ref.fwnode);
	fwnode_handle_put(ref.fwnode);
	if (!*pcs)
		return -EPROBE_DEFER;

	if (!try_module_get(*pcs->owner))
		return -EBUSY;
	return 0;
}

phylink_put_pcs(pcs)
{
	module_put(pcs->owner);
}

and keep phylink_set as-is (the above should be considered along with my comments on patch 10).

Realistically, the only time a PCS is optional is if there isn't a PCS
reference in the device tree.

> > > Alternatively, could we instead use phylink to "unplug" the PCS and
> > > mark the link down - would that be a better approach than trying to
> > > use device links?
> >
> > So here, I think the logic should be: allow phylink to "unplug" the PCS
> > only when the link is down.
>
> When a device is unbound from its driver, the driver has no say in
> whether that goes ahead or not. Think about it as grabbing that USB
> stick plugged into your computer and you yanking it out. None of the
> software gets a look in to say "don't do that".

I suspect the vast majority of PCSs will be DEVICE_FIXED.

> phylink (or any other subsystem) does not have the power to say
> "I don't want XYZ to be removed".

However, we do have the power to say "I don't want XYZ's module to be
removed", which should cover most of the situations where a device is
removed after boot.

> Yes, it's harder to do that with PCS, but my point is that if one asks
> the driver model to unbind the PCS driver from the PCS device, then
> the driver model will do that whether the PCS driver wants to allow it
> at that moment or not. It isn't something the PCS driver can prevent.
>
> One can tell the driver model not to expose the bind/unbind attributes
> for the driver, but that doesn't stop the unbind happening should the
> struct device actually go away.
>
> So, IMHO, it's better to design assuming that components will go away
> at an inconvenient time and deal with it gracefully.

See above, but I think it's better here to assume that components will
stick around and if they disappear at an inconvenient time then we
should just let the netdev be removed as well.

--Sean