| Message ID | 20211004191527.1610759-6-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for Xilinx PCS | expand |
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. 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? 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?
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. So what can the device in net_device be used for? > 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. > 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. --Sean
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. > > 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 > > 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. 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. > > 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". phylink (or any other subsystem) does not have the power to say "I don't want XYZ to be removed". 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.
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
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 6387c40c5592..046fdac3597d 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -795,6 +795,42 @@ static int phylink_register_sfp(struct phylink *pl, return ret; } +static LIST_HEAD(pcs_devices); +static DEFINE_MUTEX(pcs_mutex); + +/** + * phylink_register_pcs() - register a new PCS + * @pcs: the PCS to register + * + * Registers a new PCS which can be automatically attached to a phylink. + * + * Return: 0 on success, or -errno on error + */ +int phylink_register_pcs(struct phylink_pcs *pcs) +{ + if (!pcs->dev || !pcs->ops) + return -EINVAL; + + INIT_LIST_HEAD(&pcs->list); + mutex_lock(&pcs_mutex); + list_add(&pcs->list, &pcs_devices); + mutex_unlock(&pcs_mutex); + return 0; +} +EXPORT_SYMBOL_GPL(phylink_register_pcs); + +/** + * phylink_unregister_pcs() - unregister a PCS + * @pcs: a PCS previously registered with phylink_register_pcs() + */ +void phylink_unregister_pcs(struct phylink_pcs *pcs) +{ + mutex_lock(&pcs_mutex); + list_del(&pcs->list); + mutex_unlock(&pcs_mutex); +} +EXPORT_SYMBOL_GPL(phylink_unregister_pcs); + /** * phylink_set_pcs() - set the current PCS for phylink to use * @pl: a pointer to a &struct phylink returned from phylink_create() @@ -808,14 +844,72 @@ static int phylink_register_sfp(struct phylink *pl, * callback if a PCS is present (denoting a newer setup) so removing a PCS * is not supported, and if a PCS is going to be used, it must be registered * by calling phylink_set_pcs() at the latest in the first mac_config() call. + * + * Context: may sleep. + * Return: 0 on success or -errno on failure. */ -void phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs) +int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs) { + if (pl->pcs && pl->pcs->dev) + device_link_remove(pl->dev, pl->pcs->dev); + + if (pcs->dev) { + struct device_link *dl = + device_link_add(pl->dev, pcs->dev, 0); + + if (IS_ERR(dl)) { + dev_err(pl->dev, + "failed to create device link to %s\n", + dev_name(pcs->dev)); + return PTR_ERR(dl); + } + } + pl->pcs = pcs; pl->pcs_ops = pcs->ops; + return 0; } EXPORT_SYMBOL_GPL(phylink_set_pcs); +static struct phylink_pcs *phylink_find_pcs(struct fwnode_handle *fwnode) +{ + struct phylink_pcs *pcs; + + mutex_lock(&pcs_mutex); + list_for_each_entry(pcs, &pcs_devices, list) { + if (pcs->dev && pcs->dev->fwnode == fwnode) { + mutex_unlock(&pcs_mutex); + return pcs; + } + } + mutex_unlock(&pcs_mutex); + + return NULL; +} + +static int phylink_attach_pcs(struct phylink *pl, struct fwnode_handle *fwnode) +{ + int ret; + struct phylink_pcs *pcs; + struct fwnode_reference_args ref; + + ret = fwnode_property_get_reference_args(fwnode, "pcs", NULL, + 0, 0, &ref); + if (ret == -ENOENT) + return 0; + else if (ret) + return ret; + + pcs = phylink_find_pcs(ref.fwnode); + if (pcs) + ret = phylink_set_pcs(pl, pcs); + else + ret = -EPROBE_DEFER; + + fwnode_handle_put(ref.fwnode); + return ret; +} + /** * phylink_create() - create a phylink instance * @config: a pointer to the target &struct phylink_config @@ -893,12 +987,20 @@ struct phylink *phylink_create(struct phylink_config *config, pl->cur_link_an_mode = pl->cfg_link_an_mode; ret = phylink_register_sfp(pl, fwnode); - if (ret < 0) { - kfree(pl); - return ERR_PTR(ret); - } + if (ret < 0) + goto err_sfp; + + ret = phylink_attach_pcs(pl, fwnode); + if (ret) + goto err_pcs; return pl; + +err_pcs: + sfp_bus_del_upstream(pl->sfp_bus); +err_sfp: + kfree(pl); + return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(phylink_create); @@ -913,6 +1015,9 @@ EXPORT_SYMBOL_GPL(phylink_create); */ void phylink_destroy(struct phylink *pl) { + if (pl->pcs && pl->pcs->dev) + device_link_remove(pl->dev, pl->pcs->dev); + sfp_bus_del_upstream(pl->sfp_bus); if (pl->link_gpio) gpiod_put(pl->link_gpio); diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 237291196ce2..d60756b36ad3 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -331,14 +331,20 @@ struct phylink_pcs_ops; /** * struct phylink_pcs - PHYLINK PCS instance + * @dev: the device associated with this PCS, or %NULL if the PCS doesn't have + * a device of its own. Typically, @dev should only be %NULL for internal + * PCS devices which do not need to be looked up via phandle. * @ops: a pointer to the &struct phylink_pcs_ops structure + * @list: internal list of PCS devices * @poll: poll the PCS for link changes * * This structure is designed to be embedded within the PCS private data, * and will be passed between phylink and the PCS. */ struct phylink_pcs { + struct device *dev; const struct phylink_pcs_ops *ops; + struct list_head list; bool poll; }; @@ -433,10 +439,13 @@ void pcs_link_up(struct phylink_pcs *pcs, unsigned int mode, phy_interface_t interface, int speed, int duplex); #endif +int phylink_register_pcs(struct phylink_pcs *pcs); +void phylink_unregister_pcs(struct phylink_pcs *pcs); +int phylink_set_pcs(struct phylink *pl, struct phylink_pcs *pcs); + struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *, phy_interface_t iface, const struct phylink_mac_ops *mac_ops); -void phylink_set_pcs(struct phylink *, struct phylink_pcs *pcs); void phylink_destroy(struct phylink *); int phylink_connect_phy(struct phylink *, struct phy_device *);
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? Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- drivers/net/phy/phylink.c | 115 ++++++++++++++++++++++++++++++++++++-- include/linux/phylink.h | 11 +++- 2 files changed, 120 insertions(+), 6 deletions(-)