drivers/base/core.c: about device_find_child() function

drivers/base/core.c: about device_find_child() function

[Federico Vaga]

Hello,

I'm using the function device_find_child() [drivers/base/core.c] to retrieve 
a specific child of a device. I see that this function invokes 
get_device(child) when a child matches. I think that this function must 
return the reference to the child device without getting it.

The function's comment does not explicitly talk about an increment of the 
refcount of the device. So, "man 9 device_find_child" and various derivative 
webpages do not talk about this. The developer is not correctly informed 
about this function, unless (s)he looks at the source code.

I see that users of this function, usually, immediately do put_device() after 
the call to device_find_child(), so it is not expected that a 
device_find_child() does a get_device() on the found child.


   Immediately does put_device():
     drivers/firewire/core-device.c
     drivers/rpmsg/virtio_rpmsg_bus.c
     drivers/s390/kvm/kvm_virtio.c

   They effectively need a get_device():
     drivers/net/bluetooth/hci_sysfs.c
     drivers/net/dsa/dsa.c

   Maybe bugged because they do not do put_device():
     drivers/media/platform/s5p-mfc/s5p_mfc.c
     drivers/tty/serial/serial_core.c
   Probably I'm wrong on this and I do not find the associated put_device()


I should propose the following solution:

* Deprecate the device_find_child() function

* Create the following functions

   struct device *device_search_child(struct device *parent, void *data,
            int (*match)(struct device *dev, void *data))
   {
        struct klist_iter i;
        struct device *child;

        if (!parent)
                return NULL;

        klist_iter_init(&parent->p->klist_children, &i);
        while ((child = next_device(&i)))
                if (match(child, data))
                        break;
        klist_iter_exit(&i);
        return child;
  }

  struct device *get_device_child(struct device *parent, void *data,
            int (*match)(struct device *dev, void *data))
  {
        struct device *child;

        child = device_search_child(parent, data, match);
        if (child)
              get_device(child);
        return child;
  }


In this way, when a driver needs to find and get a child, it uses 
get_device_child() and , when it finishes its duty, it uses put_device(). In 
this situation, the developer use a pair of function with a symmetric names: 
get_device_child() and put_device().

If the driver do not need to get_device() on a child device, it simply does a 
device_search_child() to retrieve a pointer.

-- 
Federico Vaga

Re: drivers/base/core.c: about device_find_child() function

[Greg Kroah-Hartman]

On Thu, Apr 11, 2013 at 01:52:36PM +0200, Federico Vaga wrote:
> Hello,
> 
> I'm using the function device_find_child() [drivers/base/core.c] to retrieve 
> a specific child of a device. I see that this function invokes 
> get_device(child) when a child matches. I think that this function must 
> return the reference to the child device without getting it.

No, it should not, otherwise the device could "disappear" at any moment,
and the caller who had the handle, would now have a stale pointer.

> The function's comment does not explicitly talk about an increment of the 
> refcount of the device. So, "man 9 device_find_child" and various derivative 
> webpages do not talk about this. The developer is not correctly informed 
> about this function, unless (s)he looks at the source code.

You are right, I would gladly take a patch adding that comment to the
function, can you send me one?

> I see that users of this function, usually, immediately do put_device() after 
> the call to device_find_child(), so it is not expected that a 
> device_find_child() does a get_device() on the found child.
> 
> 
>    Immediately does put_device():
>      drivers/firewire/core-device.c
>      drivers/rpmsg/virtio_rpmsg_bus.c
>      drivers/s390/kvm/kvm_virtio.c

That's correct.

>    They effectively need a get_device():
>      drivers/net/bluetooth/hci_sysfs.c
>      drivers/net/dsa/dsa.c

Please fix these.

>    Maybe bugged because they do not do put_device():
>      drivers/media/platform/s5p-mfc/s5p_mfc.c
>      drivers/tty/serial/serial_core.c
>    Probably I'm wrong on this and I do not find the associated put_device()

I think the serial core is correct, but I'll audit it, the media one
should be fixed as well.

thanks,

greg k-h

Re: drivers/base/core.c: about device_find_child() function

[Federico Vaga]

On Thursday 11 April 2013 06:48:44 Greg Kroah-Hartman wrote:
> On Thu, Apr 11, 2013 at 01:52:36PM +0200, Federico Vaga wrote:
> > Hello,
> > 
> > I'm using the function device_find_child() [drivers/base/core.c] to
> > retrieve a specific child of a device. I see that this function invokes
> > get_device(child) when a child matches. I think that this function must
> > return the reference to the child device without getting it.
> 
> No, it should not, otherwise the device could "disappear" at any moment,
> and the caller who had the handle, would now have a stale pointer.

