From: Saravana Kannan <firstname.lastname@example.org> To: Andrew Lunn <email@example.com> Cc: Greg Kroah-Hartman <firstname.lastname@example.org>, "Rafael J. Wysocki" <email@example.com>, Linus Walleij <firstname.lastname@example.org>, Vivien Didelot <email@example.com>, Florian Fainelli <firstname.lastname@example.org>, Vladimir Oltean <email@example.com>, "David S. Miller" <firstname.lastname@example.org>, Jakub Kicinski <email@example.com>, Len Brown <firstname.lastname@example.org>, Alvin Sipraga <ALSI@bang-olufsen.dk>, email@example.com, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org Subject: Re: [PATCH v1 1/2] driver core: fw_devlink: Add support for FWNODE_FLAG_BROKEN_PARENT Date: Thu, 26 Aug 2021 21:02:16 -0700 [thread overview] Message-ID: <CAGETcx_r8LSxV5=GQ-1qPjh7qGbCqTsSoSkQfxAKL5q+znRoWg@mail.gmail.com> (raw) In-Reply-To: <YSg+dRPSX9email@example.com> On Thu, Aug 26, 2021 at 6:23 PM Andrew Lunn <firstname.lastname@example.org> wrote: > > > Doesn't add much to the discussion. In the example I gave, the driver > > already does synchronous probing. If the device can't probe > > successfully because a supplier isn't ready, it doesn't matter if it's > > a synchronous probe. The probe would still be deferred and we'll hit > > the same issue. Even in the situation the commit  describes, if > > parallelized probing is done and the PHY depended on something (say a > > clock), you'd still end up not probing the PHY even if the driver is > > present and the generic PHY would end up force probing it. > > > genphy is meant to be used when there is no other driver available. > It is a best effort, better than nothing, might work. And quite a few > boards rely on it. However, it should not be used when there is a > specific driver. Agreed, that's what we are trying to ensure. > So if the PHY device has been probed, and -EPROBE_DEFER was returned, > we also need to return -EPROBE_DEFER here when deciding if genphy > should be used. It should then all unwind and try again later. Yes, I think dsa_register_switch() returning -EPROBE_DEFER if the PHYs returned -EPROBE_DEFER might be okay (I think we should do it), but that doesn't solve the problem for this driver. fw_devlink=on/device links short circuits the probe() call of a consumer (in this case the PHY) and returns -EPROBE_DEFER if the supplier's (in this case switch) probe hasn't finished without an error. fw_devlink/device links effectively does the probe in graph topological order and there's a ton of good reasons to do it that way -- what's why fw_devlink=on was implemented. In this specific case though, since the PHY depends on the parent device, if we fail the parent's probe realtek_smi_probe() because the PHYs failed to probe, we'll get into a catch-22/chicken-n-egg situation and the switch/PHYs will never probe. I think a clean way to fix this at the driver level is to do what I said in . Copy pasting it here and expanding it a bit: 1. The IRQ registration and mdio bus registration should get moved to realtek_smi_probe() which probes "realtek,rtl8366rb". So realtek_smi_probe() succeeding doesn't depend on its child devices probing successfully (which makes sense for any parent device). 2. realtek_smi_probe() should also create/register a component-master/aggregate device that's "made up of" realtek,rtl8366rb and all the PHYs. So the component-master will wait for all of them to finish probing before it's initialized. 3. PHYs will probe successfully now because realtek,rtl8366rb probe() which is the supplier's probe has finished probing without problems. 4. The component device's init (the .bind op) would call dsa_register_switch() which kinda makes sense because the rtl8366rb and all the PHYs combined together is what makes up the logical DSA switch. The dsa_register_switch() will succeed and will be using the right/specific PHY driver. The same applies for any switch that has the PHYs as it's child device AND (this is the key part) the PHYs depend on the switch as a supplier (remember, if we didn't have the interrupt dependency, this would not be an issue). > I don't know the device core, but it looks like dev->can_match tells > us what we need to know. If true, we know there is a driver for this > device. But i'm hesitant to make use of this outside of driver/base. can_match is never cleared once it's set and it's meant as an optimization/preserving some probe order stuff. I wouldn't depend on it for this case. We can just have a phy_has_driver() function that searches all the currently registered PHY drivers to check if there's a matching driver. And dsa_register_switch() or phy_attach_direct() can return -EPROBE_DEFER if there is a driver but it isn't bound yet. Again, this is orthogonal to the realtek driver fix though because of the catch-22 situation above. -Saravana  - https://lore.kernel.org/netdev/CAGETcx8_vxxPxF8WrXqk=PZYfEggsozP+z9KyOu5C2bEW0VW8g@mail.gmail.com/
next prev parent reply other threads:[~2021-08-27 4:02 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-26 7:45 [PATCH v1 0/2] Fix rtl8366rb issues with fw_devlink=on Saravana Kannan 2021-08-26 7:45 ` [PATCH v1 1/2] driver core: fw_devlink: Add support for FWNODE_FLAG_BROKEN_PARENT Saravana Kannan 2021-08-26 13:13 ` Andrew Lunn 2021-08-26 19:56 ` Saravana Kannan 2021-08-26 20:53 ` Andrew Lunn 2021-08-26 23:44 ` Saravana Kannan 2021-08-27 1:23 ` Andrew Lunn 2021-08-27 4:02 ` Saravana Kannan [this message] 2021-08-27 13:44 ` Andrew Lunn 2021-08-27 18:06 ` Saravana Kannan 2021-08-27 20:11 ` Andrew Lunn 2021-08-27 21:33 ` Saravana Kannan 2021-08-28 17:01 ` Andrew Lunn 2021-08-30 19:03 ` Saravana Kannan 2021-08-31 13:16 ` Andrew Lunn 2021-08-31 19:26 ` Saravana Kannan 2021-08-31 22:05 ` Andrew Lunn 2021-08-31 22:18 ` Saravana Kannan 2021-08-31 23:02 ` Andrew Lunn 2021-08-31 23:18 ` Vladimir Oltean 2021-08-31 23:27 ` Andrew Lunn 2021-09-01 1:28 ` Vladimir Oltean 2021-09-01 1:38 ` Vladimir Oltean 2021-09-01 2:19 ` Saravana Kannan 2021-09-01 9:02 ` Vladimir Oltean 2021-09-01 22:57 ` Saravana Kannan 2021-09-01 2:00 ` Saravana Kannan 2021-09-01 8:46 ` Vladimir Oltean 2021-09-01 22:53 ` Saravana Kannan 2021-09-02 17:41 ` Andrew Lunn 2021-09-02 17:58 ` Saravana Kannan 2021-09-30 5:33 ` Saravana Kannan 2021-09-30 13:15 ` Andrew Lunn 2021-09-30 13:43 ` Vladimir Oltean 2021-09-30 14:00 ` Andrew Lunn 2021-09-30 17:31 ` Saravana Kannan 2021-09-30 19:38 ` Andrew Lunn 2021-09-30 19:48 ` Saravana Kannan 2021-09-30 20:06 ` Florian Fainelli 2021-09-30 20:14 ` Saravana Kannan 2021-09-30 21:16 ` Florian Fainelli 2021-09-30 20:22 ` Andrew Lunn 2021-09-01 1:46 ` Saravana Kannan 2021-08-31 23:18 ` Andrew Lunn 2021-09-08 18:35 ` Saravana Kannan 2021-09-09 1:39 ` Andrew Lunn 2021-09-09 3:21 ` Saravana Kannan 2021-09-09 10:38 ` Vladimir Oltean 2021-09-09 15:00 ` Andrew Lunn 2021-09-09 16:56 ` Florian Fainelli 2021-08-26 7:45 ` [PATCH v1 2/2] net: dsa: rtl8366rb: Quick fix to work with fw_devlink=on Saravana Kannan 2021-08-26 7:55 ` Greg Kroah-Hartman 2021-08-26 11:25 ` Florian Fainelli 2021-08-26 17:29 ` Saravana Kannan 2021-08-26 11:29 ` Alvin Šipraga 2021-08-26 17:26 ` Saravana Kannan 2021-08-26 18:04 ` Alvin Šipraga
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='CAGETcx_r8LSxV5=GQ-1qPjh7qGbCqTsSoSkQfxAKL5q+znRoWg@mail.gmail.com' \ --email@example.com \ --cc=ALSI@bang-olufsen.dk \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH v1 1/2] driver core: fw_devlink: Add support for FWNODE_FLAG_BROKEN_PARENT' \ /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: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).