Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

[Sergei Zviagintsev <sergei@s15v.net>]

Hi,

On Mon, Jun 29, 2015 at 06:44:23PM +0200, Dr. H. Nikolaus Schaller wrote:
[...]
> >> +	list_for_each_entry(uart, &uart_list, head) {
> >> +		if (node != uart->dev->of_node)
> >> +			continue;
> >> +
> >> +		return uart;
> > 
> > We can easily save three lines here :)
> 
> Hm. We have copied from here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
> 
> So please let us know how you want to save 3 lines.

Sorry for not being specific, I just meant that it could be written as

if (node == uart->dev->of_node)
	return uart;

> >> +/**
> >> + * devm_serial_get_uart_by_phandle - find the uart by phandle
> >> + * @dev - device that requests this uart
> >> + * @phandle - name of the property holding the uart phandle value
> >> + * @index - the index of the uart
> >> + *
> >> + * Returns the uart_port associated with the given phandle value,
> >> + * after getting a refcount to it, -ENODEV if there is no such uart or
> >> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
> >> + * not yet loaded. While at that, it also associates the device with
> >> + * the uart using devres. On driver detach, release function is invoked
> >> + * on the devres data, then, devres data is freed.
> > 
> > Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
> 
> Well, if the device is not loaded it means the caller must return -EPROBE_DEFER
> anyways since it can’t complete it’s probe function.

That was my mistake, from the first sight I hadn't found where
-EPROBE_DEFER is actually returned from the code and thus decided that
kernel-doc has an error. But now I see it, thanks.

> >> +
> >> +	*ptr = uart;
> >> +	devres_add(dev, ptr);
> > 
> > What is the point of assigning value to *ptr?
> 
> Good question. I think it is necessary to store a copy of the found uart/phy instead of a reference.
> Therefore the assignment to *ptr copies into the new memory area allocated above by 
> 
> ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
> 
> This makes the dev the owner of the data - instead of unknown ownership before.
> 
> It is the same as here (but it might be wrong/unnecessary there as well):
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n209
> 
> Maybe it has something to do with the unfinished devm_serial_uart_release().

Indeed. I haven't noticed this through the first quick look into the
code. Thank you for explanation.