I know, I am aware of this, but sometimes it *seems* that it does not
matter. (argument later [**])

> > The function's comment does not explicitly talk about an increment of the
> > refcount of the device. So, "man 9 device_find_child" and various
> > derivative webpages do not talk about this. The developer is not
> > correctly informed about this function, unless (s)he looks at the source
> > code.
> 
> You are right, I would gladly take a patch adding that comment to the
> function, can you send me one?

Already sent.

> > I see that users of this function, usually, immediately do put_device()
> > after the call to device_find_child(), so it is not expected that a
> > device_find_child() does a get_device() on the found child.
> > 
> >    Immediately does put_device():
> >      drivers/firewire/core-device.c
> >      drivers/rpmsg/virtio_rpmsg_bus.c
> >      drivers/s390/kvm/kvm_virtio.c
> 
> That's correct.

[**] (argumentation based, obviously, on my limited understanding)

These drivers work like this:

	child = device_find_child(parent, data, match_function);
	if (child) {
		put_device(child);		
		<do something unrelated with child> 
	}

In these cases we do not need to get_device(). But we need to know if there 
is a child that match a rule. It can also "disapper" after the
put_device(child) but the driver continues on its way because it does not
use the child. For example virtio_rpmsg_bus.c:

	/* make sure a similar channel doesn't already exist */
	tmp = device_find_child(dev, chinfo, rpmsg_channel_match);
	if (tmp) {
		/* decrement the matched device's refcount back */
		put_device(tmp);
		dev_err(dev, "channel %s:%x:%x already exist\n",
				chinfo->name, chinfo->src, chinfo->dst);
		return NULL;
	}

In this case, it does not matter to get_device(), the driver is interested
only on the child existence, it does not use the child.
Maybe it is wrong a driver that works like this. I mean, why retrieve a
child if you do not want to use it? This can be implemented also with
the function device_for_each_child() and return an error if a channel already
exist (-EBUSY).

The same argumentation for firewire/core-device.c:

	revived_dev = device_find_child(card->device,
					device, lookup_existing_device);
	if (revived_dev) {
		put_device(revived_dev);
		fw_device_release(&device->device);

		return;
	}

This can be done with device_for_each_child() because it does not use the
the retrieved device. The function fw_device_release() can be done in the
function lookup_existing_device() when it finds the child. 

Also the driver s390/kvm/kvm_virtio.c:

		/* device already exists */
		dev = device_find_child(kvm_root, d, match_desc);
		if (dev) {
			/* XXX check for hotplug remove */
			put_device(dev);
			continue;
		}

Probably here, in a future patch XXX will be replaced with some code,
so it is correct to use device_find_child().

Now I'm thinking that device_for_each_child() is better in the above
cases. Am I wrong? Am I missing something? Which is the cleaner solution?


Anyway, my suggestion was more about the function name rather than its 
content (again, I am aware that the refcount must be increased if I
work on the retrieved child). Because the verb 'find' does not imply the
verb 'get', so, a function named device_find_child() should not do
get_device() because it may not obvious for the reader. I think that
the name should be something like get_device_child(), get_child_device(),
get_child(), and for symmetry the user will do put_device() on the
retrived child. But I understand that for legacy reason it could be
better to continue use device_find_child()


> >    Maybe bugged because they do not do put_device():
> >      drivers/media/platform/s5p-mfc/s5p_mfc.c

Fixed with commit 6e83e6e25eb49dc57a69b3f8ecc1e764c9775101. I did not see
it before, sorry.

> >      drivers/tty/serial/serial_core.c
> >    Probably I'm wrong on this and I do not find the associated 
put_device()
>
> I think the serial core is correct, but I'll audit it, the media one
> should be fixed as well.

I did not study serial_core, I was looking only for device_find_child().
Probably I'm missing something. Anyway, here what does not convice me:

(line number on next-20130412)
serial_core.c:2003
	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
	if (!uport->suspended && device_may_wakeup(tty_dev)) {
		if (uport->irq_wake) {
			disable_irq_wake(uport->irq);
			uport->irq_wake = 0;
		}
+               put_device(tty_dev);
		mutex_unlock(&port->mutex);
		return 0;
	}
+	put_device(tty_dev);
	uport->suspended = 0; 

