[PATCH/RFC] treewide: Fix instantiation of devices in DT overlay

[PATCH/RFC] treewide: Fix instantiation of devices in DT overlay

[Geert Uytterhoeven]

When loading a DT overlay that creates a device, the device is not
instantiated, unless the DT overlay is unloaded and reloaded again.

Saravana explains:
  Basically for all overlays (I hope the function is only used for
  overlays) we assume all nodes are NOT devices until they actually
  get added as a device. Don't review the code, it's not meant to be :)

Based on a hacky patch by Saravana Kannan, which covered only platform
and spi devices.

Fixes: 4a032827daa89350 ("of: property: Simplify of_link_to_phandle()")
Link: https://lore.kernel.org/all/CAGETcx_+rhHvaC_HJXGrr5_WAd2+k5f=rWYnkCZ6z5bGX-wj4w@mail.gmail.com
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Marked RFC as Saravana said this is an ugly hack.
Still, this is a regression in v6.3-rc1 that should be fixed.
---
 drivers/bus/imx-weim.c    | 1 +
 drivers/i2c/i2c-core-of.c | 1 +
 drivers/of/dynamic.c      | 1 +
 drivers/of/platform.c     | 1 +
 drivers/spi/spi.c         | 1 +
 5 files changed, 5 insertions(+)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 2a6b4f676458612e..71d8807170fa9f29 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -329,6 +329,7 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
 				 "Failed to setup timing for '%pOF'\n", rd->dn);
 
 		if (!of_node_check_flag(rd->dn, OF_POPULATED)) {
+			rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 			if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
 				dev_err(&pdev->dev,
 					"Failed to create child device '%pOF'\n",
diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index bce6b796e04c2ca0..79a0d47010ba0b20 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -178,6 +178,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
 			return NOTIFY_OK;
 		}
 
+		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		client = of_i2c_register_device(adap, rd->dn);
 		if (IS_ERR(client)) {
 			dev_err(&adap->dev, "failed to create client for '%pOF'\n",
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 07d93753b12f5f4d..e311d406b1705306 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -226,6 +226,7 @@ static void __of_attach_node(struct device_node *np)
 	np->sibling = np->parent->child;
 	np->parent->child = np;
 	of_node_clear_flag(np, OF_DETACHED);
+	np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
 }
 
 /**
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b2bd2e783445dd78..17c92cbfb62ee3ef 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -737,6 +737,7 @@ static int of_platform_notify(struct notifier_block *nb,
 		if (of_node_check_flag(rd->dn, OF_POPULATED))
 			return NOTIFY_OK;
 
+		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		/* pdev_parent may be NULL when no bus platform device */
 		pdev_parent = of_find_device_by_node(rd->dn->parent);
 		pdev = of_platform_device_create(rd->dn, NULL,
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 1a65f96fe2aff591..7bd053a32fad1a3c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -4480,6 +4480,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
 			return NOTIFY_OK;
 		}
 
+		rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
 		spi = of_register_spi_device(ctlr, rd->dn);
 		put_device(&ctlr->dev);
 
-- 
2.34.1

Re: [PATCH/RFC] treewide: Fix instantiation of devices in DT overlay

[Saravana Kannan]

On Fri, Mar 17, 2023 at 3:33 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
>
> When loading a DT overlay that creates a device, the device is not
> instantiated, unless the DT overlay is unloaded and reloaded again.
>
> Saravana explains:
>   Basically for all overlays (I hope the function is only used for
>   overlays) we assume all nodes are NOT devices until they actually
>   get added as a device. Don't review the code, it's not meant to be :)
>
> Based on a hacky patch by Saravana Kannan, which covered only platform
> and spi devices.
>
> Fixes: 4a032827daa89350 ("of: property: Simplify of_link_to_phandle()")
> Link: https://lore.kernel.org/all/CAGETcx_+rhHvaC_HJXGrr5_WAd2+k5f=rWYnkCZ6z5bGX-wj4w@mail.gmail.com
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Marked RFC as Saravana said this is an ugly hack.
> Still, this is a regression in v6.3-rc1 that should be fixed.

Thanks for making sure this isn't forgotten.

I thought about this a bit more and I've decided what I gave earlier
isn't really too much of a hack. The other option is to handle the
clearing of the flag at the driver core level, but we incur these
additional instructions for all devices instead of just the overlay
case. But the benefit is that if more busses add overlay support in
the future, they won't need to remember to clear the flag in those
instances too. But they'll probably start off by looking at the
existing platform bus case, so they'll get it right.

I'll continue the pondering next week and maybe test it on my device
to make sure it's not doing anything weird for non-overlay cases.

-Saravana

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3611,6 +3611,15 @@ int device_add(struct device *dev)
         */
        if (dev->fwnode && !dev->fwnode->dev) {
                dev->fwnode->dev = dev;
+               /*
+                * If a fwnode was initially marked as not a device, but we
+                * clearly have a device added for it that can probe, then clear
+                * the flag so fw_devlink will continue linking consumers to
+                * this device. This code path is really expected to run only
+                * for DT overlays.
+                */
+               if (dev->bus)
+                       dev->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE
                fw_devlink_link_device(dev);
        }

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 07d93753b12f..f715b59d9bf3 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -226,6 +226,11 @@ static void __of_attach_node(struct device_node *np)
        np->sibling = np->parent->child;
        np->parent->child = np;
        of_node_clear_flag(np, OF_DETACHED);
+       /*
+        * Ask fw_devlink to assume any new node is not a device. Driver core
+        * will clear this flag if the assumption turns out to be wrong.
+        */
+       np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
 }




