From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Vladimir Oltean <olteanv@gmail.com> Cc: "Andrew Lunn" <andrew@lunn.ch>, "Heiner Kallweit" <hkallweit1@gmail.com>, "Alexandre Belloni" <alexandre.belloni@bootlin.com>, "Alvin __ipraga" <alsi@bang-olufsen.dk>, "Claudiu Manoil" <claudiu.manoil@nxp.com>, "David S. Miller" <davem@davemloft.net>, "DENG Qingfang" <dqfext@gmail.com>, "Eric Dumazet" <edumazet@google.com>, "Florian Fainelli" <f.fainelli@gmail.com>, "George McCollister" <george.mccollister@gmail.com>, "Hauke Mehrtens" <hauke@hauke-m.de>, "Jakub Kicinski" <kuba@kernel.org>, "Kurt Kanzenbach" <kurt@linutronix.de>, "Landen Chao" <Landen.Chao@mediatek.com>, "Linus Walleij" <linus.walleij@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, "Matthias Brugger" <matthias.bgg@gmail.com>, netdev@vger.kernel.org, "Paolo Abeni" <pabeni@redhat.com>, "Sean Wang" <sean.wang@mediatek.com>, UNGLinuxDriver@microchip.com, "Vivien Didelot" <vivien.didelot@gmail.com>, "Woojung Huh" <woojung.huh@microchip.com>, "Marek Behún" <kabel@kernel.org> Subject: Re: [PATCH RFC net-next 5/5] net: dsa: always use phylink for CPU and DSA ports Date: Thu, 7 Jul 2022 16:48:12 +0100 [thread overview] Message-ID: <YscAPP7mF3KEE1/p@shell.armlinux.org.uk> (raw) In-Reply-To: <20220707152727.foxrd4gvqg3zb6il@skbuf> On Thu, Jul 07, 2022 at 06:27:27PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 11:09:43AM +0100, Russell King (Oracle) wrote: > > On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote: > > > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > > > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > > > > where a fixed-link property is also missing, not just a phy-handle? > > > > Although to be entirely correct, we can also have MLO_AN_INBAND, which > > > > wouldn't be covered by these 2 checks and would still represent a valid > > > > DT binding. > > > > > > phylink_set_max_fixed_link() already excludes itself: > > > > > > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) > ~~~~~~~~~~ > > If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from > phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> > phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() > that were initiated by the phylink user between phylink_create() and > phylink_set_max_fixed_link(), correct? Those are specified as invalid in the > kerneldoc and that's about it - that's not what the checking is for, correct? No, it's not to do with sfps at all, but to do with enforcing the pre-conditions for the function - that entire line is checking that (a) we are in a sane state to be called, and (b) there is no configuration initialisation beyond the default done by phylink_create() - in other words, there is no in-band or fixed-link specified. Let's go through this step by step. 1. pl->cfg_link_an_mode != MLO_AN_PHY The default value for cfg_link_an_mode is MLO_AN_PHY. If it's anything other than that, then a fixed-link or in-band mode has been specified, and we don't want to override that. So this call needs to fail. 2. pl->phydev If a PHY has been attached, then the pre-condition for calling this function immediately after phylink_create() has been violated, because the only way it can be non-NULL is if someone's called one of the phylink functions that connects a PHY. Note: SFPs will not set their PHY here, because, for them to discover that there's a PHY, the network interface needs to be up, and it will never be up here... but in any case... 3. pl->sfp_bus If we have a SFP bus, then we definitely are not going to be operating in this default fixed-link mode - because the SFP is going to want to set its own parameters. > > > return -EBUSY; > > > > > > intentionally so that if there is anything specified for the port, be > > > that a fixed link or in-band, then phylink_set_max_fixed_link() errors > > > out with -EBUSY. > > > > > > The only case that it can't detect is if there is a PHY that may be > > > added to phylink at a later time, and that is what the check above > > > is for. > > Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy() > after phylink_set_max_fixed_link(), right? Correct. > So this is what I don't understand. If we've called phylink_set_max_fixed_link() > we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will > silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA > predicts if it's going to call either of those connect_phy() functions, > and calls phylink_set_max_fixed_link() only if it won't. Right? > > You've structured the checks in this "distributed" way because phylink > can't really predict whether phylink_{,fwnode_}connect_phy() will be > called after phylink_set_max_fixed_link(), right? I mean, it can > probably predict the fwnode_ variant, but not phylink_connect_phy, and > this is why it is up to the caller to decide when to call and when not to. phylink has no idea whether phylink_fwnode_connect_phy() will be called with the same fwnode as phylink_create(), so it really can't make any assumptions about whether there will be a PHY or not. > > > I've updated the function description to mention this detail: > > > > +/** > > + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink > > + * @pl: a pointer to a &struct phylink returned from phylink_create() > > + * > > + * Set a maximum speed fixed-link configuration for the chosen interface mode > > + * and MAC capabilities for the phylink instance if the instance has not > > + * already been configured with a SFP, fixed link, or in-band AN mode. If the > > + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported > > + * interfaces bitmap for the first interface that gives the fastest supported > > + * speed. > > > > Does this address your concern? > > > > Thanks. > > Not really, no, sorry, it just confuses me more. I find that happens a lot when I try to add more documentation to clarify things. I sometimes get to the point of deciding its better _not_ to write any documentation, because documentation just ends up being confusing and people want more and more detail. I've got to the point in some case where I've had to spell out which keys to press on the keyboard for people to formulate the "[PATCH ...]" thing correctly, because if you put it in quotes, you get people who will include the quotes even if you tell them not to. I hate documentation, I seem incapable of writing it in a way people can understand. > It should maybe also > say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() > is going to be called later. It's already a precondition that phylink_{,fwnode_}connect_phy() fail if we're in fixed-link mode (because PHYs have never been supported when in fixed-link mode - if one remembers, the old fixed-link code used to provide its own emulation of a PHY to make fixed-links work.) So PHYs and fixed-links have always been mutually exclusive before phylink, and continue to be so with phylink. > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > based on the following? > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > condition from phylink_fwnode_phy_connect(): > > phy_fwnode = fwnode_get_phy_node(fwnode); > if (IS_ERR(phy_fwnode)) { > if (pl->cfg_link_an_mode == MLO_AN_PHY) > return -ENODEV; <- here > return 0; > } My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
WARNING: multiple messages have this Message-ID (diff)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> To: Vladimir Oltean <olteanv@gmail.com> Cc: "Andrew Lunn" <andrew@lunn.ch>, "Heiner Kallweit" <hkallweit1@gmail.com>, "Alexandre Belloni" <alexandre.belloni@bootlin.com>, "Alvin __ipraga" <alsi@bang-olufsen.dk>, "Claudiu Manoil" <claudiu.manoil@nxp.com>, "David S. Miller" <davem@davemloft.net>, "DENG Qingfang" <dqfext@gmail.com>, "Eric Dumazet" <edumazet@google.com>, "Florian Fainelli" <f.fainelli@gmail.com>, "George McCollister" <george.mccollister@gmail.com>, "Hauke Mehrtens" <hauke@hauke-m.de>, "Jakub Kicinski" <kuba@kernel.org>, "Kurt Kanzenbach" <kurt@linutronix.de>, "Landen Chao" <Landen.Chao@mediatek.com>, "Linus Walleij" <linus.walleij@linaro.org>, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, "Matthias Brugger" <matthias.bgg@gmail.com>, netdev@vger.kernel.org, "Paolo Abeni" <pabeni@redhat.com>, "Sean Wang" <sean.wang@mediatek.com>, UNGLinuxDriver@microchip.com, "Vivien Didelot" <vivien.didelot@gmail.com>, "Woojung Huh" <woojung.huh@microchip.com>, "Marek Behún" <kabel@kernel.org> Subject: Re: [PATCH RFC net-next 5/5] net: dsa: always use phylink for CPU and DSA ports Date: Thu, 7 Jul 2022 16:48:12 +0100 [thread overview] Message-ID: <YscAPP7mF3KEE1/p@shell.armlinux.org.uk> (raw) In-Reply-To: <20220707152727.foxrd4gvqg3zb6il@skbuf> On Thu, Jul 07, 2022 at 06:27:27PM +0300, Vladimir Oltean wrote: > On Thu, Jul 07, 2022 at 11:09:43AM +0100, Russell King (Oracle) wrote: > > On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote: > > > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote: > > > > Can we please limit phylink_set_max_link_speed() to just the CPU ports > > > > where a fixed-link property is also missing, not just a phy-handle? > > > > Although to be entirely correct, we can also have MLO_AN_INBAND, which > > > > wouldn't be covered by these 2 checks and would still represent a valid > > > > DT binding. > > > > > > phylink_set_max_fixed_link() already excludes itself: > > > > > > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus) > ~~~~~~~~~~ > > If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from > phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() -> > phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy() > that were initiated by the phylink user between phylink_create() and > phylink_set_max_fixed_link(), correct? Those are specified as invalid in the > kerneldoc and that's about it - that's not what the checking is for, correct? No, it's not to do with sfps at all, but to do with enforcing the pre-conditions for the function - that entire line is checking that (a) we are in a sane state to be called, and (b) there is no configuration initialisation beyond the default done by phylink_create() - in other words, there is no in-band or fixed-link specified. Let's go through this step by step. 1. pl->cfg_link_an_mode != MLO_AN_PHY The default value for cfg_link_an_mode is MLO_AN_PHY. If it's anything other than that, then a fixed-link or in-band mode has been specified, and we don't want to override that. So this call needs to fail. 2. pl->phydev If a PHY has been attached, then the pre-condition for calling this function immediately after phylink_create() has been violated, because the only way it can be non-NULL is if someone's called one of the phylink functions that connects a PHY. Note: SFPs will not set their PHY here, because, for them to discover that there's a PHY, the network interface needs to be up, and it will never be up here... but in any case... 3. pl->sfp_bus If we have a SFP bus, then we definitely are not going to be operating in this default fixed-link mode - because the SFP is going to want to set its own parameters. > > > return -EBUSY; > > > > > > intentionally so that if there is anything specified for the port, be > > > that a fixed link or in-band, then phylink_set_max_fixed_link() errors > > > out with -EBUSY. > > > > > > The only case that it can't detect is if there is a PHY that may be > > > added to phylink at a later time, and that is what the check above > > > is for. > > Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy() > after phylink_set_max_fixed_link(), right? Correct. > So this is what I don't understand. If we've called phylink_set_max_fixed_link() > we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will > silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA > predicts if it's going to call either of those connect_phy() functions, > and calls phylink_set_max_fixed_link() only if it won't. Right? > > You've structured the checks in this "distributed" way because phylink > can't really predict whether phylink_{,fwnode_}connect_phy() will be > called after phylink_set_max_fixed_link(), right? I mean, it can > probably predict the fwnode_ variant, but not phylink_connect_phy, and > this is why it is up to the caller to decide when to call and when not to. phylink has no idea whether phylink_fwnode_connect_phy() will be called with the same fwnode as phylink_create(), so it really can't make any assumptions about whether there will be a PHY or not. > > > I've updated the function description to mention this detail: > > > > +/** > > + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink > > + * @pl: a pointer to a &struct phylink returned from phylink_create() > > + * > > + * Set a maximum speed fixed-link configuration for the chosen interface mode > > + * and MAC capabilities for the phylink instance if the instance has not > > + * already been configured with a SFP, fixed link, or in-band AN mode. If the > > + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported > > + * interfaces bitmap for the first interface that gives the fastest supported > > + * speed. > > > > Does this address your concern? > > > > Thanks. > > Not really, no, sorry, it just confuses me more. I find that happens a lot when I try to add more documentation to clarify things. I sometimes get to the point of deciding its better _not_ to write any documentation, because documentation just ends up being confusing and people want more and more detail. I've got to the point in some case where I've had to spell out which keys to press on the keyboard for people to formulate the "[PATCH ...]" thing correctly, because if you put it in quotes, you get people who will include the quotes even if you tell them not to. I hate documentation, I seem incapable of writing it in a way people can understand. > It should maybe also > say that this function shouldn't be called if phylink_{,fwnode_}connect_phy() > is going to be called later. It's already a precondition that phylink_{,fwnode_}connect_phy() fail if we're in fixed-link mode (because PHYs have never been supported when in fixed-link mode - if one remembers, the old fixed-link code used to provide its own emulation of a PHY to make fixed-links work.) So PHYs and fixed-links have always been mutually exclusive before phylink, and continue to be so with phylink. > Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link() > based on the following? > > (1) struct phylink_config gets extended with a bool fallback_max_fixed_link. > (2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register(). > (3) phylink_set_max_fixed_link() is hooked into this -ENODEV error > condition from phylink_fwnode_phy_connect(): > > phy_fwnode = fwnode_get_phy_node(fwnode); > if (IS_ERR(phy_fwnode)) { > if (pl->cfg_link_an_mode == MLO_AN_PHY) > return -ENODEV; <- here > return 0; > } My question in response would be - why should this DSA specific behaviour be handled completely internally within phylink, when it's a DSA specific behaviour? Why do we need boolean flags for this? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-07-07 15:48 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-05 9:46 [PATCH RFC net-next v2 0/5] net: dsa: always use phylink Russell King (Oracle) 2022-07-05 9:46 ` Russell King (Oracle) 2022-07-05 9:47 ` [PATCH RFC net-next 1/5] net: dsa: add support for retrieving the interface mode Russell King (Oracle) 2022-07-05 9:47 ` Russell King (Oracle) 2022-07-05 9:47 ` [PATCH RFC net-next 2/5] net: dsa: mv88e6xxx: report the default interface mode for the port Russell King (Oracle) 2022-07-05 9:47 ` Russell King (Oracle) 2022-07-05 10:55 ` Marek Behún 2022-07-05 10:55 ` Marek Behún 2022-07-06 11:04 ` kernel test robot 2022-07-05 9:47 ` [PATCH RFC net-next 3/5] net: phylink: split out interface to caps translation Russell King (Oracle) 2022-07-05 9:47 ` Russell King (Oracle) 2022-07-05 9:48 ` [PATCH RFC net-next 4/5] net: phylink: add phylink_set_max_fixed_link() Russell King (Oracle) 2022-07-05 9:48 ` Russell King (Oracle) 2022-07-05 10:58 ` Marek Behún 2022-07-05 10:58 ` Marek Behún 2022-07-05 9:48 ` [PATCH RFC net-next 5/5] net: dsa: always use phylink for CPU and DSA ports Russell King (Oracle) 2022-07-05 9:48 ` Russell King (Oracle) 2022-07-06 10:26 ` Vladimir Oltean 2022-07-06 10:26 ` Vladimir Oltean 2022-07-06 16:24 ` Russell King (Oracle) 2022-07-06 16:24 ` Russell King (Oracle) 2022-07-07 10:09 ` Russell King (Oracle) 2022-07-07 10:09 ` Russell King (Oracle) 2022-07-07 15:27 ` Vladimir Oltean 2022-07-07 15:27 ` Vladimir Oltean 2022-07-07 15:48 ` Russell King (Oracle) [this message] 2022-07-07 15:48 ` Russell King (Oracle) 2022-07-07 16:38 ` Vladimir Oltean 2022-07-07 16:38 ` Vladimir Oltean 2022-07-07 17:15 ` Russell King (Oracle) 2022-07-07 17:15 ` Russell King (Oracle) 2022-07-07 19:37 ` Vladimir Oltean 2022-07-07 19:37 ` Vladimir Oltean 2022-07-07 20:23 ` Russell King (Oracle) 2022-07-07 20:23 ` Russell King (Oracle) 2022-07-07 21:48 ` Vladimir Oltean 2022-07-07 21:48 ` Vladimir Oltean 2022-07-08 15:25 ` Russell King (Oracle) 2022-07-08 15:25 ` Russell King (Oracle) 2022-07-08 15:40 ` Marek Behún 2022-07-08 15:40 ` Marek Behún 2022-07-07 11:00 ` Russell King (Oracle) 2022-07-07 11:00 ` Russell King (Oracle) 2022-07-07 15:43 ` Vladimir Oltean 2022-07-07 15:43 ` Vladimir Oltean 2022-07-07 16:32 ` Russell King (Oracle) 2022-07-07 16:32 ` Russell King (Oracle) 2022-07-07 16:50 ` Vladimir Oltean 2022-07-07 16:50 ` Vladimir Oltean 2022-07-05 16:42 ` [PATCH RFC net-next v2 0/5] net: dsa: always use phylink Florian Fainelli 2022-07-05 16:42 ` Florian Fainelli 2022-07-06 10:14 ` Vladimir Oltean 2022-07-06 10:14 ` Vladimir Oltean 2022-07-06 16:27 ` Florian Fainelli 2022-07-06 16:27 ` Florian Fainelli 2022-07-06 19:05 ` Hauke Mehrtens 2022-07-06 19:05 ` Hauke Mehrtens 2022-07-06 20:24 ` Russell King (Oracle) 2022-07-06 20:24 ` Russell King (Oracle) 2022-07-06 17:22 ` Kurt Kanzenbach 2022-07-06 17:22 ` Kurt Kanzenbach 2022-07-06 22:46 ` Linus Walleij 2022-07-06 22:46 ` Linus Walleij 2022-07-07 13:46 ` Linus Walleij 2022-07-07 13:46 ` Linus Walleij
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YscAPP7mF3KEE1/p@shell.armlinux.org.uk \ --to=linux@armlinux.org.uk \ --cc=Landen.Chao@mediatek.com \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=alsi@bang-olufsen.dk \ --cc=andrew@lunn.ch \ --cc=claudiu.manoil@nxp.com \ --cc=davem@davemloft.net \ --cc=dqfext@gmail.com \ --cc=edumazet@google.com \ --cc=f.fainelli@gmail.com \ --cc=george.mccollister@gmail.com \ --cc=hauke@hauke-m.de \ --cc=hkallweit1@gmail.com \ --cc=kabel@kernel.org \ --cc=kuba@kernel.org \ --cc=kurt@linutronix.de \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=matthias.bgg@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=olteanv@gmail.com \ --cc=pabeni@redhat.com \ --cc=sean.wang@mediatek.com \ --cc=vivien.didelot@gmail.com \ --cc=woojung.huh@microchip.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.