Why is device_create_file __must_check?

Why is device_create_file __must_check?

[Paul Mackerras]

I am seeing a bunch of warnings about not checking the return value
from device_create_file() for code like this (from
arch/powerpc/kernel/pci_64.c):

static ssize_t pci_show_devspec(struct device *dev,
		struct device_attribute *attr, char *buf)
{
	struct pci_dev *pdev;
	struct device_node *np;

	pdev = to_pci_dev (dev);
	np = pci_device_to_OF_node(pdev);
	if (np == NULL || np->full_name == NULL)
		return 0;
	return sprintf(buf, "%s", np->full_name);
}
static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);

void pcibios_add_platform_entries(struct pci_dev *pdev)
{
	device_create_file(&pdev->dev, &dev_attr_devspec);
}

What bad thing could happen if device_create_file fails, other than
that the "devspec" file doesn't appear in sysfs?  I don't see how the
error could lead to any null pointer dereference later on or anything
like that.  If some bad thing could happen, how do I avert that?  If
nothing bad will happen, why does device_create_file have __must_check
on it?

Paul.

Re: Why is device_create_file __must_check?

[Andrew Morton]

On Tue, 10 Oct 2006 14:32:33 +1000
Paul Mackerras <paulus@samba.org> wrote:

> I am seeing a bunch of warnings about not checking the return value
> from device_create_file() for code like this (from
> arch/powerpc/kernel/pci_64.c):
> 
> static ssize_t pci_show_devspec(struct device *dev,
> 		struct device_attribute *attr, char *buf)
> {
> 	struct pci_dev *pdev;
> 	struct device_node *np;
> 
> 	pdev = to_pci_dev (dev);
> 	np = pci_device_to_OF_node(pdev);
> 	if (np == NULL || np->full_name == NULL)
> 		return 0;
> 	return sprintf(buf, "%s", np->full_name);
> }
> static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> 
> void pcibios_add_platform_entries(struct pci_dev *pdev)
> {
> 	device_create_file(&pdev->dev, &dev_attr_devspec);
> }
> 
> What bad thing could happen if device_create_file fails, other than
> that the "devspec" file doesn't appear in sysfs?

There are no super-strong reasons here, but if device_create_file() fails
then the required control files aren't there and the subsystem isn't
working as intended.  If it's in a module then we should fail the modprobe. 
If it's a bootup thing then best we can do is to panic.  Or at least log
the event.

The most common cause of this is a programming error: we tried to create
the same entry twice.   We want to know about that.

>  I don't see how the
> error could lead to any null pointer dereference later on or anything
> like that.  If some bad thing could happen, how do I avert that?  If
> nothing bad will happen, why does device_create_file have __must_check
> on it?

Because it can fail.  We need to take _some_ action if the setup failed - at
least report it so the user (and the kernel developers) know that something
is going wrong.

Re: Why is device_create_file __must_check?

[Greg KH]

On Mon, Oct 09, 2006 at 09:49:36PM -0700, Andrew Morton wrote:
> On Tue, 10 Oct 2006 14:32:33 +1000
> Paul Mackerras <paulus@samba.org> wrote:
> 
> > I am seeing a bunch of warnings about not checking the return value
> > from device_create_file() for code like this (from
> > arch/powerpc/kernel/pci_64.c):
> > 
> > static ssize_t pci_show_devspec(struct device *dev,
> > 		struct device_attribute *attr, char *buf)
> > {
> > 	struct pci_dev *pdev;
> > 	struct device_node *np;
> > 
> > 	pdev = to_pci_dev (dev);
> > 	np = pci_device_to_OF_node(pdev);
> > 	if (np == NULL || np->full_name == NULL)
> > 		return 0;
> > 	return sprintf(buf, "%s", np->full_name);
> > }
> > static DEVICE_ATTR(devspec, S_IRUGO, pci_show_devspec, NULL);
> > 
> > void pcibios_add_platform_entries(struct pci_dev *pdev)
> > {
> > 	device_create_file(&pdev->dev, &dev_attr_devspec);
> > }
> > 
> > What bad thing could happen if device_create_file fails, other than
> > that the "devspec" file doesn't appear in sysfs?
> 
> There are no super-strong reasons here, but if device_create_file() fails
> then the required control files aren't there and the subsystem isn't
> working as intended.  If it's in a module then we should fail the modprobe. 
> If it's a bootup thing then best we can do is to panic.  Or at least log
> the event.
> 
> The most common cause of this is a programming error: we tried to create
> the same entry twice.   We want to know about that.

Exactly, that's the most common cause here (different programmers trying
to create the same file in the same place.)  That needs to error out so
that it is caught properly.