> ---
>  drivers/bus/imx-weim.c    | 1 +
>  drivers/i2c/i2c-core-of.c | 1 +
>  drivers/of/dynamic.c      | 1 +
>  drivers/of/platform.c     | 1 +
>  drivers/spi/spi.c         | 1 +
>  5 files changed, 5 insertions(+)
>
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index 2a6b4f676458612e..71d8807170fa9f29 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -329,6 +329,7 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
>                                  "Failed to setup timing for '%pOF'\n", rd->dn);
>
>                 if (!of_node_check_flag(rd->dn, OF_POPULATED)) {
> +                       rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
>                         if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
>                                 dev_err(&pdev->dev,
>                                         "Failed to create child device '%pOF'\n",
> diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> index bce6b796e04c2ca0..79a0d47010ba0b20 100644
> --- a/drivers/i2c/i2c-core-of.c
> +++ b/drivers/i2c/i2c-core-of.c
> @@ -178,6 +178,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
>                         return NOTIFY_OK;
>                 }
>
> +               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
>                 client = of_i2c_register_device(adap, rd->dn);
>                 if (IS_ERR(client)) {
>                         dev_err(&adap->dev, "failed to create client for '%pOF'\n",
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 07d93753b12f5f4d..e311d406b1705306 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -226,6 +226,7 @@ static void __of_attach_node(struct device_node *np)
>         np->sibling = np->parent->child;
>         np->parent->child = np;
>         of_node_clear_flag(np, OF_DETACHED);
> +       np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
>  }
>
>  /**
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b2bd2e783445dd78..17c92cbfb62ee3ef 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -737,6 +737,7 @@ static int of_platform_notify(struct notifier_block *nb,
>                 if (of_node_check_flag(rd->dn, OF_POPULATED))
>                         return NOTIFY_OK;
>
> +               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
>                 /* pdev_parent may be NULL when no bus platform device */
>                 pdev_parent = of_find_device_by_node(rd->dn->parent);
>                 pdev = of_platform_device_create(rd->dn, NULL,
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 1a65f96fe2aff591..7bd053a32fad1a3c 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -4480,6 +4480,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
>                         return NOTIFY_OK;
>                 }
>
> +               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
>                 spi = of_register_spi_device(ctlr, rd->dn);
>                 put_device(&ctlr->dev);
>
> --
> 2.34.1
>

Re: [PATCH/RFC] treewide: Fix instantiation of devices in DT overlay

[Saravana Kannan]

On Fri, Mar 17, 2023 at 5:36 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Fri, Mar 17, 2023 at 3:33 AM Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
> >
> > When loading a DT overlay that creates a device, the device is not
> > instantiated, unless the DT overlay is unloaded and reloaded again.
> >
> > Saravana explains:
> >   Basically for all overlays (I hope the function is only used for
> >   overlays) we assume all nodes are NOT devices until they actually
> >   get added as a device. Don't review the code, it's not meant to be :)
> >
> > Based on a hacky patch by Saravana Kannan, which covered only platform
> > and spi devices.
> >
> > Fixes: 4a032827daa89350 ("of: property: Simplify of_link_to_phandle()")
> > Link: https://lore.kernel.org/all/CAGETcx_+rhHvaC_HJXGrr5_WAd2+k5f=rWYnkCZ6z5bGX-wj4w@mail.gmail.com
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Marked RFC as Saravana said this is an ugly hack.
> > Still, this is a regression in v6.3-rc1 that should be fixed.
>
> Thanks for making sure this isn't forgotten.
>
> I thought about this a bit more and I've decided what I gave earlier
> isn't really too much of a hack. The other option is to handle the
> clearing of the flag at the driver core level, but we incur these
> additional instructions for all devices instead of just the overlay
> case. But the benefit is that if more busses add overlay support in
> the future, they won't need to remember to clear the flag in those
> instances too. But they'll probably start off by looking at the
> existing platform bus case, so they'll get it right.
>
> I'll continue the pondering next week and maybe test it on my device
> to make sure it's not doing anything weird for non-overlay cases.
>

Geert,

I think we should stick with the original style of fix I suggested.
So, basically your patch set. Are you planning on sending a non-RFC or
do you want me to do it?

-Saravana

> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3611,6 +3611,15 @@ int device_add(struct device *dev)
>          */
>         if (dev->fwnode && !dev->fwnode->dev) {
>                 dev->fwnode->dev = dev;
> +               /*
> +                * If a fwnode was initially marked as not a device, but we
> +                * clearly have a device added for it that can probe, then clear
> +                * the flag so fw_devlink will continue linking consumers to
> +                * this device. This code path is really expected to run only
> +                * for DT overlays.
> +                */
> +               if (dev->bus)
> +                       dev->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE
>                 fw_devlink_link_device(dev);
>         }
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 07d93753b12f..f715b59d9bf3 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -226,6 +226,11 @@ static void __of_attach_node(struct device_node *np)
>         np->sibling = np->parent->child;
>         np->parent->child = np;
>         of_node_clear_flag(np, OF_DETACHED);
> +       /*
> +        * Ask fw_devlink to assume any new node is not a device. Driver core
> +        * will clear this flag if the assumption turns out to be wrong.
> +        */
> +       np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
>  }
>
>
>
>
> > ---
> >  drivers/bus/imx-weim.c    | 1 +
> >  drivers/i2c/i2c-core-of.c | 1 +
> >  drivers/of/dynamic.c      | 1 +
> >  drivers/of/platform.c     | 1 +
> >  drivers/spi/spi.c         | 1 +
> >  5 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> > index 2a6b4f676458612e..71d8807170fa9f29 100644
> > --- a/drivers/bus/imx-weim.c
> > +++ b/drivers/bus/imx-weim.c
> > @@ -329,6 +329,7 @@ static int of_weim_notify(struct notifier_block *nb, unsigned long action,
> >                                  "Failed to setup timing for '%pOF'\n", rd->dn);
> >
> >                 if (!of_node_check_flag(rd->dn, OF_POPULATED)) {
> > +                       rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
> >                         if (!of_platform_device_create(rd->dn, NULL, &pdev->dev)) {
> >                                 dev_err(&pdev->dev,
> >                                         "Failed to create child device '%pOF'\n",
> > diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
> > index bce6b796e04c2ca0..79a0d47010ba0b20 100644
> > --- a/drivers/i2c/i2c-core-of.c
> > +++ b/drivers/i2c/i2c-core-of.c
> > @@ -178,6 +178,7 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
> >                         return NOTIFY_OK;
> >                 }
> >
> > +               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
> >                 client = of_i2c_register_device(adap, rd->dn);
> >                 if (IS_ERR(client)) {
> >                         dev_err(&adap->dev, "failed to create client for '%pOF'\n",
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index 07d93753b12f5f4d..e311d406b1705306 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> > @@ -226,6 +226,7 @@ static void __of_attach_node(struct device_node *np)
> >         np->sibling = np->parent->child;
> >         np->parent->child = np;
> >         of_node_clear_flag(np, OF_DETACHED);
> > +       np->fwnode.flags |= FWNODE_FLAG_NOT_DEVICE;
> >  }
> >
> >  /**
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index b2bd2e783445dd78..17c92cbfb62ee3ef 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -737,6 +737,7 @@ static int of_platform_notify(struct notifier_block *nb,
> >                 if (of_node_check_flag(rd->dn, OF_POPULATED))
> >                         return NOTIFY_OK;
> >
> > +               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
> >                 /* pdev_parent may be NULL when no bus platform device */
> >                 pdev_parent = of_find_device_by_node(rd->dn->parent);
> >                 pdev = of_platform_device_create(rd->dn, NULL,
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 1a65f96fe2aff591..7bd053a32fad1a3c 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -4480,6 +4480,7 @@ static int of_spi_notify(struct notifier_block *nb, unsigned long action,
> >                         return NOTIFY_OK;
> >                 }
> >
> > +               rd->dn->fwnode.flags &= ~FWNODE_FLAG_NOT_DEVICE;
> >                 spi = of_register_spi_device(ctlr, rd->dn);
> >                 put_device(&ctlr->dev);
> >
> > --
> > 2.34.1
> >

Re: [PATCH/RFC] treewide: Fix instantiation of devices in DT overlay

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On Fri, Mar 17, 2023 at 11:33:18AM +0100, Geert Uytterhoeven wrote:
> When loading a DT overlay that creates a device, the device is not
> instantiated, unless the DT overlay is unloaded and reloaded again.
> 
> Saravana explains:
>   Basically for all overlays (I hope the function is only used for
>   overlays) we assume all nodes are NOT devices until they actually
>   get added as a device. Don't review the code, it's not meant to be :)
> 
> Based on a hacky patch by Saravana Kannan, which covered only platform
> and spi devices.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]