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: Wed, 8 Sep 2021 20:21:05 -0700	[thread overview]
Message-ID: <CAGETcx_U--ayNCo2GH1-EuzuD9usywjQm+B57X_YwFOjA3e+3Q@mail.gmail.com> (raw)
In-Reply-To: <YTll0i6Rz3WAAYzs@lunn.ch>

On Wed, Sep 8, 2021 at 6:39 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > --- 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;
>
> This does not work. First off, its dn, not np.

My bad. Copy paste error.

> 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).

>
> What did work was:
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index c45ca2473743..45d67d50e35f 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -6249,8 +6249,10 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>         if (!np && !pdata)
>                 return -EINVAL;
>
> -       if (np)
> +       if (np) {
>                 compat_info = of_device_get_match_data(dev);
> +               np->fwnode.flags |= FWNODE_FLAG_BROKEN_PARENT;
> +       }
>
>         if (pdata) {
>                 compat_info = pdata_device_get_match_data(dev);
>
> This will fix it for mv88e6xxx. But if the same problem occurs in any
> of the other DSA drivers, they will still be broken:
>
> ~/linux/drivers/net/dsa$ grep -r mdiobus_register *
> bcm_sf2.c:      err = mdiobus_register(priv->slave_mii_bus);
> dsa_loop_bdinfo.c:      return mdiobus_register_board_info(&bdinfo, 1);
> lantiq_gswip.c: return of_mdiobus_register(ds->slave_mii_bus, mdio_np);
> mt7530.c:       ret = mdiobus_register(bus);
> mv88e6xxx/chip.c:       err = of_mdiobus_register(bus, np);
> grep: mv88e6xxx/chip.o: binary file matches
> ocelot/seville_vsc9953.c:       rc = mdiobus_register(bus);
> ocelot/felix_vsc9959.c: rc = mdiobus_register(bus);
> qca/ar9331.c:   ret = of_mdiobus_register(mbus, mnp);
> qca8k.c:        return devm_of_mdiobus_register(priv->dev, bus, mdio);
> realtek-smi-core.c:     ret = of_mdiobus_register(smi->slave_mii_bus, mdio_np);

This one would have worked because it registers it in the ->setup()
ops. So it's not a simple grep for of_mdiobus_register(). But your
point stands nonetheless.

> sja1105/sja1105_mdio.c: rc = of_mdiobus_register(bus, np);
> sja1105/sja1105_mdio.c: rc = of_mdiobus_register(bus, np);
> sja1105/sja1105_mdio.c: rc = mdiobus_register(bus);
> sja1105/sja1105_mdio.c:int sja1105_mdiobus_register(struct dsa_switch *ds)
> sja1105/sja1105.h:int sja1105_mdiobus_register(struct dsa_switch *ds);
> sja1105/sja1105_main.c: rc = sja1105_mdiobus_register(ds);
>
> If you are happy to use a big hammer:

I'm okay with this big hammer for now while we figure out something better.

>
> 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.

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.

> 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.

-Saravana

  reply	other threads:[~2021-09-09  3:21 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 [this message]
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_U--ayNCo2GH1-EuzuD9usywjQm+B57X_YwFOjA3e+3Q@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).