thanks,

greg k-h

Re: Why is device_create_file __must_check?

[Paul Mackerras]

Andrew Morton writes:

> There are no super-strong reasons here, but if device_create_file() fails
> then the required control files aren't there and the subsystem isn't
> working as intended.  If it's in a module then we should fail the modprobe. 
> If it's a bootup thing then best we can do is to panic.  Or at least log
> the event.

In the case of the windfarm driver, the sysfs files are reporting
things like cpu voltage, current, temperature etc. which can be
interesting to know about, but the sysfs files are not essential to
the operation of the driver.  So just some cheesy printk would do in
that sort of situation, I guess.

> The most common cause of this is a programming error: we tried to create
> the same entry twice.   We want to know about that.

In that case a WARN_ON inside device_create_file when the duplicate is
detected would be better - less code, and only one place where the
check needs to be done.  The WARN_ON will give us a backtrace so we
can see where the second creation attempt happened.

> Because it can fail.  We need to take _some_ action if the setup failed - at
> least report it so the user (and the kernel developers) know that something
> is going wrong.

So we have to add printks in all sorts of places where the
device_create_file has never failed before.  If you're that concerned,
why not add a WARN_ON(error) in device_create_file() ?

Paul.

Re: Why is device_create_file __must_check?

[Andrew Morton]

On Tue, 10 Oct 2006 15:14:04 +1000
Paul Mackerras <paulus@samba.org> wrote:

> > Because it can fail.  We need to take _some_ action if the setup failed - at
> > least report it so the user (and the kernel developers) know that something
> > is going wrong.
> 
> So we have to add printks in all sorts of places where the
> device_create_file has never failed before.  If you're that concerned,

aren't you concerned too?

> why not add a WARN_ON(error) in device_create_file() ?

That might be suitable, yup.

Re: Why is device_create_file __must_check?

[Paul Mackerras]

Andrew Morton writes:

> > So we have to add printks in all sorts of places where the
> > device_create_file has never failed before.  If you're that concerned,
> 
> aren't you concerned too?

Not about the ones that have shown no sign of failing, no...

Most of the sites I have looked at have been cases where the kernel
genuinely doesn't care whether the device_create_file call succeeded
or failed.  Adding an if and printk in all these places seems like
pointless bloat when it could be done in one place - namely
device_create_file.  In one or two cases the return value from
device_create_file can be returned as its caller's return value, but
these were the minority.  In no cases that I have looked at was there
any other suitable action to take.

> > why not add a WARN_ON(error) in device_create_file() ?
> 
> That might be suitable, yup.

Greg claims that people ignore WARN_ON messages.  If that's true, I
fail to see how adding printks will help.

Paul.

Re: Why is device_create_file __must_check?

[Greg KH]

On Tue, Oct 10, 2006 at 03:14:04PM +1000, Paul Mackerras wrote:
> Andrew Morton writes:
> 
> > There are no super-strong reasons here, but if device_create_file() fails
> > then the required control files aren't there and the subsystem isn't
> > working as intended.  If it's in a module then we should fail the modprobe. 
> > If it's a bootup thing then best we can do is to panic.  Or at least log
> > the event.
> 
> In the case of the windfarm driver, the sysfs files are reporting
> things like cpu voltage, current, temperature etc. which can be
> interesting to know about, but the sysfs files are not essential to
> the operation of the driver.  So just some cheesy printk would do in
> that sort of situation, I guess.

Then add a cheesy printk into your driver then.  That's fine.

> > The most common cause of this is a programming error: we tried to create
> > the same entry twice.   We want to know about that.
> 
> In that case a WARN_ON inside device_create_file when the duplicate is
> detected would be better - less code, and only one place where the
> check needs to be done.  The WARN_ON will give us a backtrace so we
> can see where the second creation attempt happened.
> 
> > Because it can fail.  We need to take _some_ action if the setup failed - at
> > least report it so the user (and the kernel developers) know that something
> > is going wrong.
> 
> So we have to add printks in all sorts of places where the
> device_create_file has never failed before.  If you're that concerned,
> why not add a WARN_ON(error) in device_create_file() ?

Because it's kind of been proven that not even then are things fixed up
(by personal experience with people naming drivers the same thing, which
will cause an WARN_ON() in the kernel to happen, yet is ignored by the
developer.)

But I don't mind adding it there too, if that helps.  But again, even
then, don't ignore the return value, it's trying to tell you something.
If you have multiple sysfs files, just use an attribute group which will
be be unwound automatically if something goes wrong.

thanks,

greg k-h