linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Saravana Kannan <saravanak@google.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	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>,
	"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: Thu, 9 Sep 2021 13:38:15 +0300	[thread overview]
Message-ID: <20210909103815.kcrygpfssvucbex4@skbuf> (raw)
In-Reply-To: <CAGETcx_U--ayNCo2GH1-EuzuD9usywjQm+B57X_YwFOjA3e+3Q@mail.gmail.com>

On Wed, Sep 08, 2021 at 08:21:05PM -0700, Saravana Kannan wrote:
> > But with that fixed, it
> > still does not work. This is too late, the mdio busses have already
> > been registered and probed, the PHYs have been found on the busses,
> > and the PHYs would of been probed, if not for fw_devlink.
> 
> Sigh... looks like some drivers register their mdio bus in their
> dsa_switch_ops->setup while others do it in their actual probe
> function (which actually makes more sense to me).

If it makes more sense to you for of_mdiobus_register to be placed in
the probe function and not in ->setup, then please be aware of my
previous email pointing out that DSA defers probe due to other reasons
too before calling ->setup, like of_find_net_device_by_node not finding
the DSA master. If the MDIO bus was registered by then, it will be
destroyed by the unwind path and the device links will not be created a
second time, effectively defeating fw_devlink.

> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index 53f034fc2ef7..7ecd910f7fb8 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -525,6 +525,9 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
> >             NULL == bus->read || NULL == bus->write)
> >                 return -EINVAL;
> >
> > +       if (bus->parent && bus->parent->of_node)
> > +               bus->parent->of_node->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT;
> > +
> >         BUG_ON(bus->state != MDIOBUS_ALLOCATED &&
> >                bus->state != MDIOBUS_UNREGISTERED);
> >
> > So basically saying all MDIO busses potentially have a problem.
> >
> > I also don't like the name FWNODE_FLAG_BROKEN_PARENT. The parents are
> > not broken, they work fine, if fw_devlink gets out of the way and
> > allows them to do their job.
> 
> The parent assuming the child will be probed as soon as it's added is
> a broken expectation/assumption. fw_devlink is just catching them
> immediately.

It's not really a broken expectation when you come to think of the fact
that synchronous probing is requested, and this is the internal PHY of
the switch we are talking about, not just any PHY off the street, with
random dependencies. It is known beforehand, and so are the dependencies.
All dependencies that the internal PHY has should be provided by the
switch driver by the time it registers the MDIO bus.

The system is not prepared to handle an -EPROBE_DEFER simply because
there is no good reason why it should happen.

I see fw_devlink as injecting a fault in this case. Maybe we should
treat it, but in any case it is adding a pointless -EPROBE_DEFER when
the PHY driver could have probed immediately. This will slow down the
boot time when we treat it properly eventually.

> Having said that, this is not the hill either of us should choose to
> die on. So, how about something like:
> FWNODE_FLAG_NEEDS_CHILD_BOUND_ON_ADD
> 
> If that works, I can clean up the series with this and the MDIO fix
> you mentioned.

I'm okay with the "needs child bound on add" name.

> > You also asked about why the component framework is not used. DSA has
> > been around for a while, the first commit dates back to October
> > 2008. Russell Kings first commit for the component framework is
> > January 2014. The plain driver model has worked for the last 13 years,
> > so there has not been any need to change.
> 
> Thanks for the history on why it couldn't have been used earlier.
> 
> In the long run, I'd still like to fix this so that the
> dsa_tree_setup() doesn't need the flag above. I have some ideas using
> device links that'll be much simpler to understand and maintain than
> using the component framework. I'll send out patches for that (not
> meant for 5.15) later and we can go with the MDIO bus hammer for 5.15.

Details?

I am not too fond of using the component framework because I am not
convinced we should be fabricating struct devices we do not need, just
to comply with an API that solves a fabricated problem.

  reply	other threads:[~2021-09-09 10:38 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
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 [this message]
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=20210909103815.kcrygpfssvucbex4@skbuf \
    --to=olteanv@gmail.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=rafael@kernel.org \
    --cc=saravanak@google.com \
    --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).