serial_core:1936
	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
	if (device_may_wakeup(tty_dev)) {
		if (!enable_irq_wake(uport->irq))
			uport->irq_wake = 1;
		put_device(tty_dev);
		mutex_unlock(&port->mutex);
		return 0;
	}
+	put_device(tty_dev);


> >    They effectively need a get_device():
> >      drivers/net/bluetooth/hci_sysfs.c
> >      drivers/net/dsa/dsa.c
> 
> Please fix these.

Misunderstood. They are correct. I meant: in these cases is useful to have 
get_device() inside device_find_child() because they need to do something on 
child device before putting it

	child = device_find_child(parent, data, match_function);
	if (child) {
		<do something on child> 
		put_device(child);		
	}



In the previous email I forgot the following caller of device_find_child().
It is too complex for me to understand everything in few minutes; I just
looked in the file and around the function occurence.

arch/sparc/kernel/vio.c:335

static void vio_remove(struct mdesc_handle *hp, u64 node)
{
	struct device *dev;

	dev = device_find_child(&root_vdev->dev, (void *) node,
				vio_md_node_match);
	if (dev) {
		printk(KERN_INFO "VIO: Removing device %s\n", dev_name(dev));
		device_unregister(dev);
+               put_device(dev);
	}
}

Otherwise the refcount of dev after this function will be 1, but I suppose
that the expected result is 0 (remove)



Thank you very much for your patience :)

-- 
Federico Vaga

Re: drivers/base/core.c: about device_find_child() function

[Greg Kroah-Hartman]

On Fri, Apr 12, 2013 at 02:09:20PM +0200, Federico Vaga wrote:
> On Thursday 11 April 2013 06:48:44 Greg Kroah-Hartman wrote:
> > On Thu, Apr 11, 2013 at 01:52:36PM +0200, Federico Vaga wrote:
> > > Hello,
> > > 
> > > I'm using the function device_find_child() [drivers/base/core.c] to
> > > retrieve a specific child of a device. I see that this function invokes
> > > get_device(child) when a child matches. I think that this function must
> > > return the reference to the child device without getting it.
> > 
> > No, it should not, otherwise the device could "disappear" at any moment,
> > and the caller who had the handle, would now have a stale pointer.
> 
> I know, I am aware of this, but sometimes it *seems* that it does not
> matter. (argument later [**])

It does matter, it's just that you are finding users that aren't really
caring about the device at all, just if it is present or not.

<snip>

> In this case, it does not matter to get_device(), the driver is interested
> only on the child existence, it does not use the child.
> Maybe it is wrong a driver that works like this. I mean, why retrieve a
> child if you do not want to use it? This can be implemented also with
> the function device_for_each_child() and return an error if a channel already
> exist (-EBUSY).

Yes, you could use device_for_each_child(), but some people find
device_find_child() easier to call, that's up to them.  It doesn't
really matter either way.

> Anyway, my suggestion was more about the function name rather than its 
> content (again, I am aware that the refcount must be increased if I
> work on the retrieved child). Because the verb 'find' does not imply the
> verb 'get', so, a function named device_find_child() should not do
> get_device() because it may not obvious for the reader. I think that
> the name should be something like get_device_child(), get_child_device(),
> get_child(), and for symmetry the user will do put_device() on the
> retrived child. But I understand that for legacy reason it could be
> better to continue use device_find_child()

All functions in the driver core that return a pointer to a reference
counted device, return with it incremented.  The "get" vs, "find" names
do not matter here.  Symmetry would have been nice, and I have taken
some heat about the naming of the driver core functions at times,
especially by people new to the code, but realize that this was created
10+ years ago, and has grown over time.  We didn't have any real help
way back then, so we have what we have.

Patches to try to make things "cleaner" are always appreciated, as long
as you aren't just gratuitously renaming functions.

> I did not study serial_core, I was looking only for device_find_child().
> Probably I'm missing something. Anyway, here what does not convice me:
> 
> (line number on next-20130412)
> serial_core.c:2003
> 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> 	if (!uport->suspended && device_may_wakeup(tty_dev)) {
> 		if (uport->irq_wake) {
> 			disable_irq_wake(uport->irq);
> 			uport->irq_wake = 0;
> 		}
> +               put_device(tty_dev);
> 		mutex_unlock(&port->mutex);
> 		return 0;
> 	}
> +	put_device(tty_dev);
> 	uport->suspended = 0; 
> 
> serial_core:1936
> 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> 	if (device_may_wakeup(tty_dev)) {
> 		if (!enable_irq_wake(uport->irq))
> 			uport->irq_wake = 1;
> 		put_device(tty_dev);
> 		mutex_unlock(&port->mutex);
> 		return 0;
> 	}
> +	put_device(tty_dev);

