I would like some feedback on the following regarding some
form of standardising return codes from a device driver probe
to try and stop some basic mistakes.
This document is not complete, any additions would be welcone.
probe() error return codes
==========================
-ENODEV This should be reserved for the case where there
really is no device here.
If you do not have a necessary resource or other setup
information this is NOT the error you want to return as
the driver core will not print any error message to the
user. Even if you driver prints a warning, this is still
not the error code to return.
-ENXIO See -ENODEV, this is taken by the driver core to mean
there is "No such device or address (POSIX.1)".
-ENOMEM This is used to signify lack of memory resources, such
as a failure to kmalloc() device state.
-EBUSY A resource you need is not avaialable.
This is returned if you cannot get exclusive access to a
resource such as a request_mem_region() has failed.
-EINVAL An argument to the driver is invalid.
-EIO An input/output error occured. This could be due to
the device responding in an unexpected way or that
the device did not complete a request properly.
Generic driver examples
=======================
Requesting an memory region
---------------------------
if (!request_mem_region(start, len, why))
return -EBUSY;
Mapping a IO region
-------------------
regs = ioremap(base, size);
if (!regs)
return -EIO;
possibly -EFAULT here?
Not -ENOMEM, which a number of drivers do.
Platform driver specific examples
=================================
Examples when writing platform drivers.
All examples in this section assume the probe prototype is:
static int my_probe(struct platform_device *pdev)
Checking platform data
----------------------
if (!pdev->dev.platform_data)
return -EINVAL;
should -ENOENT be returned here?
Finding platform resource(s)
----------------------------
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return -ENOENT;
Getting a clock
---------------
clk = clk_get(&pdev->dev, NULL);
if (IS_ERR(clk))
return PTR_ERR(clk);
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
On Tue, Dec 16, 2008 at 11:41:28PM -0800, Andrew Morton wrote:
> On Tue, 16 Dec 2008 21:53:31 +0000 Ben Dooks <[email protected]> wrote:
>
> > I would like some feedback on the following regarding some
> > form of standardising return codes from a device driver probe
> > to try and stop some basic mistakes.
> >
> > This document is not complete, any additions would be welcone.
Hm, shouldn't you have at least copied me on this?
What is this for? Each of the different busses treat return codes for
their probe functions a bit differently, are you wanting to unify them?
And if so, why?
thanks,
greg k-h
On Thu, Dec 18, 2008 at 09:14:36PM -0800, Greg KH wrote:
> On Tue, Dec 16, 2008 at 11:41:28PM -0800, Andrew Morton wrote:
> > On Tue, 16 Dec 2008 21:53:31 +0000 Ben Dooks <[email protected]> wrote:
> >
> > > I would like some feedback on the following regarding some
> > > form of standardising return codes from a device driver probe
> > > to try and stop some basic mistakes.
> > >
> > > This document is not complete, any additions would be welcone.
>
> Hm, shouldn't you have at least copied me on this?
Sorry, assumed you'd be reading linux-kernel.
> What is this for? Each of the different busses treat return codes for
> their probe functions a bit differently, are you wanting to unify them?
> And if so, why?
I was trying to make a guide for people to try and avoid the general
mistakes such as returning -ENODEV when it clearly isn't the right
thing to do. There are a number of drivers which return this causing
confusion as to why devices are not being bound as they neither print
an error nor cause the driver core to print anything [1].
The idea is to provide a guide to what error numbers are acceptable
to return and what the best return code for the common situations
that drivers tend to do and what to avoid.
As a note, having looked at the base driver, pci, platform and i2c
they all pass the error straight back to the core driver probe.
[1] There is a case to be put that drivers such as the i2c bus where
the machine specific support has declared a device to be present
to report an error if the probe then returns an -ENODEV or -ENXIO
to say the device is not there.
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
On Thu, Dec 18, 2008 at 10:14:26AM +0000, Ben Dooks wrote:
> On Tue, Dec 16, 2008 at 11:41:28PM -0800, Andrew Morton wrote:
> > On Tue, 16 Dec 2008 21:53:31 +0000 Ben Dooks <[email protected]> wrote:
> >
> > > I would like some feedback on the following regarding some
> > > form of standardising return codes from a device driver probe
> > > to try and stop some basic mistakes.
> > >
> > > This document is not complete, any additions would be welcone.
> > >
> > >
> > > probe() error return codes
> > > ==========================
> > >
> > > -ENODEV This should be reserved for the case where there
> > > really is no device here.
> > >
> > > If you do not have a necessary resource or other setup
> > > information this is NOT the error you want to return as
> > > the driver core will not print any error message to the
> > > user. Even if you driver prints a warning, this is still
> > > not the error code to return.
> > >
> > > -ENXIO See -ENODEV, this is taken by the driver core to mean
> > > there is "No such device or address (POSIX.1)".
> > >
> > > -ENOMEM This is used to signify lack of memory resources, such
> > > as a failure to kmalloc() device state.
> > >
> > > -EBUSY A resource you need is not avaialable.
> > >
> > > This is returned if you cannot get exclusive access to a
> > > resource such as a request_mem_region() has failed.
> > >
> > > -EINVAL An argument to the driver is invalid.
> > >
> > > -EIO An input/output error occured. This could be due to
> > > the device responding in an unexpected way or that
> > > the device did not complete a request properly.
> > >
> > >
> >
> > Sounds good. I forsee a lot of patches to fix all this up :)
> >
> > >
> > > Generic driver examples
> > > =======================
> > >
> > > Requesting an memory region
> > > ---------------------------
> > >
> > > if (!request_mem_region(start, len, why))
> > > return -EBUSY;
> > >
> > > Mapping a IO region
> > > -------------------
> > >
> > > regs = ioremap(base, size);
> > > if (!regs)
> > > return -EIO;
> > >
> > > possibly -EFAULT here?
> >
> > I don't think EFAULT is appropriate here - there was no fault.
> >
> > And EIO to me implies some problem with a hardware device.
> >
> > > Not -ENOMEM, which a number of drivers do.
> >
> > And ENOMEM should be reserved for page allocation exhaustion.
> >
> > It's a bit presumptuous, but perhaps EBUSY here?
>
> Ok, that might be ok, but it will get overloaded as -EBUSY is
> a possible return from a number of resource reservation calls.
As a note, Russell has said that -ENOMEM should be more than acceptable
here, as generally a failure from ioremap() is due to there being
insufficient memory to create the necessary page mappings.
> > >
> > > Platform driver specific examples
> > > =================================
> > >
> > > Examples when writing platform drivers.
> > >
> > > All examples in this section assume the probe prototype is:
> > >
> > > static int my_probe(struct platform_device *pdev)
> > >
> > >
> > > Checking platform data
> > > ----------------------
> > >
> > > if (!pdev->dev.platform_data)
> > > return -EINVAL;
> > >
> > > should -ENOENT be returned here?
> >
> > Well.. what would cause this to come about?
> >
> > Perhaps BUG() is more appropriate ;)
>
> WARN_ON() possibly, BUG() is a bit terminal to the whole kernel
> process and if triggered before the console is working can be
> rather hard to detect.
>
> Possibly change this to:
>
> if (!pdev->dev.platform_data) {
> dev_dbg(&pdev->dev, "no platform data supplied\n");
> WARN();
> return -EINVAL;
> }
>
> > >
> > > Finding platform resource(s)
> > > ----------------------------
> > >
> > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > if (!res)
> > > return -ENOENT;
> >
> > I don't really like the practice of returning what are clearly
> > filesystem-related errnos just because they're kinda sorta conceptually
> > similar to what actually happened.
> >
> > Again, what could actually cause this to happen? The device doesn't
> > have any memory resources? So it's busted? EIO or ENODEV?
>
> ENODEV is part of the problem that we're trying to avoid returning
> as it gets silently discarded by the device core. Unfortunately without
> creating a new set of error codes specifically for device probes then
> we're out of choice.
>
> > >
> > > Getting a clock
> > > ---------------
> > >
> > > clk = clk_get(&pdev->dev, NULL);
> > > if (IS_ERR(clk))
> > > return PTR_ERR(clk);
> >
> > yup.
> >
> >
> > But really, the drivers should be doing far less of this
> > errno-guessing. In the majority of cases (platform_get_resource,
> > ioremap, request_mem_region from your examples) the core kernel
> > function should have returned an IS_ERR value and all the driver needs
> > to do is to return it.
>
> Yes, although the work to fixup some of these might be large it certainly
> shouldn't be too complicated to do.
>
> --
> Ben ([email protected], http://www.fluff.org/)
>
> 'a smiley only costs 4 bytes'
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'
On Fri, Dec 19, 2008 at 08:16:36AM +0000, Ben Dooks wrote:
> On Thu, Dec 18, 2008 at 09:14:36PM -0800, Greg KH wrote:
> > On Tue, Dec 16, 2008 at 11:41:28PM -0800, Andrew Morton wrote:
> > > On Tue, 16 Dec 2008 21:53:31 +0000 Ben Dooks <[email protected]> wrote:
> > >
> > > > I would like some feedback on the following regarding some
> > > > form of standardising return codes from a device driver probe
> > > > to try and stop some basic mistakes.
> > > >
> > > > This document is not complete, any additions would be welcone.
> >
> > Hm, shouldn't you have at least copied me on this?
>
> Sorry, assumed you'd be reading linux-kernel.
I try to, but things get through at at times. Please always cc: me if
you want me to read it.
> > What is this for? Each of the different busses treat return codes for
> > their probe functions a bit differently, are you wanting to unify them?
> > And if so, why?
>
> I was trying to make a guide for people to try and avoid the general
> mistakes such as returning -ENODEV when it clearly isn't the right
> thing to do. There are a number of drivers which return this causing
> confusion as to why devices are not being bound as they neither print
> an error nor cause the driver core to print anything [1].
>
> The idea is to provide a guide to what error numbers are acceptable
> to return and what the best return code for the common situations
> that drivers tend to do and what to avoid.
>
> As a note, having looked at the base driver, pci, platform and i2c
> they all pass the error straight back to the core driver probe.
Fair enough, care to respin this and send it out to me for review?
thanks,
greg k-h
On Tue, 16 Dec 2008 21:53:31 +0000 Ben Dooks <[email protected]> wrote:
> I would like some feedback on the following regarding some
> form of standardising return codes from a device driver probe
> to try and stop some basic mistakes.
>
> This document is not complete, any additions would be welcone.
>
>
> probe() error return codes
> ==========================
>
> -ENODEV This should be reserved for the case where there
> really is no device here.
>
> If you do not have a necessary resource or other setup
> information this is NOT the error you want to return as
> the driver core will not print any error message to the
> user. Even if you driver prints a warning, this is still
> not the error code to return.
>
> -ENXIO See -ENODEV, this is taken by the driver core to mean
> there is "No such device or address (POSIX.1)".
>
> -ENOMEM This is used to signify lack of memory resources, such
> as a failure to kmalloc() device state.
>
> -EBUSY A resource you need is not avaialable.
>
> This is returned if you cannot get exclusive access to a
> resource such as a request_mem_region() has failed.
>
> -EINVAL An argument to the driver is invalid.
>
> -EIO An input/output error occured. This could be due to
> the device responding in an unexpected way or that
> the device did not complete a request properly.
>
>
Sounds good. I forsee a lot of patches to fix all this up :)
>
> Generic driver examples
> =======================
>
> Requesting an memory region
> ---------------------------
>
> if (!request_mem_region(start, len, why))
> return -EBUSY;
>
> Mapping a IO region
> -------------------
>
> regs = ioremap(base, size);
> if (!regs)
> return -EIO;
>
> possibly -EFAULT here?
I don't think EFAULT is appropriate here - there was no fault.
And EIO to me implies some problem with a hardware device.
> Not -ENOMEM, which a number of drivers do.
And ENOMEM should be reserved for page allocation exhaustion.
It's a bit presumptuous, but perhaps EBUSY here?
>
> Platform driver specific examples
> =================================
>
> Examples when writing platform drivers.
>
> All examples in this section assume the probe prototype is:
>
> static int my_probe(struct platform_device *pdev)
>
>
> Checking platform data
> ----------------------
>
> if (!pdev->dev.platform_data)
> return -EINVAL;
>
> should -ENOENT be returned here?
Well.. what would cause this to come about?
Perhaps BUG() is more appropriate ;)
>
> Finding platform resource(s)
> ----------------------------
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res)
> return -ENOENT;
I don't really like the practice of returning what are clearly
filesystem-related errnos just because they're kinda sorta conceptually
similar to what actually happened.
Again, what could actually cause this to happen? The device doesn't
have any memory resources? So it's busted? EIO or ENODEV?
>
> Getting a clock
> ---------------
>
> clk = clk_get(&pdev->dev, NULL);
> if (IS_ERR(clk))
> return PTR_ERR(clk);
yup.
But really, the drivers should be doing far less of this
errno-guessing. In the majority of cases (platform_get_resource,
ioremap, request_mem_region from your examples) the core kernel
function should have returned an IS_ERR value and all the driver needs
to do is to return it.
But we screwed that up again and again.
On Tue, Dec 16, 2008 at 11:41:28PM -0800, Andrew Morton wrote:
> On Tue, 16 Dec 2008 21:53:31 +0000 Ben Dooks <[email protected]> wrote:
>
> > I would like some feedback on the following regarding some
> > form of standardising return codes from a device driver probe
> > to try and stop some basic mistakes.
> >
> > This document is not complete, any additions would be welcone.
> >
> >
> > probe() error return codes
> > ==========================
> >
> > -ENODEV This should be reserved for the case where there
> > really is no device here.
> >
> > If you do not have a necessary resource or other setup
> > information this is NOT the error you want to return as
> > the driver core will not print any error message to the
> > user. Even if you driver prints a warning, this is still
> > not the error code to return.
> >
> > -ENXIO See -ENODEV, this is taken by the driver core to mean
> > there is "No such device or address (POSIX.1)".
> >
> > -ENOMEM This is used to signify lack of memory resources, such
> > as a failure to kmalloc() device state.
> >
> > -EBUSY A resource you need is not avaialable.
> >
> > This is returned if you cannot get exclusive access to a
> > resource such as a request_mem_region() has failed.
> >
> > -EINVAL An argument to the driver is invalid.
> >
> > -EIO An input/output error occured. This could be due to
> > the device responding in an unexpected way or that
> > the device did not complete a request properly.
> >
> >
>
> Sounds good. I forsee a lot of patches to fix all this up :)
>
> >
> > Generic driver examples
> > =======================
> >
> > Requesting an memory region
> > ---------------------------
> >
> > if (!request_mem_region(start, len, why))
> > return -EBUSY;
> >
> > Mapping a IO region
> > -------------------
> >
> > regs = ioremap(base, size);
> > if (!regs)
> > return -EIO;
> >
> > possibly -EFAULT here?
>
> I don't think EFAULT is appropriate here - there was no fault.
>
> And EIO to me implies some problem with a hardware device.
>
> > Not -ENOMEM, which a number of drivers do.
>
> And ENOMEM should be reserved for page allocation exhaustion.
>
> It's a bit presumptuous, but perhaps EBUSY here?
Ok, that might be ok, but it will get overloaded as -EBUSY is
a possible return from a number of resource reservation calls.
> >
> > Platform driver specific examples
> > =================================
> >
> > Examples when writing platform drivers.
> >
> > All examples in this section assume the probe prototype is:
> >
> > static int my_probe(struct platform_device *pdev)
> >
> >
> > Checking platform data
> > ----------------------
> >
> > if (!pdev->dev.platform_data)
> > return -EINVAL;
> >
> > should -ENOENT be returned here?
>
> Well.. what would cause this to come about?
>
> Perhaps BUG() is more appropriate ;)
WARN_ON() possibly, BUG() is a bit terminal to the whole kernel
process and if triggered before the console is working can be
rather hard to detect.
Possibly change this to:
if (!pdev->dev.platform_data) {
dev_dbg(&pdev->dev, "no platform data supplied\n");
WARN();
return -EINVAL;
}
> >
> > Finding platform resource(s)
> > ----------------------------
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res)
> > return -ENOENT;
>
> I don't really like the practice of returning what are clearly
> filesystem-related errnos just because they're kinda sorta conceptually
> similar to what actually happened.
>
> Again, what could actually cause this to happen? The device doesn't
> have any memory resources? So it's busted? EIO or ENODEV?
ENODEV is part of the problem that we're trying to avoid returning
as it gets silently discarded by the device core. Unfortunately without
creating a new set of error codes specifically for device probes then
we're out of choice.
> >
> > Getting a clock
> > ---------------
> >
> > clk = clk_get(&pdev->dev, NULL);
> > if (IS_ERR(clk))
> > return PTR_ERR(clk);
>
> yup.
>
>
> But really, the drivers should be doing far less of this
> errno-guessing. In the majority of cases (platform_get_resource,
> ioremap, request_mem_region from your examples) the core kernel
> function should have returned an IS_ERR value and all the driver needs
> to do is to return it.
Yes, although the work to fixup some of these might be large it certainly
shouldn't be too complicated to do.
--
Ben ([email protected], http://www.fluff.org/)
'a smiley only costs 4 bytes'