linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Saravana Kannan <saravanak@google.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Len Brown <lenb@kernel.org>,
	Alvin Sipraga <ALSI@bang-olufsen.dk>,
	kernel-team@android.com, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH v1 1/2] driver core: fw_devlink: Add support for FWNODE_FLAG_BROKEN_PARENT
Date: Mon, 30 Aug 2021 12:03:15 -0700	[thread overview]
Message-ID: <CAGETcx_mjY10WzaOvb=vuojbodK7pvY1srvKmimu4h6xWkeQuQ@mail.gmail.com> (raw)
In-Reply-To: <YSpr/BOZj2PKoC8B@lunn.ch>

On Sat, Aug 28, 2021 at 10:01 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Aug 27, 2021 at 02:33:02PM -0700, Saravana Kannan wrote:
> > On Fri, Aug 27, 2021 at 1:11 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > > > I've not yet looked at plain Ethernet drivers. This pattern could also
> > > > > exist there. And i wonder about other complex structures, i2c bus
> > > > > multiplexors, you can have interrupt controllers as i2c devices,
> > > > > etc. So the general case could exist in other places.
> > > >
> > > > I haven't seen any generic issues like this reported so far. It's only
> > > > after adding phy-handle that we are hitting these issues with DSA
> > > > switches.
> > >
> > > Can you run your parser over the 2250 DTB blobs and see how many
> > > children have dependencies on a parent? That could give us an idea how
> > > many moles need whacking. And maybe, where in the tree they are
> > > hiding?
> >
> > You are only responding to part of my email. As I said in my previous
> > email: "There are plenty of cases where it's better to delay the child
> > device's probe until the parent finishes. You even gave an example[7]
> > where it would help avoid unnecessary deferred probes." Can you please
> > give your thoughts on the rest of the points I made too?
>
> I must admit, my main problem at the moment is -rc1 in two weeks
> time. It seems like a number of board with Ethernet switches will be
> broken, that worked before. phy-handle is not limited to switch
> drivers, it is also used for Ethernet drivers. So it could be, a
> number of Ethernet drivers are also going to be broken in -rc1?

Again, in those cases, based on your FEC example, fw_devlink=on
actually improves things.

> But the issues sounds not to be specific to phy-handle, but any
> phandle that points back to a parent.

I feel like I'm going in circles here. This statement is not true.
Please read my previous explanations.

> So it could be drivers outside
> of networking are also going to be broken with -rc1?
> You have been very focused on one or two drivers. I would much rather
> see you getting an idea of how wide spread this problem is, and what
> should we do for -rc1?

Again, it's not a widespread problem as I explained before.
fw_devlink=on has been the default for 2 kernel versions now. With no
unfixed reported issues.

> Even if modifying DSA drivers to component drivers is possible, while
> not breaking backwards compatibility with DT,

It should be possible without needing any DT changes.

> it is not going to
> happen over night. That is something for the next merge window, not
> this merge window.

Right, I wasn't suggesting the component driver option be implemented
right away. We were talking about what the longer term proper fix
would be for DSA (and Ethernet if we actually find issues there) and
who would do it. That's what I hope this discussion could be.

Also, if we replace Patch 2/2 in this series with the patch below, it
will work as a generic quick fix for DSA that we could use for -rc1.
And if we still have issues reported on the phy-handle patch by -rc5
or so, we could revert the phy-handle patch then so that v5.15 isn't
broken.

-Saravana

--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1286,6 +1286,17 @@ static int dsa_switch_parse_of(struct
dsa_switch *ds, struct device_node *dn)
 {
        int err;

+       /* A lot of switch devices have their PHYs as child devices and have
+        * the PHYs depend on the switch as a supplier (Eg: interrupt
+        * controller). With fw_devlink=on, that means the PHYs will defer
+        * probe until the probe() of the switch completes. However, the way
+        * the DSA framework is designed, the PHYs are expected to be probed
+        * successfully before the probe() of the switch completes.
+        *
+        * So, mark the switch devices as a "broken parent" so that fw_devlink
+        * knows not to create device links between PHYs and the parent switch.
+        */
+       np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT;
        err = dsa_switch_parse_member_of(ds, dn);
        if (err)
                return err;

  reply	other threads:[~2021-08-30 19:03 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
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 [this message]
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_mjY10WzaOvb=vuojbodK7pvY1srvKmimu4h6xWkeQuQ@mail.gmail.com' \
    --to=saravanak@google.com \
    --cc=ALSI@bang-olufsen.dk \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-team@android.com \
    --cc=kuba@kernel.org \
    --cc=lenb@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=olteanv@gmail.com \
    --cc=rafael@kernel.org \
    --cc=vivien.didelot@gmail.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).