| Message ID | 1414528565-10907-5-git-send-email-pantelis.antoniou@konsulko.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 28 Oct 2014 22:36:01 +0200 , Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote: > Add OF notifier handler needed for creating/destroying platform devices > according to dynamic runtime changes in the DT live tree. > > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Hi Pantelis, Some comments below. Feel free to send me fixup patches instead of respinning. g. > --- > drivers/base/platform.c | 18 +++++++++-- > drivers/of/platform.c | 78 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/of_platform.h | 10 ++++++ > 3 files changed, 103 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index b2afc29..282bfec 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -1002,10 +1002,22 @@ int __init platform_bus_init(void) > > error = device_register(&platform_bus); > if (error) > - return error; > - error = bus_register(&platform_bus_type); > + goto err_out; > + > + error = bus_register(&platform_bus_type); > + if (error) > + goto err_unreg_dev; > + > + error = of_platform_register_reconfig_notifier(); > if (error) > - device_unregister(&platform_bus); > + goto err_unreg_bus; We really don't want to fail out here. If the notifier registration fails, it doesn't make sense to cause the entire boot to fail. Instead of refactoring the function, just add the call to of_platform_register_reconfig_notifier(), and WARN_ON() failure without bailing. (Actually, you don't need to send me a fixup for this; I've gone ahead and fixed it up in my tree) > + > + return 0; > +err_unreg_bus: > + bus_unregister(&platform_bus_type); > +err_unreg_dev: > + device_unregister(&platform_bus); > +err_out: > return error; > } > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 3b64d0b..aa8db92 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -546,4 +546,82 @@ void of_platform_depopulate(struct device *parent) > } > EXPORT_SYMBOL_GPL(of_platform_depopulate); > > +#ifdef CONFIG_OF_DYNAMIC > + > +static struct notifier_block platform_of_notifier; > + > +static int of_platform_notify(struct notifier_block *nb, > + unsigned long action, void *arg) > +{ > + struct platform_device *pdev_parent, *pdev; > + struct device_node *dn; > + int state; > + bool children_left; > + > + state = of_reconfig_get_state_change(action, arg); > + > + /* no change? */ > + if (state == -1) > + return NOTIFY_OK; > + > + switch (action) { > + case OF_RECONFIG_ATTACH_NODE: > + case OF_RECONFIG_DETACH_NODE: > + dn = arg; > + break; > + case OF_RECONFIG_ADD_PROPERTY: > + case OF_RECONFIG_REMOVE_PROPERTY: > + case OF_RECONFIG_UPDATE_PROPERTY: > + dn = ((struct of_prop_reconfig *)arg)->dn; > + break; > + default: > + return NOTIFY_OK; > + } > + > + if (state) { > + > + /* verify that the parent is a bus */ > + if (!of_match_node(of_default_bus_match_table, dn->parent)) > + return NOTIFY_OK; /* not for us */ This doesn't work reliably. Not all callers of of_platform_populate use the of_default_bus_match_table. The code needs to actively track the nodes that were used to create child devices. We've got a flag in the device node now that you can use for that; OF_POPULATED_BUS. > + > + /* pdev_parent may be NULL when no bus platform device */ > + pdev_parent = of_find_device_by_node(dn->parent); > + pdev = of_platform_device_create(dn, NULL, > + pdev_parent ? &pdev_parent->dev : NULL); > + of_dev_put(pdev_parent); > + > + if (pdev == NULL) { > + pr_err("%s: failed to create for '%s'\n", > + __func__, dn->full_name); > + /* of_platform_device_create tosses the error code */ > + return notifier_from_errno(-EINVAL); > + } > + > + } else { > + > + /* find our device by node */ > + pdev = of_find_device_by_node(dn); > + if (pdev == NULL) > + return NOTIFY_OK; /* no? not meant for us */ > + > + /* unregister takes one ref away */ > + of_platform_device_destroy(&pdev->dev, &children_left); > + > + /* and put the reference of the find */ > + of_dev_put(pdev); > + > + } > + > + return NOTIFY_OK; > +} > + > +int of_platform_register_reconfig_notifier(void) > +{ > + platform_of_notifier.notifier_call = of_platform_notify; > + return of_reconfig_notifier_register(&platform_of_notifier); > +} > +EXPORT_SYMBOL_GPL(of_platform_register_reconfig_notifier); > + > +#endif > + > #endif /* CONFIG_OF_ADDRESS */ > diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h > index c2b0627..01fe5d6 100644 > --- a/include/linux/of_platform.h > +++ b/include/linux/of_platform.h > @@ -84,4 +84,14 @@ static inline int of_platform_populate(struct device_node *root, > static inline void of_platform_depopulate(struct device *parent) { } > #endif > > +#ifdef CONFIG_OF_DYNAMIC > +extern int of_platform_register_reconfig_notifier(void); > +#else > +static inline int of_platform_register_reconfig_notifier(void) > +{ > + return 0; > +} > +#endif > + > + > #endif /* _LINUX_OF_PLATFORM_H */ > -- > 1.7.12 > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, > On Nov 14, 2014, at 01:29 , Grant Likely <grant.likely@secretlab.ca> wrote: > > On Tue, 28 Oct 2014 22:36:01 +0200 > , Pantelis Antoniou <pantelis.antoniou@konsulko.com> > wrote: >> Add OF notifier handler needed for creating/destroying platform devices >> according to dynamic runtime changes in the DT live tree. >> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> > > Hi Pantelis, > > Some comments below. Feel free to send me fixup patches instead of > respinning. > Sure, I’ll get around to send updated patches over the weekend. > g. > >> --- >> drivers/base/platform.c | 18 +++++++++-- >> drivers/of/platform.c | 78 +++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/of_platform.h | 10 ++++++ >> 3 files changed, 103 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c >> index b2afc29..282bfec 100644 >> --- a/drivers/base/platform.c >> +++ b/drivers/base/platform.c >> @@ -1002,10 +1002,22 @@ int __init platform_bus_init(void) >> >> error = device_register(&platform_bus); >> if (error) >> - return error; >> - error = bus_register(&platform_bus_type); >> + goto err_out; >> + >> + error = bus_register(&platform_bus_type); >> + if (error) >> + goto err_unreg_dev; >> + >> + error = of_platform_register_reconfig_notifier(); >> if (error) >> - device_unregister(&platform_bus); >> + goto err_unreg_bus; > > We really don't want to fail out here. If the notifier registration fails, it > doesn't make sense to cause the entire boot to fail. > > Instead of refactoring the function, just add the call to > of_platform_register_reconfig_notifier(), and WARN_ON() failure without > bailing. > > (Actually, you don't need to send me a fixup for this; I've gone ahead > and fixed it up in my tree) > I see. Thanks. I’m curious under which condition the of_platform_register_reconfig_notifier() would fail. Only under extreme low memory conditions (and very early in the boot-sequence). I doubt we’ll get very far - the boot-sequence will croak shortly after. >> + >> + return 0; >> +err_unreg_bus: >> + bus_unregister(&platform_bus_type); >> +err_unreg_dev: >> + device_unregister(&platform_bus); >> +err_out: >> return error; >> } >> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index 3b64d0b..aa8db92 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -546,4 +546,82 @@ void of_platform_depopulate(struct device *parent) >> } >> EXPORT_SYMBOL_GPL(of_platform_depopulate); >> >> +#ifdef CONFIG_OF_DYNAMIC >> + >> +static struct notifier_block platform_of_notifier; >> + >> +static int of_platform_notify(struct notifier_block *nb, >> + unsigned long action, void *arg) >> +{ >> + struct platform_device *pdev_parent, *pdev; >> + struct device_node *dn; >> + int state; >> + bool children_left; >> + >> + state = of_reconfig_get_state_change(action, arg); >> + >> + /* no change? */ >> + if (state == -1) >> + return NOTIFY_OK; >> + >> + switch (action) { >> + case OF_RECONFIG_ATTACH_NODE: >> + case OF_RECONFIG_DETACH_NODE: >> + dn = arg; >> + break; >> + case OF_RECONFIG_ADD_PROPERTY: >> + case OF_RECONFIG_REMOVE_PROPERTY: >> + case OF_RECONFIG_UPDATE_PROPERTY: >> + dn = ((struct of_prop_reconfig *)arg)->dn; >> + break; >> + default: >> + return NOTIFY_OK; >> + } >> + >> + if (state) { >> + >> + /* verify that the parent is a bus */ >> + if (!of_match_node(of_default_bus_match_table, dn->parent)) >> + return NOTIFY_OK; /* not for us */ > > This doesn't work reliably. Not all callers of of_platform_populate use > the of_default_bus_match_table. The code needs to actively track the > nodes that were used to create child devices. We've got a flag in the > device node now that you can use for that; OF_POPULATED_BUS. > I see. That’s a relatively new flag no? >> + >> + /* pdev_parent may be NULL when no bus platform device */ >> + pdev_parent = of_find_device_by_node(dn->parent); >> + pdev = of_platform_device_create(dn, NULL, >> + pdev_parent ? &pdev_parent->dev : NULL); >> + of_dev_put(pdev_parent); >> + >> + if (pdev == NULL) { >> + pr_err("%s: failed to create for '%s'\n", >> + __func__, dn->full_name); >> + /* of_platform_device_create tosses the error code */ >> + return notifier_from_errno(-EINVAL); >> + } >> + >> + } else { >> + >> + /* find our device by node */ >> + pdev = of_find_device_by_node(dn); >> + if (pdev == NULL) >> + return NOTIFY_OK; /* no? not meant for us */ >> + >> + /* unregister takes one ref away */ >> + of_platform_device_destroy(&pdev->dev, &children_left); >> + >> + /* and put the reference of the find */ >> + of_dev_put(pdev); >> + >> + } >> + >> + return NOTIFY_OK; >> +} >> + >> +int of_platform_register_reconfig_notifier(void) >> +{ >> + platform_of_notifier.notifier_call = of_platform_notify; >> + return of_reconfig_notifier_register(&platform_of_notifier); >> +} >> +EXPORT_SYMBOL_GPL(of_platform_register_reconfig_notifier); >> + >> +#endif >> + >> #endif /* CONFIG_OF_ADDRESS */ >> diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h >> index c2b0627..01fe5d6 100644 >> --- a/include/linux/of_platform.h >> +++ b/include/linux/of_platform.h >> @@ -84,4 +84,14 @@ static inline int of_platform_populate(struct device_node *root, >> static inline void of_platform_depopulate(struct device *parent) { } >> #endif >> >> +#ifdef CONFIG_OF_DYNAMIC >> +extern int of_platform_register_reconfig_notifier(void); >> +#else >> +static inline int of_platform_register_reconfig_notifier(void) >> +{ >> + return 0; >> +} >> +#endif >> + >> + >> #endif /* _LINUX_OF_PLATFORM_H */ >> -- >> 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index b2afc29..282bfec 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -1002,10 +1002,22 @@ int __init platform_bus_init(void) error = device_register(&platform_bus); if (error) - return error; - error = bus_register(&platform_bus_type); + goto err_out; + + error = bus_register(&platform_bus_type); + if (error) + goto err_unreg_dev; + + error = of_platform_register_reconfig_notifier(); if (error) - device_unregister(&platform_bus); + goto err_unreg_bus; + + return 0; +err_unreg_bus: + bus_unregister(&platform_bus_type); +err_unreg_dev: + device_unregister(&platform_bus); +err_out: return error; } diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 3b64d0b..aa8db92 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -546,4 +546,82 @@ void of_platform_depopulate(struct device *parent) } EXPORT_SYMBOL_GPL(of_platform_depopulate); +#ifdef CONFIG_OF_DYNAMIC + +static struct notifier_block platform_of_notifier; + +static int of_platform_notify(struct notifier_block *nb, + unsigned long action, void *arg) +{ + struct platform_device *pdev_parent, *pdev; + struct device_node *dn; + int state; + bool children_left; + + state = of_reconfig_get_state_change(action, arg); + + /* no change? */ + if (state == -1) + return NOTIFY_OK; + + switch (action) { + case OF_RECONFIG_ATTACH_NODE: + case OF_RECONFIG_DETACH_NODE: + dn = arg; + break; + case OF_RECONFIG_ADD_PROPERTY: + case OF_RECONFIG_REMOVE_PROPERTY: + case OF_RECONFIG_UPDATE_PROPERTY: + dn = ((struct of_prop_reconfig *)arg)->dn; + break; + default: + return NOTIFY_OK; + } + + if (state) { + + /* verify that the parent is a bus */ + if (!of_match_node(of_default_bus_match_table, dn->parent)) + return NOTIFY_OK; /* not for us */ + + /* pdev_parent may be NULL when no bus platform device */ + pdev_parent = of_find_device_by_node(dn->parent); + pdev = of_platform_device_create(dn, NULL, + pdev_parent ? &pdev_parent->dev : NULL); + of_dev_put(pdev_parent); + + if (pdev == NULL) { + pr_err("%s: failed to create for '%s'\n", + __func__, dn->full_name); + /* of_platform_device_create tosses the error code */ + return notifier_from_errno(-EINVAL); + } + + } else { + + /* find our device by node */ + pdev = of_find_device_by_node(dn); + if (pdev == NULL) + return NOTIFY_OK; /* no? not meant for us */ + + /* unregister takes one ref away */ + of_platform_device_destroy(&pdev->dev, &children_left); + + /* and put the reference of the find */ + of_dev_put(pdev); + + } + + return NOTIFY_OK; +} + +int of_platform_register_reconfig_notifier(void) +{ + platform_of_notifier.notifier_call = of_platform_notify; + return of_reconfig_notifier_register(&platform_of_notifier); +} +EXPORT_SYMBOL_GPL(of_platform_register_reconfig_notifier); + +#endif + #endif /* CONFIG_OF_ADDRESS */ diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h index c2b0627..01fe5d6 100644 --- a/include/linux/of_platform.h +++ b/include/linux/of_platform.h @@ -84,4 +84,14 @@ static inline int of_platform_populate(struct device_node *root, static inline void of_platform_depopulate(struct device *parent) { } #endif +#ifdef CONFIG_OF_DYNAMIC +extern int of_platform_register_reconfig_notifier(void); +#else +static inline int of_platform_register_reconfig_notifier(void) +{ + return 0; +} +#endif + + #endif /* _LINUX_OF_PLATFORM_H */
Add OF notifier handler needed for creating/destroying platform devices according to dynamic runtime changes in the DT live tree. Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> --- drivers/base/platform.c | 18 +++++++++-- drivers/of/platform.c | 78 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_platform.h | 10 ++++++ 3 files changed, 103 insertions(+), 3 deletions(-)