device_for_each_child() before device_add() doesn't work

device_for_each_child() before device_add() doesn't work

[David Vrabel]

Calling device_for_each_child() on a device that has yet to be added
(with device_add()) used to work.  UWB subsystem currently tries to do this.

Since "driver core: move klist_children into private structure"[1] this
no longer works (an oops occurs in device_for_each_chid()).  Is it
something that ought to work? Or should the UWB subsystem be changed to
not do this?

David

[1] f791b8c836307b58cbf62133a6a772ed1a92fb33
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/

Re: device_for_each_child() before device_add() doesn't work

[Greg KH]

On Tue, Apr 14, 2009 at 05:54:40PM +0100, David Vrabel wrote:
> Calling device_for_each_child() on a device that has yet to be added
> (with device_add()) used to work.  UWB subsystem currently tries to do this.
> 
> Since "driver core: move klist_children into private structure"[1] this
> no longer works (an oops occurs in device_for_each_chid()).  Is it
> something that ought to work? Or should the UWB subsystem be changed to
> not do this?

Why would you call device_for_each_child() if you have never added it to
the bus?  How would you expect that to work, as there will never be any
children for it?

thanks,

greg k-h

Re: device_for_each_child() before device_add() doesn't work

[David Vrabel]

Greg KH wrote:
> On Tue, Apr 14, 2009 at 05:54:40PM +0100, David Vrabel wrote:
>> Calling device_for_each_child() on a device that has yet to be added
>> (with device_add()) used to work.  UWB subsystem currently tries to do this.
>>
>> Since "driver core: move klist_children into private structure"[1] this
>> no longer works (an oops occurs in device_for_each_chid()).  Is it
>> something that ought to work? Or should the UWB subsystem be changed to
>> not do this?
> 
> Why would you call device_for_each_child() if you have never added it to
> the bus?  How would you expect that to work, as there will never be any
> children for it?

The children of a UWB radio controller (RC) are it's neighbors.  When 
assigning a random DevAddr to an RC we need to check that the address is 
  unique within the beacon group.  We use device_for_each_child() here 
to check that the generated DevAddr is not used by any of its neighbors.

This address must be assigned during initialization and during normal 
operation and in the past we didn't need to special case the address 
assignment during initialization.

David

Re: device_for_each_child() before device_add() doesn't work

[Greg KH]

On Tue, Apr 14, 2009 at 07:16:31PM +0100, David Vrabel wrote:
> Greg KH wrote:
> > On Tue, Apr 14, 2009 at 05:54:40PM +0100, David Vrabel wrote:
> >> Calling device_for_each_child() on a device that has yet to be added
> >> (with device_add()) used to work.  UWB subsystem currently tries to do this.
> >>
> >> Since "driver core: move klist_children into private structure"[1] this
> >> no longer works (an oops occurs in device_for_each_chid()).  Is it
> >> something that ought to work? Or should the UWB subsystem be changed to
> >> not do this?
> > 
> > Why would you call device_for_each_child() if you have never added it to
> > the bus?  How would you expect that to work, as there will never be any
> > children for it?
> 
> The children of a UWB radio controller (RC) are it's neighbors.  When 
> assigning a random DevAddr to an RC we need to check that the address is 
>   unique within the beacon group.  We use device_for_each_child() here 
> to check that the generated DevAddr is not used by any of its neighbors.

So the problem is for the first device being added, right?  Can't you
just have a flag for this?

> This address must be assigned during initialization and during normal 
> operation and in the past we didn't need to special case the address 
> assignment during initialization.

Does the patch below fix the problem for you?

thanks,

greg k-h

---------------

 drivers/base/core.c |    3 +++
 1 file changed, 3 insertions(+)

--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1142,6 +1142,9 @@ int device_for_each_child(struct device 
 	struct device *child;
 	int error = 0;
 
+	if (!parent->p)
+		return 0;
+
 	klist_iter_init(&parent->p->klist_children, &i);
 	while ((child = next_device(&i)) && !error)
 		error = fn(child, data);

Re: device_for_each_child() before device_add() doesn't work

[David Vrabel]

Greg KH wrote:
> 
> So the problem is for the first device being added, right?  Can't you
> just have a flag for this?

It's trivial to fix in the UWB stack.

>> This address must be assigned during initialization and during normal 
>> operation and in the past we didn't need to special case the address 
>> assignment during initialization.
> 
> Does the patch below fix the problem for you?

Yes.

>  drivers/base/core.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1142,6 +1142,9 @@ int device_for_each_child(struct device 
>  	struct device *child;
>  	int error = 0;
>  
> +	if (!parent->p)
> +		return 0;
> +
>  	klist_iter_init(&parent->p->klist_children, &i);
>  	while ((child = next_device(&i)) && !error)
>  		error = fn(child, data);
> 
> 

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/

Re: device_for_each_child() before device_add() doesn't work

[Greg KH]

On Wed, Apr 15, 2009 at 03:51:03PM +0100, David Vrabel wrote:
> Greg KH wrote:
> > 
> > So the problem is for the first device being added, right?  Can't you
> > just have a flag for this?
> 
> It's trivial to fix in the UWB stack.

I'd recommend doing that :)

> >> This address must be assigned during initialization and during normal 
> >> operation and in the past we didn't need to special case the address 
> >> assignment during initialization.
> > 
> > Does the patch below fix the problem for you?
> 
> Yes.

Thanks for testing, I've now added the patch to my tree.

greg k-h