On 01/30/2015 09:08 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 07:03:03AM -0500, Peter Hurley wrote:
>> On 01/30/2015 05:18 AM, Thierry Reding wrote:
>>> -ENODEV is certainly not the correct return value if a resource is not
>>> available. It translates to "no such device", but the device must
>>> clearly be there, otherwise the ->probe() shouldn't have been called.
>>
>> -ENODEV is the appropriate return from the probe(); there is no device.
>
> No it is not. "no such device" means "the device is not present". If
> the device is not present, we wouldn't have a struct device associated
> with it.
>
> The missing resource is an error condition: what it's saying is that the
> device is there, but something has failed in providing the IO regions
> necessary to access it. That's really something very different from
> "there is no device present".
This is masking behavior changes behind what is essentially a refactor.
For example, here's Felipe's changelog to omap-serial:
commit d044d2356f8dd18c755e13f34318bc03ef9c6887
Author: Felipe Balbi <balbi@xxxxxx>
Date: Wed Apr 23 09:58:33 2014 -0500
tty: serial: omap: switch over to devm_ioremap_resource
just using helper function to remove some duplicated
code a bit. While at that, also move allocation of
struct uart_omap_port higher in the code so that
we return much earlier in case of no memory.
Signed-off-by: Felipe Balbi <balbi@xxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
No mention of correcting an error return value here.
Why change the error return from probe() now?
Before you say consistency, I think you should look at the stats below.
IOW, if you want to change the error code return from probe() for
consistency's sake, a tree-wide patch would be the appropriate way.
>>> Or if it really isn't there, then you'd at least need a memory region
>>> to probe, otherwise you can't determine whether it's there or not. So
>>> from the perspective of a device driver I think a missing, or NULL,
>>> resource, is indeed an "invalid argument".
>>
>> Trying to argue that a missing host bus window is an "invalid argument"
>> to probe() is just trying to make the shoe fit.
>
> As is arguing that -ENODEV is an appropriate return value for the missing
> resource.
>
> Moreover, returning -ENODEV is actually *bad* in this case - the kernel's
> generic probe handling does not report the failure of the driver to bind
> given this, so a missing resource potentially becomes a silent failure of
> a driver - leading users to wonder why their devices aren't found.
>
> If we /really/ have a problem with the error code, then why not invent
> a new error code to cater for this condition - maybe, EBADRES (bad resource).
>
>>> I understand that people might see some ambiguity there, and -EINVAL is
>>> certainly not a very accurate description, but such is the nature of
>>> error codes. You pick what fits best. -ENXIO and -EADDRNOTAVAIL had been
>>> under discussion as well if I remember correctly, but they both equally
>>> ambiguous. -ENODATA might have been a better fit, but that too has a
>>> different meaning in other contexts.
>>>
>>> Besides, there's an error message that goes along with the error code
>>> return, that should remove any ambiguity for people looking at dmesg.
>>>
>>> All of that said, the original assertion that the check is not required
>>> is still valid. We can argue at length about the specific error code but
>>> if we decide that it needs to change, then we should modify
>>> devm_ioremap_resource() rather than requiring all callers to perform the
>>> extra check again.
>>
>> None of the devm_ioremap_resource() changes have been submitted for
>> serial drivers.
>
> $ grep devm_ioremap_resource drivers/tty/serial/ -r | wc -l
> 10
Ok, not 'none' but hardly tree-wide.
And of those 10 drivers now using devm_ioremap_resource(),
3 drivers still return ENODEV for a missing resource [1]. (FWIW, I wrote 'none'
on the basis of a grep of devm_ioremap_resource and looking at the last one,
serial-tegra.c, which has exactly the construct objected to in the Spreadtrum
driver).
Another 9 drivers still use plain devm_ioremap(), even those with
trivial conversions like samsung.c [2]
Of the serial drivers which use platform_get_resource(),
10 return ENODEV
5 return EINVAL (not including those converted to devm_ioremap_resource())
4 return ENXIO
3 return ENOMEM
2 return ENOENT
1 returns EBUSY
1 returns EFAULT
So to recap, I see no reason to respin this driver submission when:
1. even drivers already using devm_ioremap_resource() aren't consistent
2. drivers which could trivially use devm_ioremap_resource, don't
3. there's no basis for requiring consistent return value _yet_
(or even what that value should be)
4. the platform_get_resource()/devm_ioremap_resource is an awkward code construct
Regards,
Peter Hurley
[1]
drivers/tty/serial/bcm63xx_uart.c- res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/tty/serial/bcm63xx_uart.c- if (!res_mem)
drivers/tty/serial/bcm63xx_uart.c- return -ENODEV;
drivers/tty/serial/bcm63xx_uart.c-
drivers/tty/serial/bcm63xx_uart.c- port->mapbase = res_mem->start;
drivers/tty/serial/bcm63xx_uart.c: port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
drivers/tty/serial/vt8500_serial.c- mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/tty/serial/vt8500_serial.c- irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
drivers/tty/serial/vt8500_serial.c- if (!mmres || !irqres)
drivers/tty/serial/vt8500_serial.c- return -ENODEV;
[...]
drivers/tty/serial/vt8500_serial.c: vt8500_port->uart.membase = devm_ioremap_resource(&pdev->dev, mmres);
drivers/tty/serial/serial-tegra.c- resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drivers/tty/serial/serial-tegra.c- if (!resource) {
drivers/tty/serial/serial-tegra.c- dev_err(&pdev->dev, "No IO memory resource\n");
drivers/tty/serial/serial-tegra.c- return -ENODEV;
drivers/tty/serial/serial-tegra.c- }
drivers/tty/serial/serial-tegra.c-
drivers/tty/serial/serial-tegra.c- u->mapbase = resource->start;
drivers/tty/serial/serial-tegra.c: u->membase = devm_ioremap_resource(&pdev->dev, resource);
[2]
drivers/tty/serial/samsung.c: res = platform_get_resource(platdev, IORESOURCE_MEM, 0);
drivers/tty/serial/samsung.c- if (res == NULL) {
drivers/tty/serial/samsung.c- dev_err(port->dev, "failed to find memory resource for uart\n");
drivers/tty/serial/samsung.c- return -EINVAL;
drivers/tty/serial/samsung.c- }
drivers/tty/serial/samsung.c-
drivers/tty/serial/samsung.c- dbg("resource %pR)\n", res);
drivers/tty/serial/samsung.c-
drivers/tty/serial/samsung.c- port->membase = devm_ioremap(port->dev, res->start, resource_size(res));
drivers/tty/serial/samsung.c- if (!port->membase) {
drivers/tty/serial/samsung.c- dev_err(port->dev, "failed to remap controller address\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
