* [PATCH] OF: mark released devices as no longer populated @ 2016-08-09 9:33 ` Russell King 0 siblings, 0 replies; 15+ messages in thread From: Russell King @ 2016-08-09 9:33 UTC (permalink / raw) To: Rob Herring, Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA Cc: Fabio Estevam, horia.geanta-3arQi8VN3Tc, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r When a Linux device is released and cleaned up, we left the OF device node marked as populated. This causes the Freescale CAAM driver (drivers/crypto/caam) problems when the module is removed and re- inserted: JR0 Platform device creation error JR0 Platform device creation error caam 2100000.caam: no queues configured, terminating caam: probe of 2100000.caam failed with error -12 The reason is that CAAM creates platform devices for each job ring: for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { ctrlpriv->jrpdev[ring] = of_platform_device_create(np, NULL, dev); which sets OF_POPULATED on the device node, but then it cleans these up: /* Remove platform devices for JobRs */ for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { if (ctrlpriv->jrpdev[ring]) of_device_unregister(ctrlpriv->jrpdev[ring]); } which leaves OF_POPULATED set. Arrange for platform devices with a device node to clear the OF_POPULATED bit when they are released. Signed-off-by: Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> --- Please check this carefully - it may have issues where an of_node pointer is copied from one platform device to another, but IMHO doing that is itself buggy behaviour. Resending due to wrong list address, sorry. include/linux/of_device.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/of_device.h b/include/linux/of_device.h index cc7dd687a89d..7a8362d0c6d2 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env static inline void of_device_node_put(struct device *dev) { + of_node_clear_flag(dev->of_node, OF_POPULATED); of_node_put(dev->of_node); } -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] OF: mark released devices as no longer populated @ 2016-08-09 9:33 ` Russell King 0 siblings, 0 replies; 15+ messages in thread From: Russell King @ 2016-08-09 9:33 UTC (permalink / raw) To: linux-arm-kernel When a Linux device is released and cleaned up, we left the OF device node marked as populated. This causes the Freescale CAAM driver (drivers/crypto/caam) problems when the module is removed and re- inserted: JR0 Platform device creation error JR0 Platform device creation error caam 2100000.caam: no queues configured, terminating caam: probe of 2100000.caam failed with error -12 The reason is that CAAM creates platform devices for each job ring: for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { ctrlpriv->jrpdev[ring] = of_platform_device_create(np, NULL, dev); which sets OF_POPULATED on the device node, but then it cleans these up: /* Remove platform devices for JobRs */ for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { if (ctrlpriv->jrpdev[ring]) of_device_unregister(ctrlpriv->jrpdev[ring]); } which leaves OF_POPULATED set. Arrange for platform devices with a device node to clear the OF_POPULATED bit when they are released. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- Please check this carefully - it may have issues where an of_node pointer is copied from one platform device to another, but IMHO doing that is itself buggy behaviour. Resending due to wrong list address, sorry. include/linux/of_device.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/of_device.h b/include/linux/of_device.h index cc7dd687a89d..7a8362d0c6d2 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env static inline void of_device_node_put(struct device *dev) { + of_node_clear_flag(dev->of_node, OF_POPULATED); of_node_put(dev->of_node); } -- 2.1.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
[parent not found: <E1bX3Pw-00088i-KU-eh5Bv4kxaXIk46pC+1QYvQNdhmdF6hFW@public.gmane.org>]
* Re: [PATCH] OF: mark released devices as no longer populated 2016-08-09 9:33 ` Russell King @ 2016-08-09 16:48 ` Rob Herring -1 siblings, 0 replies; 15+ messages in thread From: Rob Herring @ 2016-08-09 16:48 UTC (permalink / raw) To: Russell King Cc: Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA, Fabio Estevam, horia.geanta-3arQi8VN3Tc, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> wrote: > When a Linux device is released and cleaned up, we left the OF device > node marked as populated. This causes the Freescale CAAM driver > (drivers/crypto/caam) problems when the module is removed and re- > inserted: > > JR0 Platform device creation error > JR0 Platform device creation error > caam 2100000.caam: no queues configured, terminating > caam: probe of 2100000.caam failed with error -12 > > The reason is that CAAM creates platform devices for each job ring: > > for_each_available_child_of_node(nprop, np) > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > ctrlpriv->jrpdev[ring] = > of_platform_device_create(np, NULL, dev); > > which sets OF_POPULATED on the device node, but then it cleans these > up: > > /* Remove platform devices for JobRs */ > for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { > if (ctrlpriv->jrpdev[ring]) > of_device_unregister(ctrlpriv->jrpdev[ring]); This looks a bit asymmetrical to me with a of_platform_device_* call and a of_device_* call. I think you could use of_platform_{de}populate here instead. That would simplify things in the driver a bit too as you wouldn't need to store jrpdev. It wouldn't work if there are other child nodes with compatible strings which you don't want devices created. > } > > which leaves OF_POPULATED set. > > Arrange for platform devices with a device node to clear the > OF_POPULATED bit when they are released. > > Signed-off-by: Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> > --- > Please check this carefully - it may have issues where an of_node > pointer is copied from one platform device to another, but IMHO > doing that is itself buggy behaviour. Agreed, that is wrong. > > Resending due to wrong list address, sorry. > > include/linux/of_device.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > index cc7dd687a89d..7a8362d0c6d2 100644 > --- a/include/linux/of_device.h > +++ b/include/linux/of_device.h > @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env > > static inline void of_device_node_put(struct device *dev) > { > + of_node_clear_flag(dev->of_node, OF_POPULATED); This would result in clearing the flag twice in the of_platform_populate/of_platform_depopulate case. It would do the same for other bus types like i2c as well. That doesn't really hurt anything that I can think of, but just not the best implementation. I think adding a of_platform_device_unregister() call that wraps of_platform_device_destroy would be more balanced. I looked thru all the callers of of_platform_device_create. The only other ones affected by this are: drivers/macintosh/ams/ams-core.c drivers/macintosh/therm_adt746x.c drivers/macintosh/therm_windtunnel.c The others either have no remove path or a buggy remove path. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] OF: mark released devices as no longer populated @ 2016-08-09 16:48 ` Rob Herring 0 siblings, 0 replies; 15+ messages in thread From: Rob Herring @ 2016-08-09 16:48 UTC (permalink / raw) To: linux-arm-kernel On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel@armlinux.org.uk> wrote: > When a Linux device is released and cleaned up, we left the OF device > node marked as populated. This causes the Freescale CAAM driver > (drivers/crypto/caam) problems when the module is removed and re- > inserted: > > JR0 Platform device creation error > JR0 Platform device creation error > caam 2100000.caam: no queues configured, terminating > caam: probe of 2100000.caam failed with error -12 > > The reason is that CAAM creates platform devices for each job ring: > > for_each_available_child_of_node(nprop, np) > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > ctrlpriv->jrpdev[ring] = > of_platform_device_create(np, NULL, dev); > > which sets OF_POPULATED on the device node, but then it cleans these > up: > > /* Remove platform devices for JobRs */ > for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { > if (ctrlpriv->jrpdev[ring]) > of_device_unregister(ctrlpriv->jrpdev[ring]); This looks a bit asymmetrical to me with a of_platform_device_* call and a of_device_* call. I think you could use of_platform_{de}populate here instead. That would simplify things in the driver a bit too as you wouldn't need to store jrpdev. It wouldn't work if there are other child nodes with compatible strings which you don't want devices created. > } > > which leaves OF_POPULATED set. > > Arrange for platform devices with a device node to clear the > OF_POPULATED bit when they are released. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > Please check this carefully - it may have issues where an of_node > pointer is copied from one platform device to another, but IMHO > doing that is itself buggy behaviour. Agreed, that is wrong. > > Resending due to wrong list address, sorry. > > include/linux/of_device.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > index cc7dd687a89d..7a8362d0c6d2 100644 > --- a/include/linux/of_device.h > +++ b/include/linux/of_device.h > @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env > > static inline void of_device_node_put(struct device *dev) > { > + of_node_clear_flag(dev->of_node, OF_POPULATED); This would result in clearing the flag twice in the of_platform_populate/of_platform_depopulate case. It would do the same for other bus types like i2c as well. That doesn't really hurt anything that I can think of, but just not the best implementation. I think adding a of_platform_device_unregister() call that wraps of_platform_device_destroy would be more balanced. I looked thru all the callers of of_platform_device_create. The only other ones affected by this are: drivers/macintosh/ams/ams-core.c drivers/macintosh/therm_adt746x.c drivers/macintosh/therm_windtunnel.c The others either have no remove path or a buggy remove path. Rob ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <CAL_Jsq+pDeL1_=p06ynrV4CK+h8-Yx_KDbH-W=d-aSdARe3ZHw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH] OF: mark released devices as no longer populated 2016-08-09 16:48 ` Rob Herring @ 2017-03-31 10:39 ` Russell King - ARM Linux -1 siblings, 0 replies; 15+ messages in thread From: Russell King - ARM Linux @ 2017-03-31 10:39 UTC (permalink / raw) To: Rob Herring, Fabio Estevam, horia.geanta-3arQi8VN3Tc Cc: Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Ping, this issue still exists with 4.11-rc4 - and there's been no reaction from the alleged CAAM maintainers. On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote: > On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> wrote: > > When a Linux device is released and cleaned up, we left the OF device > > node marked as populated. This causes the Freescale CAAM driver > > (drivers/crypto/caam) problems when the module is removed and re- > > inserted: > > > > JR0 Platform device creation error > > JR0 Platform device creation error > > caam 2100000.caam: no queues configured, terminating > > caam: probe of 2100000.caam failed with error -12 > > > > The reason is that CAAM creates platform devices for each job ring: > > > > for_each_available_child_of_node(nprop, np) > > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > > ctrlpriv->jrpdev[ring] = > > of_platform_device_create(np, NULL, dev); > > > > which sets OF_POPULATED on the device node, but then it cleans these > > up: > > > > /* Remove platform devices for JobRs */ > > for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { > > if (ctrlpriv->jrpdev[ring]) > > of_device_unregister(ctrlpriv->jrpdev[ring]); > > This looks a bit asymmetrical to me with a of_platform_device_* call > and a of_device_* call. > > I think you could use of_platform_{de}populate here instead. That > would simplify things in the driver a bit too as you wouldn't need to > store jrpdev. It wouldn't work if there are other child nodes with > compatible strings which you don't want devices created. > > > } > > > > which leaves OF_POPULATED set. > > > > Arrange for platform devices with a device node to clear the > > OF_POPULATED bit when they are released. > > > > Signed-off-by: Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> > > --- > > Please check this carefully - it may have issues where an of_node > > pointer is copied from one platform device to another, but IMHO > > doing that is itself buggy behaviour. > > Agreed, that is wrong. > > > > > Resending due to wrong list address, sorry. > > > > include/linux/of_device.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > > index cc7dd687a89d..7a8362d0c6d2 100644 > > --- a/include/linux/of_device.h > > +++ b/include/linux/of_device.h > > @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env > > > > static inline void of_device_node_put(struct device *dev) > > { > > + of_node_clear_flag(dev->of_node, OF_POPULATED); > > This would result in clearing the flag twice in the > of_platform_populate/of_platform_depopulate case. It would do the same > for other bus types like i2c as well. That doesn't really hurt > anything that I can think of, but just not the best implementation. I > think adding a of_platform_device_unregister() call that wraps > of_platform_device_destroy would be more balanced. > > I looked thru all the callers of of_platform_device_create. The only > other ones affected by this are: > > drivers/macintosh/ams/ams-core.c > drivers/macintosh/therm_adt746x.c > drivers/macintosh/therm_windtunnel.c > > The others either have no remove path or a buggy remove path. > > Rob -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] OF: mark released devices as no longer populated @ 2017-03-31 10:39 ` Russell King - ARM Linux 0 siblings, 0 replies; 15+ messages in thread From: Russell King - ARM Linux @ 2017-03-31 10:39 UTC (permalink / raw) To: linux-arm-kernel Ping, this issue still exists with 4.11-rc4 - and there's been no reaction from the alleged CAAM maintainers. On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote: > On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel@armlinux.org.uk> wrote: > > When a Linux device is released and cleaned up, we left the OF device > > node marked as populated. This causes the Freescale CAAM driver > > (drivers/crypto/caam) problems when the module is removed and re- > > inserted: > > > > JR0 Platform device creation error > > JR0 Platform device creation error > > caam 2100000.caam: no queues configured, terminating > > caam: probe of 2100000.caam failed with error -12 > > > > The reason is that CAAM creates platform devices for each job ring: > > > > for_each_available_child_of_node(nprop, np) > > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > > ctrlpriv->jrpdev[ring] = > > of_platform_device_create(np, NULL, dev); > > > > which sets OF_POPULATED on the device node, but then it cleans these > > up: > > > > /* Remove platform devices for JobRs */ > > for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { > > if (ctrlpriv->jrpdev[ring]) > > of_device_unregister(ctrlpriv->jrpdev[ring]); > > This looks a bit asymmetrical to me with a of_platform_device_* call > and a of_device_* call. > > I think you could use of_platform_{de}populate here instead. That > would simplify things in the driver a bit too as you wouldn't need to > store jrpdev. It wouldn't work if there are other child nodes with > compatible strings which you don't want devices created. > > > } > > > > which leaves OF_POPULATED set. > > > > Arrange for platform devices with a device node to clear the > > OF_POPULATED bit when they are released. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > Please check this carefully - it may have issues where an of_node > > pointer is copied from one platform device to another, but IMHO > > doing that is itself buggy behaviour. > > Agreed, that is wrong. > > > > > Resending due to wrong list address, sorry. > > > > include/linux/of_device.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/linux/of_device.h b/include/linux/of_device.h > > index cc7dd687a89d..7a8362d0c6d2 100644 > > --- a/include/linux/of_device.h > > +++ b/include/linux/of_device.h > > @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env > > > > static inline void of_device_node_put(struct device *dev) > > { > > + of_node_clear_flag(dev->of_node, OF_POPULATED); > > This would result in clearing the flag twice in the > of_platform_populate/of_platform_depopulate case. It would do the same > for other bus types like i2c as well. That doesn't really hurt > anything that I can think of, but just not the best implementation. I > think adding a of_platform_device_unregister() call that wraps > of_platform_device_destroy would be more balanced. > > I looked thru all the callers of of_platform_device_create. The only > other ones affected by this are: > > drivers/macintosh/ams/ams-core.c > drivers/macintosh/therm_adt746x.c > drivers/macintosh/therm_windtunnel.c > > The others either have no remove path or a buggy remove path. > > Rob -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] OF: mark released devices as no longer populated 2017-03-31 10:39 ` Russell King - ARM Linux @ 2017-03-31 15:23 ` Horia Geantă -1 siblings, 0 replies; 15+ messages in thread From: Horia Geantă @ 2017-03-31 15:23 UTC (permalink / raw) To: Russell King - ARM Linux, Rob Herring, Fabio Estevam Cc: Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-crypto-u79uwXL29TY76Z2rM5mHXA, Dan Douglass On 3/31/2017 1:40 PM, Russell King - ARM Linux wrote: > Ping, this issue still exists with 4.11-rc4 - and there's been no > reaction from the alleged CAAM maintainers. > Sorry, this somehow slipped through (Cc vs. To, no linux-crypto). > On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote: >> On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> wrote: >>> When a Linux device is released and cleaned up, we left the OF device >>> node marked as populated. This causes the Freescale CAAM driver >>> (drivers/crypto/caam) problems when the module is removed and re- >>> inserted: >>> >>> JR0 Platform device creation error >>> JR0 Platform device creation error >>> caam 2100000.caam: no queues configured, terminating >>> caam: probe of 2100000.caam failed with error -12 >>> >>> The reason is that CAAM creates platform devices for each job ring: >>> >>> for_each_available_child_of_node(nprop, np) >>> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || >>> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { >>> ctrlpriv->jrpdev[ring] = >>> of_platform_device_create(np, NULL, dev); >>> >>> which sets OF_POPULATED on the device node, but then it cleans these >>> up: >>> >>> /* Remove platform devices for JobRs */ >>> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { >>> if (ctrlpriv->jrpdev[ring]) >>> of_device_unregister(ctrlpriv->jrpdev[ring]); >> >> This looks a bit asymmetrical to me with a of_platform_device_* call >> and a of_device_* call. >> >> I think you could use of_platform_{de}populate here instead. That >> would simplify things in the driver a bit too as you wouldn't need to >> store jrpdev. It wouldn't work if there are other child nodes with Indeed, this would clean-up the driver a bit. However, the driver needs to know how many of the devices probed successfully - to print the number and more importantly to exit in case total_jobrs = 0. Thus, I would keep the one-by-one probing of the devices. What options are there in this case? Should a function symmetric to of_platform_device_create() be added - to replace of_device_unregister() - or rely on an open-coded solution? Thanks, Horia >> compatible strings which you don't want devices created. >> >>> } >>> >>> which leaves OF_POPULATED set. >>> >>> Arrange for platform devices with a device node to clear the >>> OF_POPULATED bit when they are released. >>> >>> Signed-off-by: Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> >>> --- >>> Please check this carefully - it may have issues where an of_node >>> pointer is copied from one platform device to another, but IMHO >>> doing that is itself buggy behaviour. >> >> Agreed, that is wrong. >> >>> >>> Resending due to wrong list address, sorry. >>> >>> include/linux/of_device.h | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h >>> index cc7dd687a89d..7a8362d0c6d2 100644 >>> --- a/include/linux/of_device.h >>> +++ b/include/linux/of_device.h >>> @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env >>> >>> static inline void of_device_node_put(struct device *dev) >>> { >>> + of_node_clear_flag(dev->of_node, OF_POPULATED); >> >> This would result in clearing the flag twice in the >> of_platform_populate/of_platform_depopulate case. It would do the same >> for other bus types like i2c as well. That doesn't really hurt >> anything that I can think of, but just not the best implementation. I >> think adding a of_platform_device_unregister() call that wraps >> of_platform_device_destroy would be more balanced. >> >> I looked thru all the callers of of_platform_device_create. The only >> other ones affected by this are: >> >> drivers/macintosh/ams/ams-core.c >> drivers/macintosh/therm_adt746x.c >> drivers/macintosh/therm_windtunnel.c >> >> The others either have no remove path or a buggy remove path. >> >> Rob > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] OF: mark released devices as no longer populated @ 2017-03-31 15:23 ` Horia Geantă 0 siblings, 0 replies; 15+ messages in thread From: Horia Geantă @ 2017-03-31 15:23 UTC (permalink / raw) To: linux-arm-kernel On 3/31/2017 1:40 PM, Russell King - ARM Linux wrote: > Ping, this issue still exists with 4.11-rc4 - and there's been no > reaction from the alleged CAAM maintainers. > Sorry, this somehow slipped through (Cc vs. To, no linux-crypto). > On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote: >> On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel@armlinux.org.uk> wrote: >>> When a Linux device is released and cleaned up, we left the OF device >>> node marked as populated. This causes the Freescale CAAM driver >>> (drivers/crypto/caam) problems when the module is removed and re- >>> inserted: >>> >>> JR0 Platform device creation error >>> JR0 Platform device creation error >>> caam 2100000.caam: no queues configured, terminating >>> caam: probe of 2100000.caam failed with error -12 >>> >>> The reason is that CAAM creates platform devices for each job ring: >>> >>> for_each_available_child_of_node(nprop, np) >>> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || >>> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { >>> ctrlpriv->jrpdev[ring] = >>> of_platform_device_create(np, NULL, dev); >>> >>> which sets OF_POPULATED on the device node, but then it cleans these >>> up: >>> >>> /* Remove platform devices for JobRs */ >>> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { >>> if (ctrlpriv->jrpdev[ring]) >>> of_device_unregister(ctrlpriv->jrpdev[ring]); >> >> This looks a bit asymmetrical to me with a of_platform_device_* call >> and a of_device_* call. >> >> I think you could use of_platform_{de}populate here instead. That >> would simplify things in the driver a bit too as you wouldn't need to >> store jrpdev. It wouldn't work if there are other child nodes with Indeed, this would clean-up the driver a bit. However, the driver needs to know how many of the devices probed successfully - to print the number and more importantly to exit in case total_jobrs = 0. Thus, I would keep the one-by-one probing of the devices. What options are there in this case? Should a function symmetric to of_platform_device_create() be added - to replace of_device_unregister() - or rely on an open-coded solution? Thanks, Horia >> compatible strings which you don't want devices created. >> >>> } >>> >>> which leaves OF_POPULATED set. >>> >>> Arrange for platform devices with a device node to clear the >>> OF_POPULATED bit when they are released. >>> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >>> --- >>> Please check this carefully - it may have issues where an of_node >>> pointer is copied from one platform device to another, but IMHO >>> doing that is itself buggy behaviour. >> >> Agreed, that is wrong. >> >>> >>> Resending due to wrong list address, sorry. >>> >>> include/linux/of_device.h | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h >>> index cc7dd687a89d..7a8362d0c6d2 100644 >>> --- a/include/linux/of_device.h >>> +++ b/include/linux/of_device.h >>> @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env >>> >>> static inline void of_device_node_put(struct device *dev) >>> { >>> + of_node_clear_flag(dev->of_node, OF_POPULATED); >> >> This would result in clearing the flag twice in the >> of_platform_populate/of_platform_depopulate case. It would do the same >> for other bus types like i2c as well. That doesn't really hurt >> anything that I can think of, but just not the best implementation. I >> think adding a of_platform_device_unregister() call that wraps >> of_platform_device_destroy would be more balanced. >> >> I looked thru all the callers of of_platform_device_create. The only >> other ones affected by this are: >> >> drivers/macintosh/ams/ams-core.c >> drivers/macintosh/therm_adt746x.c >> drivers/macintosh/therm_windtunnel.c >> >> The others either have no remove path or a buggy remove path. >> >> Rob > ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <VI1PR0401MB2591E5FD962023914F042C9B98370-9IDQY6o3qQhGNIhRVge97I3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>]
* Re: [PATCH] OF: mark released devices as no longer populated 2017-03-31 15:23 ` Horia Geantă @ 2017-03-31 16:59 ` Rob Herring -1 siblings, 0 replies; 15+ messages in thread From: Rob Herring @ 2017-03-31 16:59 UTC (permalink / raw) To: Horia Geantă Cc: Russell King - ARM Linux, Fabio Estevam, Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, linux-crypto-u79uwXL29TY76Z2rM5mHXA, Dan Douglass On Fri, Mar 31, 2017 at 10:23 AM, Horia Geantă <horia.geanta-3arQi8VN3Tc@public.gmane.org> wrote: > On 3/31/2017 1:40 PM, Russell King - ARM Linux wrote: >> Ping, this issue still exists with 4.11-rc4 - and there's been no >> reaction from the alleged CAAM maintainers. >> > Sorry, this somehow slipped through (Cc vs. To, no linux-crypto). > >> On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote: >>> On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> wrote: >>>> When a Linux device is released and cleaned up, we left the OF device >>>> node marked as populated. This causes the Freescale CAAM driver >>>> (drivers/crypto/caam) problems when the module is removed and re- >>>> inserted: >>>> >>>> JR0 Platform device creation error >>>> JR0 Platform device creation error >>>> caam 2100000.caam: no queues configured, terminating >>>> caam: probe of 2100000.caam failed with error -12 >>>> >>>> The reason is that CAAM creates platform devices for each job ring: >>>> >>>> for_each_available_child_of_node(nprop, np) >>>> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || >>>> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { >>>> ctrlpriv->jrpdev[ring] = >>>> of_platform_device_create(np, NULL, dev); >>>> >>>> which sets OF_POPULATED on the device node, but then it cleans these >>>> up: >>>> >>>> /* Remove platform devices for JobRs */ >>>> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { >>>> if (ctrlpriv->jrpdev[ring]) >>>> of_device_unregister(ctrlpriv->jrpdev[ring]); >>> >>> This looks a bit asymmetrical to me with a of_platform_device_* call >>> and a of_device_* call. >>> >>> I think you could use of_platform_{de}populate here instead. That >>> would simplify things in the driver a bit too as you wouldn't need to >>> store jrpdev. It wouldn't work if there are other child nodes with > Indeed, this would clean-up the driver a bit. However, the driver needs > to know how many of the devices probed successfully - to print the > number and more importantly to exit in case total_jobrs = 0. The only thing you are guaranteed is the OF code created some platform devices. That's it. Whether any driver probed successfully is separate and a lot more things can go wrong there. The only thing you are checking is that your dtb is not crap. > Thus, I would keep the one-by-one probing of the devices. > What options are there in this case? > Should a function symmetric to of_platform_device_create() be added - to > replace of_device_unregister() - or rely on an open-coded solution? Certainly not the latter. We don't want drivers mucking with flags internal to the DT code. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] OF: mark released devices as no longer populated @ 2017-03-31 16:59 ` Rob Herring 0 siblings, 0 replies; 15+ messages in thread From: Rob Herring @ 2017-03-31 16:59 UTC (permalink / raw) To: linux-arm-kernel On Fri, Mar 31, 2017 at 10:23 AM, Horia Geant? <horia.geanta@nxp.com> wrote: > On 3/31/2017 1:40 PM, Russell King - ARM Linux wrote: >> Ping, this issue still exists with 4.11-rc4 - and there's been no >> reaction from the alleged CAAM maintainers. >> > Sorry, this somehow slipped through (Cc vs. To, no linux-crypto). > >> On Tue, Aug 09, 2016 at 11:48:38AM -0500, Rob Herring wrote: >>> On Tue, Aug 9, 2016 at 4:33 AM, Russell King <rmk+kernel@armlinux.org.uk> wrote: >>>> When a Linux device is released and cleaned up, we left the OF device >>>> node marked as populated. This causes the Freescale CAAM driver >>>> (drivers/crypto/caam) problems when the module is removed and re- >>>> inserted: >>>> >>>> JR0 Platform device creation error >>>> JR0 Platform device creation error >>>> caam 2100000.caam: no queues configured, terminating >>>> caam: probe of 2100000.caam failed with error -12 >>>> >>>> The reason is that CAAM creates platform devices for each job ring: >>>> >>>> for_each_available_child_of_node(nprop, np) >>>> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || >>>> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { >>>> ctrlpriv->jrpdev[ring] = >>>> of_platform_device_create(np, NULL, dev); >>>> >>>> which sets OF_POPULATED on the device node, but then it cleans these >>>> up: >>>> >>>> /* Remove platform devices for JobRs */ >>>> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { >>>> if (ctrlpriv->jrpdev[ring]) >>>> of_device_unregister(ctrlpriv->jrpdev[ring]); >>> >>> This looks a bit asymmetrical to me with a of_platform_device_* call >>> and a of_device_* call. >>> >>> I think you could use of_platform_{de}populate here instead. That >>> would simplify things in the driver a bit too as you wouldn't need to >>> store jrpdev. It wouldn't work if there are other child nodes with > Indeed, this would clean-up the driver a bit. However, the driver needs > to know how many of the devices probed successfully - to print the > number and more importantly to exit in case total_jobrs = 0. The only thing you are guaranteed is the OF code created some platform devices. That's it. Whether any driver probed successfully is separate and a lot more things can go wrong there. The only thing you are checking is that your dtb is not crap. > Thus, I would keep the one-by-one probing of the devices. > What options are there in this case? > Should a function symmetric to of_platform_device_create() be added - to > replace of_device_unregister() - or rely on an open-coded solution? Certainly not the latter. We don't want drivers mucking with flags internal to the DT code. Rob ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] crypto: caam - fix JR platform device subsequent (re)creations 2017-03-31 16:59 ` Rob Herring (?) @ 2017-04-03 15:12 ` Horia Geantă 2017-04-03 15:52 ` Rob Herring 2017-04-05 14:08 ` Herbert Xu -1 siblings, 2 replies; 15+ messages in thread From: Horia Geantă @ 2017-04-03 15:12 UTC (permalink / raw) To: Herbert Xu, Rob Herring, Russell King Cc: David S. Miller, linux-crypto, Dan Douglass, Ruchika Gupta The way Job Ring platform devices are created and released does not allow for multiple create-release cycles. JR0 Platform device creation error JR0 Platform device creation error caam 2100000.caam: no queues configured, terminating caam: probe of 2100000.caam failed with error -12 The reason is that platform devices are created for each job ring: for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { ctrlpriv->jrpdev[ring] = of_platform_device_create(np, NULL, dev); which sets OF_POPULATED on the device node, but then it cleans these up: /* Remove platform devices for JobRs */ for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { if (ctrlpriv->jrpdev[ring]) of_device_unregister(ctrlpriv->jrpdev[ring]); } which leaves OF_POPULATED set. Use of_platform_populate / of_platform_depopulate instead. This allows for a bit of driver clean-up, jrpdev is no longer needed. Logic changes a bit too: -exit in case of_platform_populate fails, since currently even QI backend depends on JR; true, we no longer support the case when "some" of the JR DT nodes are incorrect -when cleaning up, caam_remove() would also depopulate RTIC in case it would have been populated somewhere else - not the case for now Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring") Reported-by: Russell King <rmk+kernel@armlinux.org.uk> Suggested-by: Rob Herring <robh+dt@kernel.org> Signed-off-by: Horia Geantă <horia.geanta@nxp.com> --- Not sending this directly to -stable, since it does not apply cleanly. drivers/crypto/caam/ctrl.c | 64 ++++++++++++++------------------------------ drivers/crypto/caam/intern.h | 1 - 2 files changed, 20 insertions(+), 45 deletions(-) diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c index b3a94d5eff26..f7792a99469a 100644 --- a/drivers/crypto/caam/ctrl.c +++ b/drivers/crypto/caam/ctrl.c @@ -305,15 +305,13 @@ static int caam_remove(struct platform_device *pdev) struct device *ctrldev; struct caam_drv_private *ctrlpriv; struct caam_ctrl __iomem *ctrl; - int ring; ctrldev = &pdev->dev; ctrlpriv = dev_get_drvdata(ctrldev); ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl; - /* Remove platform devices for JobRs */ - for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) - of_device_unregister(ctrlpriv->jrpdev[ring]); + /* Remove platform devices under the crypto node */ + of_platform_depopulate(ctrldev); #ifdef CONFIG_CAAM_QI if (ctrlpriv->qidev) @@ -410,10 +408,21 @@ int caam_get_era(void) } EXPORT_SYMBOL(caam_get_era); +static const struct of_device_id caam_match[] = { + { + .compatible = "fsl,sec-v4.0", + }, + { + .compatible = "fsl,sec4.0", + }, + {}, +}; +MODULE_DEVICE_TABLE(of, caam_match); + /* Probe routine for CAAM top (controller) level */ static int caam_probe(struct platform_device *pdev) { - int ret, ring, ridx, rspec, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; + int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN; u64 caam_id; struct device *dev; struct device_node *nprop, *np; @@ -589,21 +598,9 @@ static int caam_probe(struct platform_device *pdev) goto iounmap_ctrl; } - /* - * Detect and enable JobRs - * First, find out how many ring spec'ed, allocate references - * for all, then go probe each one. - */ - rspec = 0; - for_each_available_child_of_node(nprop, np) - if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || - of_device_is_compatible(np, "fsl,sec4.0-job-ring")) - rspec++; - - ctrlpriv->jrpdev = devm_kcalloc(&pdev->dev, rspec, - sizeof(*ctrlpriv->jrpdev), GFP_KERNEL); - if (ctrlpriv->jrpdev == NULL) { - ret = -ENOMEM; + ret = of_platform_populate(nprop, caam_match, NULL, dev); + if (ret) { + dev_err(dev, "JR platform devices creation error\n"); goto iounmap_ctrl; } @@ -618,29 +615,19 @@ static int caam_probe(struct platform_device *pdev) ctrlpriv->dfs_root = debugfs_create_dir(dev_name(dev), NULL); ctrlpriv->ctl = debugfs_create_dir("ctl", ctrlpriv->dfs_root); #endif + ring = 0; - ridx = 0; - ctrlpriv->total_jobrs = 0; for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { - ctrlpriv->jrpdev[ring] = - of_platform_device_create(np, NULL, dev); - if (!ctrlpriv->jrpdev[ring]) { - pr_warn("JR physical index %d: Platform device creation error\n", - ridx); - ridx++; - continue; - } ctrlpriv->jr[ring] = (struct caam_job_ring __iomem __force *) ((__force uint8_t *)ctrl + - (ridx + JR_BLOCK_NUMBER) * + (ring + JR_BLOCK_NUMBER) * BLOCK_OFFSET ); ctrlpriv->total_jobrs++; ring++; - ridx++; - } + } /* Check to see if QI present. If so, enable */ ctrlpriv->qi_present = @@ -849,17 +836,6 @@ static int caam_probe(struct platform_device *pdev) return ret; } -static struct of_device_id caam_match[] = { - { - .compatible = "fsl,sec-v4.0", - }, - { - .compatible = "fsl,sec4.0", - }, - {}, -}; -MODULE_DEVICE_TABLE(of, caam_match); - static struct platform_driver caam_driver = { .driver = { .name = "caam", diff --git a/drivers/crypto/caam/intern.h b/drivers/crypto/caam/intern.h index c334df638ff6..85b6c5835b8f 100644 --- a/drivers/crypto/caam/intern.h +++ b/drivers/crypto/caam/intern.h @@ -66,7 +66,6 @@ struct caam_drv_private_jr { struct caam_drv_private { struct device *dev; - struct platform_device **jrpdev; /* Alloc'ed array per sub-device */ #ifdef CONFIG_CAAM_QI struct device *qidev; #endif -- 2.12.0.264.gd6db3f216544 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: caam - fix JR platform device subsequent (re)creations 2017-04-03 15:12 ` [PATCH] crypto: caam - fix JR platform device subsequent (re)creations Horia Geantă @ 2017-04-03 15:52 ` Rob Herring 2017-04-05 14:08 ` Herbert Xu 1 sibling, 0 replies; 15+ messages in thread From: Rob Herring @ 2017-04-03 15:52 UTC (permalink / raw) To: Horia Geantă Cc: Herbert Xu, Russell King, David S. Miller, linux-crypto, Dan Douglass, Ruchika Gupta On Mon, Apr 3, 2017 at 10:12 AM, Horia Geantă <horia.geanta@nxp.com> wrote: > The way Job Ring platform devices are created and released does not > allow for multiple create-release cycles. > > JR0 Platform device creation error > JR0 Platform device creation error > caam 2100000.caam: no queues configured, terminating > caam: probe of 2100000.caam failed with error -12 > > The reason is that platform devices are created for each job ring: > > for_each_available_child_of_node(nprop, np) > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > ctrlpriv->jrpdev[ring] = > of_platform_device_create(np, NULL, dev); > > which sets OF_POPULATED on the device node, but then it cleans these up: > > /* Remove platform devices for JobRs */ > for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { > if (ctrlpriv->jrpdev[ring]) > of_device_unregister(ctrlpriv->jrpdev[ring]); > } > > which leaves OF_POPULATED set. > > Use of_platform_populate / of_platform_depopulate instead. > This allows for a bit of driver clean-up, jrpdev is no longer needed. > > Logic changes a bit too: > -exit in case of_platform_populate fails, since currently even QI backend > depends on JR; true, we no longer support the case when "some" of the JR > DT nodes are incorrect > -when cleaning up, caam_remove() would also depopulate RTIC in case > it would have been populated somewhere else - not the case for now > > Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring") > Reported-by: Russell King <rmk+kernel@armlinux.org.uk> > Suggested-by: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: caam - fix JR platform device subsequent (re)creations 2017-04-03 15:12 ` [PATCH] crypto: caam - fix JR platform device subsequent (re)creations Horia Geantă 2017-04-03 15:52 ` Rob Herring @ 2017-04-05 14:08 ` Herbert Xu 2017-04-05 17:09 ` Horia Geantă 1 sibling, 1 reply; 15+ messages in thread From: Herbert Xu @ 2017-04-05 14:08 UTC (permalink / raw) To: Horia Geantă Cc: Rob Herring, Russell King, David S. Miller, linux-crypto, Dan Douglass, Ruchika Gupta On Mon, Apr 03, 2017 at 06:12:04PM +0300, Horia Geantă wrote: > The way Job Ring platform devices are created and released does not > allow for multiple create-release cycles. > > JR0 Platform device creation error > JR0 Platform device creation error > caam 2100000.caam: no queues configured, terminating > caam: probe of 2100000.caam failed with error -12 > > The reason is that platform devices are created for each job ring: > > for_each_available_child_of_node(nprop, np) > if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || > of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { > ctrlpriv->jrpdev[ring] = > of_platform_device_create(np, NULL, dev); > > which sets OF_POPULATED on the device node, but then it cleans these up: > > /* Remove platform devices for JobRs */ > for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { > if (ctrlpriv->jrpdev[ring]) > of_device_unregister(ctrlpriv->jrpdev[ring]); > } > > which leaves OF_POPULATED set. > > Use of_platform_populate / of_platform_depopulate instead. > This allows for a bit of driver clean-up, jrpdev is no longer needed. > > Logic changes a bit too: > -exit in case of_platform_populate fails, since currently even QI backend > depends on JR; true, we no longer support the case when "some" of the JR > DT nodes are incorrect > -when cleaning up, caam_remove() would also depopulate RTIC in case > it would have been populated somewhere else - not the case for now > > Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring") > Reported-by: Russell King <rmk+kernel@armlinux.org.uk> > Suggested-by: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > --- > Not sending this directly to -stable, since it does not apply cleanly. Patch applied. I forced it to apply on the crypto tree, then merged it forward to cryptodev. Please check the end result and let me know if it's wrong. Thanks, -- Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] crypto: caam - fix JR platform device subsequent (re)creations 2017-04-05 14:08 ` Herbert Xu @ 2017-04-05 17:09 ` Horia Geantă 0 siblings, 0 replies; 15+ messages in thread From: Horia Geantă @ 2017-04-05 17:09 UTC (permalink / raw) To: Herbert Xu Cc: Rob Herring, Russell King, David S. Miller, linux-crypto, Dan Douglass, Ruchika Gupta On 4/5/2017 5:14 PM, Herbert Xu wrote: > On Mon, Apr 03, 2017 at 06:12:04PM +0300, Horia Geantă wrote: >> The way Job Ring platform devices are created and released does not >> allow for multiple create-release cycles. >> >> JR0 Platform device creation error >> JR0 Platform device creation error >> caam 2100000.caam: no queues configured, terminating >> caam: probe of 2100000.caam failed with error -12 >> >> The reason is that platform devices are created for each job ring: >> >> for_each_available_child_of_node(nprop, np) >> if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || >> of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { >> ctrlpriv->jrpdev[ring] = >> of_platform_device_create(np, NULL, dev); >> >> which sets OF_POPULATED on the device node, but then it cleans these up: >> >> /* Remove platform devices for JobRs */ >> for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { >> if (ctrlpriv->jrpdev[ring]) >> of_device_unregister(ctrlpriv->jrpdev[ring]); >> } >> >> which leaves OF_POPULATED set. >> >> Use of_platform_populate / of_platform_depopulate instead. >> This allows for a bit of driver clean-up, jrpdev is no longer needed. >> >> Logic changes a bit too: >> -exit in case of_platform_populate fails, since currently even QI backend >> depends on JR; true, we no longer support the case when "some" of the JR >> DT nodes are incorrect >> -when cleaning up, caam_remove() would also depopulate RTIC in case >> it would have been populated somewhere else - not the case for now >> >> Fixes: 313ea293e9c4d ("crypto: caam - Add Platform driver for Job Ring") >> Reported-by: Russell King <rmk+kernel@armlinux.org.uk> >> Suggested-by: Rob Herring <robh+dt@kernel.org> >> Signed-off-by: Horia Geantă <horia.geanta@nxp.com> >> --- >> Not sending this directly to -stable, since it does not apply cleanly. > > Patch applied. I forced it to apply on the crypto tree, then merged > it forward to cryptodev. Please check the end result and let me know > if it's wrong. > Looks fine. Thanks, Horia ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] OF: mark released devices as no longer populated @ 2016-08-09 9:26 Russell King 0 siblings, 0 replies; 15+ messages in thread From: Russell King @ 2016-08-09 9:26 UTC (permalink / raw) To: Rob Herring, Frank Rowand, devicetree-u79uwXL29TY76Z2rM5mHXA Cc: Fabio Estevam, horia.geanta-3arQi8VN3Tc, linux-arm-kernel-u79uwXL29TY76Z2rM5mHXA When a Linux device is released and cleaned up, we left the OF device node marked as populated. This causes the Freescale CAAM driver (drivers/crypto/caam) problems when the module is removed and re- inserted: JR0 Platform device creation error JR0 Platform device creation error caam 2100000.caam: no queues configured, terminating caam: probe of 2100000.caam failed with error -12 The reason is that CAAM creates platform devices for each job ring: for_each_available_child_of_node(nprop, np) if (of_device_is_compatible(np, "fsl,sec-v4.0-job-ring") || of_device_is_compatible(np, "fsl,sec4.0-job-ring")) { ctrlpriv->jrpdev[ring] = of_platform_device_create(np, NULL, dev); which sets OF_POPULATED on the device node, but then it cleans these up: /* Remove platform devices for JobRs */ for (ring = 0; ring < ctrlpriv->total_jobrs; ring++) { if (ctrlpriv->jrpdev[ring]) of_device_unregister(ctrlpriv->jrpdev[ring]); } which leaves OF_POPULATED set. Arrange for platform devices with a device node to clear the OF_POPULATED bit when they are released. Signed-off-by: Russell King <rmk+kernel-I+IVW8TIWO2tmTQ+vhA3Yw@public.gmane.org> --- Please check this carefully - it may have issues where an of_node pointer is copied from one platform device to another, but IMHO doing that is itself buggy behaviour. include/linux/of_device.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/linux/of_device.h b/include/linux/of_device.h index cc7dd687a89d..7a8362d0c6d2 100644 --- a/include/linux/of_device.h +++ b/include/linux/of_device.h @@ -43,6 +43,7 @@ extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env static inline void of_device_node_put(struct device *dev) { + of_node_clear_flag(dev->of_node, OF_POPULATED); of_node_put(dev->of_node); } -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-04-05 17:09 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-09 9:33 [PATCH] OF: mark released devices as no longer populated Russell King 2016-08-09 9:33 ` Russell King [not found] ` <E1bX3Pw-00088i-KU-eh5Bv4kxaXIk46pC+1QYvQNdhmdF6hFW@public.gmane.org> 2016-08-09 16:48 ` Rob Herring 2016-08-09 16:48 ` Rob Herring [not found] ` <CAL_Jsq+pDeL1_=p06ynrV4CK+h8-Yx_KDbH-W=d-aSdARe3ZHw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2017-03-31 10:39 ` Russell King - ARM Linux 2017-03-31 10:39 ` Russell King - ARM Linux 2017-03-31 15:23 ` Horia Geantă 2017-03-31 15:23 ` Horia Geantă [not found] ` <VI1PR0401MB2591E5FD962023914F042C9B98370-9IDQY6o3qQhGNIhRVge97I3W/0Ik+aLCnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org> 2017-03-31 16:59 ` Rob Herring 2017-03-31 16:59 ` Rob Herring 2017-04-03 15:12 ` [PATCH] crypto: caam - fix JR platform device subsequent (re)creations Horia Geantă 2017-04-03 15:52 ` Rob Herring 2017-04-05 14:08 ` Herbert Xu 2017-04-05 17:09 ` Horia Geantă -- strict thread matches above, loose matches on Subject: below -- 2016-08-09 9:26 [PATCH] OF: mark released devices as no longer populated Russell King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.