[PATCH] OF: mark released devices as no longer populated

[PATCH] OF: mark released devices as no longer populated

[Russell King]

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

[PATCH] OF: mark released devices as no longer populated

[Russell King]

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

Re: [PATCH] OF: mark released devices as no longer populated

[Rob Herring]

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

[PATCH] OF: mark released devices as no longer populated

[Rob Herring]

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

Re: [PATCH] OF: mark released devices as no longer populated

[Russell King - ARM Linux]

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

[PATCH] OF: mark released devices as no longer populated

[Russell King - ARM Linux]

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.

Re: [PATCH] OF: mark released devices as no longer populated

[Horia Geantă]

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

[PATCH] OF: mark released devices as no longer populated

[Horia Geantă]

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
> 

Re: [PATCH] OF: mark released devices as no longer populated

[Rob Herring]

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

[PATCH] OF: mark released devices as no longer populated

[Rob Herring]

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

[PATCH] crypto: caam - fix JR platform device subsequent (re)creations

[Horia Geantă]

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

Re: [PATCH] crypto: caam - fix JR platform device subsequent (re)creations

[Rob Herring]

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>

Re: [PATCH] crypto: caam - fix JR platform device subsequent (re)creations

[Herbert Xu]

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

Re: [PATCH] crypto: caam - fix JR platform device subsequent (re)creations

[Horia Geantă]

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