On 10/7/21 6:23 AM, Russell King (Oracle) wrote:

On Tue, Oct 05, 2021 at 12:42:53PM -0400, Sean Anderson wrote:

quoted

On 10/5/21 5:48 AM, Russell King (Oracle) wrote:

quoted

On Mon, Oct 04, 2021 at 03:15:16PM -0400, Sean Anderson wrote:

quoted

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?

quoted

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?

quoted

quoted

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.

quoted

quoted

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