That looks like a good patch, care to properly send it, (after you test
it first of course), so that we can apply it?

thanks,

greg k-h

Re: drivers/base/core.c: about device_find_child() function

[Federico Vaga]

Thank you very much Greg
 
> > I did not study serial_core, I was looking only for device_find_child().
> > Probably I'm missing something. Anyway, here what does not convice me:
> > 
> > (line number on next-20130412)
> > serial_core.c:2003
> > 
> > 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> > 	if (!uport->suspended && device_may_wakeup(tty_dev)) {
> > 	
> > 		if (uport->irq_wake) {
> > 		
> > 			disable_irq_wake(uport->irq);
> > 			uport->irq_wake = 0;
> > 		
> > 		}
> > 
> > +               put_device(tty_dev);
> > 
> > 		mutex_unlock(&port->mutex);
> > 		return 0;
> > 	
> > 	}
> > 
> > +	put_device(tty_dev);
> > 
> > 	uport->suspended = 0;
> > 
> > serial_core:1936
> > 
> > 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
> > 	if (device_may_wakeup(tty_dev)) {
> > 	
> > 		if (!enable_irq_wake(uport->irq))
> > 		
> > 			uport->irq_wake = 1;
> > 		
> > 		put_device(tty_dev);
> > 		mutex_unlock(&port->mutex);
> > 		return 0;
> > 	
> > 	}
> > 
> > +	put_device(tty_dev);
> 
> That looks like a good patch, care to properly send it, (after you test
> it first of course), so that we can apply it?

Yes, I can do it and test it. I hope to find the time for a test in these 
days.

-- 
Federico Vaga

Re: drivers/base/core.c: about device_find_child() function

[Lars-Peter Clausen]

On 04/12/2013 02:09 PM, Federico Vaga wrote:
> [...]
> 
> [**] (argumentation based, obviously, on my limited understanding)
> 
> These drivers work like this:
> 
> 	child = device_find_child(parent, data, match_function);
> 	if (child) {
> 		put_device(child);		
> 		<do something unrelated with child> 
> 	}
> 
> In these cases we do not need to get_device(). But we need to know if there 
> is a child that match a rule. It can also "disapper" after the
> put_device(child) but the driver continues on its way because it does not
> use the child. For example virtio_rpmsg_bus.c:
> 
> 	/* make sure a similar channel doesn't already exist */
> 	tmp = device_find_child(dev, chinfo, rpmsg_channel_match);
> 	if (tmp) {
> 		/* decrement the matched device's refcount back */
> 		put_device(tmp);
> 		dev_err(dev, "channel %s:%x:%x already exist\n",
> 				chinfo->name, chinfo->src, chinfo->dst);
> 		return NULL;
> 	}

Considering that there seems to be a common pattern here where the caller
only wants to know if the device exists, but is not really interested in the
device itself, how about adding a helper function for this?

- Lars

Re: drivers/base/core.c: about device_find_child() function

[Federico Vaga]

Hi Lars,
 
> Considering that there seems to be a common pattern here where the caller
> only wants to know if the device exists, but is not really interested in the
> device itself, how about adding a helper function for this?

It was my first thought when I opened this thread. But now I'm convinced that 
device_for_each_child() is the best choice (maybe I'm wrong).

device_for_each_child() allow you to perform an operation of each child of a 
device: look for a specific child, do something on every children, retrieve a 
specific group of children, etc.

I think that an helper for this case will be a perfect duplication of 
device_for_each_child() with a different name and comment (borrowed from 
device_find_child()). Maybe, a macro to assign a different name to this 
function:

/*
 * [...]
 * The callback should return 0 if the device doesn't match and non-zero
 * if it does
 * [...]
 */
#define device_has_child(parent, data, match) device_for_each_child(parent, 
data, match)

But, is it useful? It can be a suggestion to developers to prefer 
device_for_each_child() instead of device_find_child() when (s)he is 
interested only about the child existence. So, (s)he does not need to 
put_device(). But I think that is not a strong argumentation, and later in 
time someone will propose his own special use of device_for_each_child().

I think that device_for_each_child() is generic enough to cover this problem.

-- 
Federico Vaga