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

[Sean Anderson <sean.anderson@seco.com>]

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