Message-ID: <20081219081739.GL12431@fluff.org.uk>
Date:	Fri, 19 Dec 2008 08:17:39 +0000
From:	Ben Dooks <ben-linux@...ff.org>
To:	Ben Dooks <ben-linux@...ff.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: device driver probe return codes

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 <ben-linux@...ff.org> 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 (ben@...ff.org, 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 majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Ben (ben@...ff.org, 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists