On 5/23/19 5:32 PM, Florian Fainelli wrote:
On 5/23/2019 5:10 AM, Ioana Ciornei wrote:
Subject: Re: [RFC PATCH net-next 5/9] net: phylink: Add phylink_create_raw
On 5/22/2019 7:25 PM, Florian Fainelli wrote:
On 5/22/2019 6:20 PM, Ioana Ciornei wrote:
This adds a new entry point to PHYLINK that does not require a
net_device structure.
The main intended use are DSA ports that do not have net devices
registered for them (mainly because doing so would be redundant - see
Documentation/networking/dsa/dsa.rst for details). So far DSA has
been using PHYLIB fixed PHYs for these ports, driven manually with
genphy instead of starting a full PHY state machine, but this does
not scale well when there are actual PHYs that need a driver on those
ports, or when a fixed-link is requested in DT that has a speed
unsupported by the fixed PHY C22 emulation (such as SGMII-2500).
The proposed solution comes in the form of a notifier chain owned by
the PHYLINK instance, and the passing of phylink_notifier_info
structures back to the driver through a blocking notifier call.
The event API exposed by the new notifier mechanism is a 1:1 mapping
to the existing PHYLINK mac_ops, plus the PHYLINK fixed-link callback.
Both the standard phylink_create() function, as well as its raw
variant, call the same underlying function which initializes either
the netdev field or the notifier block of the PHYLINK instance.
All PHYLINK driver callbacks have been extended to call the notifier
chain in case the instance is a raw one.
Signed-off-by: Ioana Ciornei <ioana.cior...@nxp.com>
Signed-off-by: Vladimir Oltean <olte...@gmail.com>
---
[snip]
+ struct phylink_notifier_info info = {
+ .link_an_mode = pl->link_an_mode,
+ /* Discard const pointer */
+ .state = (struct phylink_link_state *)state,
+ };
+
netdev_dbg(pl->netdev,
"%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u
an=%u\n",
__func__, phylink_an_mode_str(pl->link_an_mode),
@@ -299,7 +317,12 @@ static void phylink_mac_config(struct phylink *pl,
__ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
state->pause, state->link, state->an_enabled);
Don't you need to guard that netdev_dbg() with an if (pl->ops) to
avoid de-referencing a NULL net_device?
The netdev_* print will not dereference a NULL net_device since it has explicit
checks agains this.
Instead it will just print (net/core/dev.c, __netdev_printk):
printk("%s(NULL net_device): %pV", level, vaf);
Another possibility could be to change the signature of the
phylink_mac_ops to take an opaque pointer and in the case where we
called phylink_create() and passed down a net_device pointer, we
somehow remember that for doing any operation that requires a
net_device (printing, setting carrier). We lose strict typing in doing
that, but we'd have fewer places to patch for a blocking notifier call.
Or even make those functions part of phylink_mac_ops such that the caller
could pass an .carrier_ok callback which is netif_carrier_ok() for a net_device,
else it's NULL, same with printing functions if desired...
--
Florian
Let me see if I understood this correctly. I presume that any API that we add
should not break any current PHYLINK users.
You suggest to change the prototype of the phylink_mac_ops from
void (*validate)(struct net_device *ndev, unsigned long *supported,
struct phylink_link_state *state);
to something that takes a void pointer:
void (*validate)(void *dev, unsigned long *supported,
struct phylink_link_state *state);
That is what I am suggesting, but I am also suggesting passing all
netdev specific calls that must be made as callbacks as well, so
something like:
bool (*carrier_ok)(const void *dev)
void (*carrier_set)(const void *dev, bool on)
void (*print)(const void *dev, const char *fmt)
as new members of phylink_mac_ops.
This would imply that the any function in PHYLINK would have to somehow
differentiate if the dev provided is indeed a net_device or another structure
in order to make the decision if netif_carrier_off should be called or not
(this is so we do not break any drivers using PHYLINK). I cannot see how this
judgement can be made.
You don't have to make the judgement you can just do:
if (pl->ops->carrier_set)
pl->ops->carrier_set(dev,
where dev was this opaque pointer passed to phylink_create() the first
time it was created. Like I wrote, we lose strong typing doing that, but
we don't have to update all code paths for if (pl->ops) else notifier.
Hi Florian,
Have you thought this through?
What about the totally random stuff, such as this portion from 2/9:
@@ -1187,8 +1190,10 @@ int phy_attach_direct(struct net_device *dev, struct
phy_device *phydev,
* our own module->refcnt here, otherwise we would not be able to
* unload later on.
*/
+ if (dev)
+ ndev_owner = dev->dev.parent->driver->owner;
if (ndev_owner != bus->owner && !try_module_get(bus->owner)) {
- dev_err(&dev->dev, "failed to get the bus module\n");
+ phydev_err(phydev, "failed to get the bus module\n");
return -EIO;
}
Which is in PHYLIB by the way.
Do you just add a pl->ops->owns_mdio_bus() callback? What if that code
goes away in the future? Do you remove it? This is code that all users
of phylink_create_raw will have to implement.
IMO the whole point is to change as little as possible from PHYLINK's
surface, and nothing from PHYLIB's. What you're suggesting is to change
everything, *including* phylib. And PHYLINK's print callback can't be
used in PHYLIB unless struct phylink is made public.
And if you want to replace "struct net_device *ndev" with "const void
*dev", how will you even assign the phydev->attached_dev->phydev
backlink? Another callback?
As for carrier state - realistically I don't see how any raw PHYLINK
user would implement it any other way except keep a variable for it.
Hence just let PHYLINK do it once.
I fail to see how this is cleaner.
-Vladimir
