| Message ID | 20190523011958.14944-6-ioana.ciornei@nxp.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Decoupling PHYLINK from struct net_device | expand |
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.ciornei@nxp.com> > Signed-off-by: Vladimir Oltean <olteanv@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? 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.
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.ciornei@nxp.com> >> Signed-off-by: Vladimir Oltean <olteanv@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? > > 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...
> 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.ciornei@nxp.com> > >> Signed-off-by: Vladimir Oltean <olteanv@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); 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. -- Ioana
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.ciornei@nxp.com> >>>> Signed-off-by: Vladimir Oltean <olteanv@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.
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.ciornei@nxp.com> >>>>> Signed-off-by: Vladimir Oltean <olteanv@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
On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote: > @@ -111,7 +114,16 @@ static const char *phylink_an_mode_str(unsigned int mode) > static int phylink_validate(struct phylink *pl, unsigned long *supported, > struct phylink_link_state *state) > { > - pl->ops->validate(pl->netdev, supported, state); > + struct phylink_notifier_info info = { > + .supported = supported, > + .state = state, > + }; > + > + if (pl->ops) > + pl->ops->validate(pl->netdev, supported, state); > + else > + blocking_notifier_call_chain(&pl->notifier_chain, > + PHYLINK_VALIDATE, &info); I don't like this use of notifiers for several reasons: 1. It becomes harder to grep for users of this. 2. We lose documentation about what is passed for each method. 3. We lose const-ness for parameters, which then allows users to modify phylink-internal data structures inappropriately from these notifier calls. Please find another way.
On 5/23/19 1:32 PM, Vladimir Oltean wrote: > 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.ciornei@nxp.com> >>>>>> Signed-off-by: Vladimir Oltean <olteanv@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? Not to the point of seeing the problems you are highlighting. > 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. Fine, it's not.
On Fri, 24 May 2019 at 00:28, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote: > > @@ -111,7 +114,16 @@ static const char *phylink_an_mode_str(unsigned int mode) > > static int phylink_validate(struct phylink *pl, unsigned long *supported, > > struct phylink_link_state *state) > > { > > - pl->ops->validate(pl->netdev, supported, state); > > + struct phylink_notifier_info info = { > > + .supported = supported, > > + .state = state, > > + }; > > + > > + if (pl->ops) > > + pl->ops->validate(pl->netdev, supported, state); > > + else > > + blocking_notifier_call_chain(&pl->notifier_chain, > > + PHYLINK_VALIDATE, &info); > > I don't like this use of notifiers for several reasons: > > 1. It becomes harder to grep for users of this. > 2. We lose documentation about what is passed for each method. > 3. We lose const-ness for parameters, which then allows users to > modify phylink-internal data structures inappropriately from > these notifier calls. > > Please find another way. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up Hi Russell, Items 2 and 3 can be addressed by creating an union of structures in struct phylink_notifier_info, just like switchdev does. For 1 (grep), you mean that the notifiers are upper-case and the regular callbacks are lower-case? -Vladimir
On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote: > + if (pl->ops) { > + pl->ops->mac_link_up(ndev, pl->link_an_mode, > pl->phy_state.interface, > pl->phydev); > > + netif_carrier_on(ndev); > > + netdev_info(ndev, > + "Link is Up - %s/%s - flow control %s\n", > + phy_speed_to_str(link_state.speed), > + phy_duplex_to_str(link_state.duplex), > + phylink_pause_to_str(link_state.pause)); > + } else { > + blocking_notifier_call_chain(&pl->notifier_chain, > + PHYLINK_MAC_LINK_UP, &info); > + phydev_info(pl->phydev, > + "Link is Up - %s/%s - flow control %s\n", > + phy_speed_to_str(link_state.speed), > + phy_duplex_to_str(link_state.duplex), > + phylink_pause_to_str(link_state.pause)); > + } So if we don't have pl->ops, what happens when we call phydev_info() with a NULL phydev, which is a very real possibility: one of phylink's whole points is to support dynamic presence of a PHY. What will happen in that case is this will oops, due to dereferencing an offset NULL pointer via: #define phydev_info(_phydev, format, args...) \ dev_info(&_phydev->mdio.dev, format, ##args) You can't just decide that if there's no netdev, we will be guaranteed a phy.
On Fri, 24 May 2019 at 00:55, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote: > > + if (pl->ops) { > > + pl->ops->mac_link_up(ndev, pl->link_an_mode, > > pl->phy_state.interface, > > pl->phydev); > > > > + netif_carrier_on(ndev); > > > > + netdev_info(ndev, > > + "Link is Up - %s/%s - flow control %s\n", > > + phy_speed_to_str(link_state.speed), > > + phy_duplex_to_str(link_state.duplex), > > + phylink_pause_to_str(link_state.pause)); > > + } else { > > + blocking_notifier_call_chain(&pl->notifier_chain, > > + PHYLINK_MAC_LINK_UP, &info); > > + phydev_info(pl->phydev, > > + "Link is Up - %s/%s - flow control %s\n", > > + phy_speed_to_str(link_state.speed), > > + phy_duplex_to_str(link_state.duplex), > > + phylink_pause_to_str(link_state.pause)); > > + } > > So if we don't have pl->ops, what happens when we call phydev_info() > with a NULL phydev, which is a very real possibility: one of phylink's > whole points is to support dynamic presence of a PHY. > > What will happen in that case is this will oops, due to dereferencing > an offset NULL pointer via: > > #define phydev_info(_phydev, format, args...) \ > dev_info(&_phydev->mdio.dev, format, ##args) > > You can't just decide that if there's no netdev, we will be guaranteed > a phy. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up True, however it does not crash: [ 2.539949] (NULL device *): Link is Up - 1Gbps/Full - flow control off I agree that a better printing system has to be established though. -Vladimir
On Fri, May 24, 2019 at 01:04:01AM +0300, Vladimir Oltean wrote: > On Fri, 24 May 2019 at 00:55, Russell King - ARM Linux admin > <linux@armlinux.org.uk> wrote: > > > > On Thu, May 23, 2019 at 01:20:40AM +0000, Ioana Ciornei wrote: > > > + if (pl->ops) { > > > + pl->ops->mac_link_up(ndev, pl->link_an_mode, > > > pl->phy_state.interface, > > > pl->phydev); > > > > > > + netif_carrier_on(ndev); > > > > > > + netdev_info(ndev, > > > + "Link is Up - %s/%s - flow control %s\n", > > > + phy_speed_to_str(link_state.speed), > > > + phy_duplex_to_str(link_state.duplex), > > > + phylink_pause_to_str(link_state.pause)); > > > + } else { > > > + blocking_notifier_call_chain(&pl->notifier_chain, > > > + PHYLINK_MAC_LINK_UP, &info); > > > + phydev_info(pl->phydev, > > > + "Link is Up - %s/%s - flow control %s\n", > > > + phy_speed_to_str(link_state.speed), > > > + phy_duplex_to_str(link_state.duplex), > > > + phylink_pause_to_str(link_state.pause)); > > > + } > > > > So if we don't have pl->ops, what happens when we call phydev_info() > > with a NULL phydev, which is a very real possibility: one of phylink's > > whole points is to support dynamic presence of a PHY. > > > > What will happen in that case is this will oops, due to dereferencing > > an offset NULL pointer via: > > > > #define phydev_info(_phydev, format, args...) \ > > dev_info(&_phydev->mdio.dev, format, ##args) > > > > You can't just decide that if there's no netdev, we will be guaranteed > > a phy. > > > > -- > > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > > According to speedtest.net: 11.9Mbps down 500kbps up > > True, however it does not crash: > > [ 2.539949] (NULL device *): Link is Up - 1Gbps/Full - flow control off > > I agree that a better printing system has to be established though. The only reason that happens is because struct mdio_device is at the start of struct phy_device, and struct device is at the start of struct mdio_device. Should either of these move, that breaks and we get an oops. Sorry, that's way too fragile. Plus, of course, do we think that printing "(NULL device *):" is really acceptable? We completely lose any information about _what_ link came up or went down.
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 68cfe31240cc..7b6b233c1a07 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -18,6 +18,7 @@ #include <linux/spinlock.h> #include <linux/timer.h> #include <linux/workqueue.h> +#include <linux/notifier.h> #include "sfp.h" #include "swphy.h" @@ -41,6 +42,8 @@ struct phylink { /* private: */ struct net_device *netdev; const struct phylink_mac_ops *ops; + struct notifier_block *nb; + struct blocking_notifier_head notifier_chain; unsigned long phylink_disable_state; /* bitmask of disables */ struct phy_device *phydev; @@ -111,7 +114,16 @@ static const char *phylink_an_mode_str(unsigned int mode) static int phylink_validate(struct phylink *pl, unsigned long *supported, struct phylink_link_state *state) { - pl->ops->validate(pl->netdev, supported, state); + struct phylink_notifier_info info = { + .supported = supported, + .state = state, + }; + + if (pl->ops) + pl->ops->validate(pl->netdev, supported, state); + else + blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_VALIDATE, &info); return phylink_is_empty_linkmode(supported) ? -EINVAL : 0; } @@ -290,6 +302,12 @@ static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode) static void phylink_mac_config(struct phylink *pl, const struct phylink_link_state *state) { + 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); - pl->ops->mac_config(pl->netdev, pl->link_an_mode, state); + if (pl->ops) + pl->ops->mac_config(pl->netdev, pl->link_an_mode, state); + else + blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_MAC_CONFIG, &info); + } static void phylink_mac_config_up(struct phylink *pl, @@ -311,13 +334,22 @@ static void phylink_mac_config_up(struct phylink *pl, static void phylink_mac_an_restart(struct phylink *pl) { - if (pl->link_config.an_enabled && - phy_interface_mode_is_8023z(pl->link_config.interface)) + if (!pl->link_config.an_enabled && + !phy_interface_mode_is_8023z(pl->link_config.interface)) + return; + + if (pl->ops) pl->ops->mac_an_restart(pl->netdev); + else + blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_MAC_AN_RESTART, NULL); } static int phylink_get_mac_state(struct phylink *pl, struct phylink_link_state *state) { + struct phylink_notifier_info info = { + .state = state, + }; struct net_device *ndev = pl->netdev; linkmode_copy(state->advertising, pl->link_config.advertising); @@ -330,7 +362,12 @@ static int phylink_get_mac_state(struct phylink *pl, struct phylink_link_state * state->an_complete = 0; state->link = 1; - return pl->ops->mac_link_state(ndev, state); + if (pl->ops) + return pl->ops->mac_link_state(ndev, state); + else + return blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_MAC_LINK_STATE, + &info); } /* The fixed state is... fixed except for the link state, @@ -338,9 +375,17 @@ static int phylink_get_mac_state(struct phylink *pl, struct phylink_link_state * */ static void phylink_get_fixed_state(struct phylink *pl, struct phylink_link_state *state) { + struct phylink_notifier_info info = { + .state = state, + }; + *state = pl->link_config; if (pl->get_fixed_state) pl->get_fixed_state(pl->netdev, state); + else if (pl->nb) + blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_GET_FIXED_STATE, + &info); else if (pl->link_gpio) state->link = !!gpiod_get_value_cansleep(pl->link_gpio); } @@ -399,28 +444,54 @@ static void phylink_mac_link_up(struct phylink *pl, struct phylink_link_state link_state) { struct net_device *ndev = pl->netdev; + struct phylink_notifier_info info = { + .link_an_mode = pl->link_an_mode, + .interface = pl->phy_state.interface, + .phydev = pl->phydev, + }; - pl->ops->mac_link_up(ndev, pl->link_an_mode, + if (pl->ops) { + pl->ops->mac_link_up(ndev, pl->link_an_mode, pl->phy_state.interface, pl->phydev); - netif_carrier_on(ndev); + netif_carrier_on(ndev); - netdev_info(ndev, - "Link is Up - %s/%s - flow control %s\n", - phy_speed_to_str(link_state.speed), - phy_duplex_to_str(link_state.duplex), - phylink_pause_to_str(link_state.pause)); + netdev_info(ndev, + "Link is Up - %s/%s - flow control %s\n", + phy_speed_to_str(link_state.speed), + phy_duplex_to_str(link_state.duplex), + phylink_pause_to_str(link_state.pause)); + } else { + blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_MAC_LINK_UP, &info); + phydev_info(pl->phydev, + "Link is Up - %s/%s - flow control %s\n", + phy_speed_to_str(link_state.speed), + phy_duplex_to_str(link_state.duplex), + phylink_pause_to_str(link_state.pause)); + } } static void phylink_mac_link_down(struct phylink *pl) { struct net_device *ndev = pl->netdev; + struct phylink_notifier_info info = { + .link_an_mode = pl->link_an_mode, + .interface = pl->phy_state.interface, + .phydev = pl->phydev, + }; - netif_carrier_off(ndev); - pl->ops->mac_link_down(ndev, pl->link_an_mode, + if (pl->ops) { + netif_carrier_off(ndev); + pl->ops->mac_link_down(ndev, pl->link_an_mode, pl->phy_state.interface); - netdev_info(ndev, "Link is Down\n"); + netdev_info(ndev, "Link is Down\n"); + } else { + blocking_notifier_call_chain(&pl->notifier_chain, + PHYLINK_MAC_LINK_DOWN, &info); + phydev_info(pl->phydev, "Link is Down\n"); + } } static void phylink_resolve(struct work_struct *w) @@ -477,7 +548,10 @@ static void phylink_resolve(struct work_struct *w) } } - if (link_state.link != netif_carrier_ok(ndev)) { + /* Take the branch without checking the carrier status + * if there is no netdevice. + */ + if (!pl->ops || link_state.link != netif_carrier_ok(ndev)) { if (!link_state.link) phylink_mac_link_down(pl); else @@ -546,24 +620,12 @@ static int phylink_register_sfp(struct phylink *pl, return 0; } -/** - * phylink_create() - create a phylink instance - * @ndev: a pointer to the &struct net_device - * @fwnode: a pointer to a &struct fwnode_handle describing the network - * interface - * @iface: the desired link mode defined by &typedef phy_interface_t - * @ops: a pointer to a &struct phylink_mac_ops for the MAC. - * - * Create a new phylink instance, and parse the link parameters found in @np. - * This will parse in-band modes, fixed-link or SFP configuration. - * - * Returns a pointer to a &struct phylink, or an error-pointer value. Users - * must use IS_ERR() to check for errors from this function. - */ -struct phylink *phylink_create(struct net_device *ndev, - struct fwnode_handle *fwnode, - phy_interface_t iface, - const struct phylink_mac_ops *ops) +static inline struct phylink * +__phylink_create_raw(struct net_device *ndev, + struct notifier_block *nb, + struct fwnode_handle *fwnode, + phy_interface_t iface, + const struct phylink_mac_ops *ops) { struct phylink *pl; int ret; @@ -574,7 +636,17 @@ struct phylink *phylink_create(struct net_device *ndev, mutex_init(&pl->state_mutex); INIT_WORK(&pl->resolve, phylink_resolve); - pl->netdev = ndev; + + if (ndev) { + pl->netdev = ndev; + } else if (nb) { + BLOCKING_INIT_NOTIFIER_HEAD(&pl->notifier_chain); + blocking_notifier_chain_register(&pl->notifier_chain, nb); + pl->nb = nb; + } else { + return ERR_PTR(-EINVAL); + } + pl->phy_state.interface = iface; pl->link_interface = iface; if (iface == PHY_INTERFACE_MODE_MOCA) @@ -616,8 +688,52 @@ struct phylink *phylink_create(struct net_device *ndev, return pl; } + +/** + * phylink_create() - create a phylink instance + * @ndev: a pointer to the &struct net_device + * @fwnode: a pointer to a &struct fwnode_handle describing the network + * interface + * @iface: the desired link mode defined by &typedef phy_interface_t + * @ops: a pointer to a &struct phylink_mac_ops for the MAC. + * + * Create a new phylink instance, and parse the link parameters found in @np. + * This will parse in-band modes, fixed-link or SFP configuration. + * + * Returns a pointer to a &struct phylink, or an error-pointer value. Users + * must use IS_ERR() to check for errors from this function. + */ +struct phylink *phylink_create(struct net_device *ndev, + struct fwnode_handle *fwnode, + phy_interface_t iface, + const struct phylink_mac_ops *ops) +{ + return __phylink_create_raw(ndev, NULL, fwnode, iface, ops); +} EXPORT_SYMBOL_GPL(phylink_create); +/** + * phylink_create_raw() - create a raw phylink instance + * @nb: a pointer to a struct notifier_block which which will be called on + * on phylink events + * @fwnode: a pointer to a &struct fwnode_handle describing the network + * interface + * @iface: the desired link mode defined by &typedef phy_interface_t + * + * Create a new raw phylink instance, and parse the link parameters found in + * @np. This will parse in-band modes, fixed-link or SFP configuration. + * + * Returns a pointer to a &struct phylink, or an error-pointer value. Users + * must use IS_ERR() to check for errors from this function. + */ +struct phylink *phylink_create_raw(struct notifier_block *nb, + struct fwnode_handle *fwnode, + phy_interface_t iface) +{ + return __phylink_create_raw(NULL, nb, fwnode, iface, NULL); +} +EXPORT_SYMBOL_GPL(phylink_create_raw); + /** * phylink_destroy() - cleanup and destroy the phylink instance * @pl: a pointer to a &struct phylink returned from phylink_create() @@ -909,7 +1025,8 @@ void phylink_start(struct phylink *pl) phy_modes(pl->link_config.interface)); /* Always set the carrier off */ - netif_carrier_off(pl->netdev); + if (pl->netdev) + netif_carrier_off(pl->netdev); /* Apply the link configuration to the MAC when starting. This allows * a fixed-link to start with the correct parameters, and also diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 6411c624f63a..d171156eab4e 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -54,6 +54,24 @@ struct phylink_link_state { unsigned int an_complete:1; }; +enum phylink_cmd { + PHYLINK_VALIDATE = 1, + PHYLINK_MAC_LINK_STATE, + PHYLINK_MAC_AN_RESTART, + PHYLINK_MAC_CONFIG, + PHYLINK_MAC_LINK_DOWN, + PHYLINK_MAC_LINK_UP, + PHYLINK_GET_FIXED_STATE, +}; + +struct phylink_notifier_info { + struct phylink_link_state *state; + unsigned long *supported; + u8 link_an_mode; + phy_interface_t interface; + struct phy_device *phydev; +}; + /** * struct phylink_mac_ops - MAC operations structure. * @validate: Validate and update the link configuration. @@ -200,6 +218,9 @@ void mac_link_up(struct net_device *ndev, unsigned int mode, struct phylink *phylink_create(struct net_device *, struct fwnode_handle *, phy_interface_t iface, const struct phylink_mac_ops *ops); +struct phylink *phylink_create_raw(struct notifier_block *nb, + struct fwnode_handle *fwnode, + phy_interface_t iface); void phylink_destroy(struct phylink *); int phylink_connect_phy(struct phylink *, struct phy_device *);