Check the return value of device_create_bin_file in pci_create_bus,
unwind if neccessary, and propogate any errors to the caller.
Signed-off-by: Simon Horman <[email protected]>
---
drivers/pci/probe.c: In function `pci_create_bus':
drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
# ia64-unknown-linux-gnu-gcc --version
ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c 2008-08-05 19:58:43.000000000 +1000
+++ linux-2.6/drivers/pci/probe.c 2008-08-05 19:59:15.000000000 +1000
@@ -53,26 +53,37 @@ EXPORT_SYMBOL(no_pci_devices);
* a per-bus basis. This routine creates the files and ties them into
* their associated read, write and mmap files from pci-sysfs.c
*/
-static void pci_create_legacy_files(struct pci_bus *b)
+static int pci_create_legacy_files(struct pci_bus *b)
{
+ int error;
+
b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
GFP_ATOMIC);
- if (b->legacy_io) {
- b->legacy_io->attr.name = "legacy_io";
- b->legacy_io->size = 0xffff;
- b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_io->read = pci_read_legacy_io;
- b->legacy_io->write = pci_write_legacy_io;
- device_create_bin_file(&b->dev, b->legacy_io);
-
- /* Allocated above after the legacy_io struct */
- b->legacy_mem = b->legacy_io + 1;
- b->legacy_mem->attr.name = "legacy_mem";
- b->legacy_mem->size = 1024*1024;
- b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_mem->mmap = pci_mmap_legacy_mem;
- device_create_bin_file(&b->dev, b->legacy_mem);
+ if (!b->legacy_io)
+ return -ENOMEM;
+
+ b->legacy_io->attr.name = "legacy_io";
+ b->legacy_io->size = 0xffff;
+ b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_io->read = pci_read_legacy_io;
+ b->legacy_io->write = pci_write_legacy_io;
+ error = device_create_bin_file(&b->dev, b->legacy_io);
+ if (error)
+ return error;
+
+ /* Allocated above after the legacy_io struct */
+ b->legacy_mem = b->legacy_io + 1;
+ b->legacy_mem->attr.name = "legacy_mem";
+ b->legacy_mem->size = 1024*1024;
+ b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_mem->mmap = pci_mmap_legacy_mem;
+ error = device_create_bin_file(&b->dev, b->legacy_mem);
+ if (error) {
+ device_remove_bin_file(&b->dev, b->legacy_io);
+ return error;
}
+
+ return 0;
}
void pci_remove_legacy_files(struct pci_bus *b)
@@ -84,7 +95,7 @@ void pci_remove_legacy_files(struct pci_
}
}
#else /* !HAVE_PCI_LEGACY */
-static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
+static inline int pci_create_legacy_files(struct pci_bus *bus) { return 0; }
void pci_remove_legacy_files(struct pci_bus *bus) { return; }
#endif /* HAVE_PCI_LEGACY */
@@ -1157,7 +1168,9 @@ struct pci_bus * pci_create_bus(struct d
goto dev_create_file_err;
/* Create legacy_io and legacy_mem files for this bus */
- pci_create_legacy_files(b);
+ error = pci_create_legacy_files(b);
+ if (error)
+ goto pci_create_legacy_files_err;
b->number = b->secondary = bus;
b->resource[0] = &ioport_resource;
@@ -1167,6 +1180,8 @@ struct pci_bus * pci_create_bus(struct d
return b;
+pci_create_legacy_files_err:
+ device_remove_file(&b->dev, &dev_attr_cpuaffinity);
dev_create_file_err:
device_unregister(&b->dev);
class_dev_reg_err:
On Tue, 2008-08-05 at 20:16 +1000, Simon Horman wrote:
> Check the return value of device_create_bin_file in pci_create_bus,
> unwind if neccessary, and propogate any errors to the caller.
>
> Signed-off-by: Simon Horman <[email protected]>
Hi Horms,
Fixing these kinds of warnings is highly useful but thankless work, so I
hate to nit pick ..
> Index: linux-2.6/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/probe.c 2008-08-05 19:58:43.000000000 +1000
> +++ linux-2.6/drivers/pci/probe.c 2008-08-05 19:59:15.000000000 +1000
> @@ -53,26 +53,37 @@ EXPORT_SYMBOL(no_pci_devices);
> * a per-bus basis. This routine creates the files and ties them into
> * their associated read, write and mmap files from pci-sysfs.c
> */
> -static void pci_create_legacy_files(struct pci_bus *b)
> +static int pci_create_legacy_files(struct pci_bus *b)
> {
> + int error;
> +
> b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
> GFP_ATOMIC);
> - if (b->legacy_io) {
> - b->legacy_io->attr.name = "legacy_io";
> - b->legacy_io->size = 0xffff;
> - b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> - b->legacy_io->read = pci_read_legacy_io;
> - b->legacy_io->write = pci_write_legacy_io;
> - device_create_bin_file(&b->dev, b->legacy_io);
> -
> - /* Allocated above after the legacy_io struct */
> - b->legacy_mem = b->legacy_io + 1;
> - b->legacy_mem->attr.name = "legacy_mem";
> - b->legacy_mem->size = 1024*1024;
> - b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> - b->legacy_mem->mmap = pci_mmap_legacy_mem;
> - device_create_bin_file(&b->dev, b->legacy_mem);
> + if (!b->legacy_io)
> + return -ENOMEM;
> +
> + b->legacy_io->attr.name = "legacy_io";
> + b->legacy_io->size = 0xffff;
> + b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> + b->legacy_io->read = pci_read_legacy_io;
> + b->legacy_io->write = pci_write_legacy_io;
> + error = device_create_bin_file(&b->dev, b->legacy_io);
> + if (error)
> + return error;
Should we kfree(b->legacy_io) here?
> +
> + /* Allocated above after the legacy_io struct */
> + b->legacy_mem = b->legacy_io + 1;
> + b->legacy_mem->attr.name = "legacy_mem";
> + b->legacy_mem->size = 1024*1024;
> + b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> + b->legacy_mem->mmap = pci_mmap_legacy_mem;
> + error = device_create_bin_file(&b->dev, b->legacy_mem);
> + if (error) {
> + device_remove_bin_file(&b->dev, b->legacy_io);
> + return error;
And here?
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
On Tue, 5 Aug 2008, Simon Horman wrote:
> Check the return value of device_create_bin_file in pci_create_bus,
> unwind if neccessary, and propogate any errors to the caller.
>
> Signed-off-by: Simon Horman <[email protected]>
>
> ---
>
> drivers/pci/probe.c: In function `pci_create_bus':
> drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
>
> # ia64-unknown-linux-gnu-gcc --version
> ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
> Copyright (C) 2004 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> Index: linux-2.6/drivers/pci/probe.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/probe.c 2008-08-05 19:58:43.000000000 +1000
> +++ linux-2.6/drivers/pci/probe.c 2008-08-05 19:59:15.000000000 +1000
> @@ -53,26 +53,37 @@ EXPORT_SYMBOL(no_pci_devices);
> * a per-bus basis. This routine creates the files and ties them into
> * their associated read, write and mmap files from pci-sysfs.c
> */
> -static void pci_create_legacy_files(struct pci_bus *b)
> +static int pci_create_legacy_files(struct pci_bus *b)
> {
> + int error;
> +
> b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
> GFP_ATOMIC);
> - if (b->legacy_io) {
> - b->legacy_io->attr.name = "legacy_io";
> - b->legacy_io->size = 0xffff;
> - b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> - b->legacy_io->read = pci_read_legacy_io;
> - b->legacy_io->write = pci_write_legacy_io;
> - device_create_bin_file(&b->dev, b->legacy_io);
> -
> - /* Allocated above after the legacy_io struct */
> - b->legacy_mem = b->legacy_io + 1;
> - b->legacy_mem->attr.name = "legacy_mem";
> - b->legacy_mem->size = 1024*1024;
> - b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> - b->legacy_mem->mmap = pci_mmap_legacy_mem;
> - device_create_bin_file(&b->dev, b->legacy_mem);
> + if (!b->legacy_io)
> + return -ENOMEM;
> +
> + b->legacy_io->attr.name = "legacy_io";
> + b->legacy_io->size = 0xffff;
> + b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> + b->legacy_io->read = pci_read_legacy_io;
> + b->legacy_io->write = pci_write_legacy_io;
> + error = device_create_bin_file(&b->dev, b->legacy_io);
> + if (error)
> + return error;
I'd release the memory here and NULLify legacy_io.
> +
> + /* Allocated above after the legacy_io struct */
> + b->legacy_mem = b->legacy_io + 1;
> + b->legacy_mem->attr.name = "legacy_mem";
> + b->legacy_mem->size = 1024*1024;
> + b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> + b->legacy_mem->mmap = pci_mmap_legacy_mem;
> + error = device_create_bin_file(&b->dev, b->legacy_mem);
> + if (error) {
> + device_remove_bin_file(&b->dev, b->legacy_io);
> + return error;
Here too.
Reason: If we fail to create the legacy_io file, legacy_mem will still be
NULL, because it has been not initialized at that point. But we will try
to remove it in pci_remove_legacy_files and in sysfs_remove_bin_file
we're going to derefence it and blow up.
> }
> +
> + return 0;
> }
>
> void pci_remove_legacy_files(struct pci_bus *b)
> @@ -84,7 +95,7 @@ void pci_remove_legacy_files(struct pci_
> }
> }
> #else /* !HAVE_PCI_LEGACY */
> -static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
> +static inline int pci_create_legacy_files(struct pci_bus *bus) { return 0; }
> void pci_remove_legacy_files(struct pci_bus *bus) { return; }
> #endif /* HAVE_PCI_LEGACY */
>
> @@ -1157,7 +1168,9 @@ struct pci_bus * pci_create_bus(struct d
> goto dev_create_file_err;
>
> /* Create legacy_io and legacy_mem files for this bus */
> - pci_create_legacy_files(b);
> + error = pci_create_legacy_files(b);
> + if (error)
> + goto pci_create_legacy_files_err;
>
> b->number = b->secondary = bus;
> b->resource[0] = &ioport_resource;
> @@ -1167,6 +1180,8 @@ struct pci_bus * pci_create_bus(struct d
>
> return b;
>
> +pci_create_legacy_files_err:
> + device_remove_file(&b->dev, &dev_attr_cpuaffinity);
> dev_create_file_err:
> device_unregister(&b->dev);
> class_dev_reg_err:
On Tue, Aug 05, 2008 at 12:39:31PM +0200, Sven Wegener wrote:
> On Tue, 5 Aug 2008, Simon Horman wrote:
>
> > Check the return value of device_create_bin_file in pci_create_bus,
> > unwind if neccessary, and propogate any errors to the caller.
> >
> > Signed-off-by: Simon Horman <[email protected]>
> >
> > ---
> >
> > drivers/pci/probe.c: In function `pci_create_bus':
> > drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> > drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> >
> > # ia64-unknown-linux-gnu-gcc --version
> > ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
> > Copyright (C) 2004 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions. There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> >
> > Index: linux-2.6/drivers/pci/probe.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/probe.c 2008-08-05 19:58:43.000000000 +1000
> > +++ linux-2.6/drivers/pci/probe.c 2008-08-05 19:59:15.000000000 +1000
> > @@ -53,26 +53,37 @@ EXPORT_SYMBOL(no_pci_devices);
> > * a per-bus basis. This routine creates the files and ties them into
> > * their associated read, write and mmap files from pci-sysfs.c
> > */
> > -static void pci_create_legacy_files(struct pci_bus *b)
> > +static int pci_create_legacy_files(struct pci_bus *b)
> > {
> > + int error;
> > +
> > b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
> > GFP_ATOMIC);
> > - if (b->legacy_io) {
> > - b->legacy_io->attr.name = "legacy_io";
> > - b->legacy_io->size = 0xffff;
> > - b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> > - b->legacy_io->read = pci_read_legacy_io;
> > - b->legacy_io->write = pci_write_legacy_io;
> > - device_create_bin_file(&b->dev, b->legacy_io);
> > -
> > - /* Allocated above after the legacy_io struct */
> > - b->legacy_mem = b->legacy_io + 1;
> > - b->legacy_mem->attr.name = "legacy_mem";
> > - b->legacy_mem->size = 1024*1024;
> > - b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> > - b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > - device_create_bin_file(&b->dev, b->legacy_mem);
> > + if (!b->legacy_io)
> > + return -ENOMEM;
> > +
> > + b->legacy_io->attr.name = "legacy_io";
> > + b->legacy_io->size = 0xffff;
> > + b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> > + b->legacy_io->read = pci_read_legacy_io;
> > + b->legacy_io->write = pci_write_legacy_io;
> > + error = device_create_bin_file(&b->dev, b->legacy_io);
> > + if (error)
> > + return error;
>
> I'd release the memory here and NULLify legacy_io.
>
> > +
> > + /* Allocated above after the legacy_io struct */
> > + b->legacy_mem = b->legacy_io + 1;
> > + b->legacy_mem->attr.name = "legacy_mem";
> > + b->legacy_mem->size = 1024*1024;
> > + b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> > + b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > + error = device_create_bin_file(&b->dev, b->legacy_mem);
> > + if (error) {
> > + device_remove_bin_file(&b->dev, b->legacy_io);
> > + return error;
>
> Here too.
>
> Reason: If we fail to create the legacy_io file, legacy_mem will still be
> NULL, because it has been not initialized at that point. But we will try
> to remove it in pci_remove_legacy_files and in sysfs_remove_bin_file
> we're going to derefence it and blow up.
Yes, sorry, my bad.
Check the return value of device_create_bin_file in pci_create_bus,
unwind if necessary, and propagate any errors to the caller.
Cc: Sven Wegener <[email protected]>
Cc: Michael Ellerman <[email protected]>
Signed-off-by: Simon Horman <[email protected]>
---
Revised the patch to free and NULLify b->legacy_io in
pci_create_legacy_files() on error. Thanks to
Sven Wegener and Michael Ellerman for pointing out this omission.
This patch resolves the following warnings:
drivers/pci/probe.c: In function `pci_create_bus':
drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
# ia64-unknown-linux-gnu-gcc --version
ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c 2008-08-05 20:50:10.000000000 +1000
+++ linux-2.6/drivers/pci/probe.c 2008-08-05 21:08:27.000000000 +1000
@@ -53,26 +53,42 @@ EXPORT_SYMBOL(no_pci_devices);
* a per-bus basis. This routine creates the files and ties them into
* their associated read, write and mmap files from pci-sysfs.c
*/
-static void pci_create_legacy_files(struct pci_bus *b)
+static int pci_create_legacy_files(struct pci_bus *b)
{
+ int error;
+
b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
GFP_ATOMIC);
- if (b->legacy_io) {
- b->legacy_io->attr.name = "legacy_io";
- b->legacy_io->size = 0xffff;
- b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_io->read = pci_read_legacy_io;
- b->legacy_io->write = pci_write_legacy_io;
- device_create_bin_file(&b->dev, b->legacy_io);
-
- /* Allocated above after the legacy_io struct */
- b->legacy_mem = b->legacy_io + 1;
- b->legacy_mem->attr.name = "legacy_mem";
- b->legacy_mem->size = 1024*1024;
- b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_mem->mmap = pci_mmap_legacy_mem;
- device_create_bin_file(&b->dev, b->legacy_mem);
- }
+ if (!b->legacy_io)
+ return -ENOMEM;
+
+ b->legacy_io->attr.name = "legacy_io";
+ b->legacy_io->size = 0xffff;
+ b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_io->read = pci_read_legacy_io;
+ b->legacy_io->write = pci_write_legacy_io;
+ error = device_create_bin_file(&b->dev, b->legacy_io);
+ if (error)
+ goto legacy_io_err;
+
+ /* Allocated above after the legacy_io struct */
+ b->legacy_mem = b->legacy_io + 1;
+ b->legacy_mem->attr.name = "legacy_mem";
+ b->legacy_mem->size = 1024*1024;
+ b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_mem->mmap = pci_mmap_legacy_mem;
+ error = device_create_bin_file(&b->dev, b->legacy_mem);
+ if (error)
+ goto legacy_mem_err;
+
+ return 0;
+
+legacy_mem_err:
+ device_remove_bin_file(&b->dev, b->legacy_io);
+legacy_io_err:
+ kfree(b->legacy_io);
+ b->legacy_io = NULL;
+ return error;
}
void pci_remove_legacy_files(struct pci_bus *b)
@@ -84,7 +100,7 @@ void pci_remove_legacy_files(struct pci_
}
}
#else /* !HAVE_PCI_LEGACY */
-static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
+static inline int pci_create_legacy_files(struct pci_bus *bus) { return 0; }
void pci_remove_legacy_files(struct pci_bus *bus) { return; }
#endif /* HAVE_PCI_LEGACY */
@@ -1157,7 +1173,9 @@ struct pci_bus * pci_create_bus(struct d
goto dev_create_file_err;
/* Create legacy_io and legacy_mem files for this bus */
- pci_create_legacy_files(b);
+ error = pci_create_legacy_files(b);
+ if (error)
+ goto pci_create_legacy_files_err;
b->number = b->secondary = bus;
b->resource[0] = &ioport_resource;
@@ -1167,6 +1185,8 @@ struct pci_bus * pci_create_bus(struct d
return b;
+pci_create_legacy_files_err:
+ device_remove_file(&b->dev, &dev_attr_cpuaffinity);
dev_create_file_err:
device_unregister(&b->dev);
class_dev_reg_err:
On Tue, Aug 05, 2008 at 09:14:07PM +1000, Simon Horman wrote:
> Check the return value of device_create_bin_file in pci_create_bus,
> unwind if necessary, and propagate any errors to the caller.
Yes, but you're essentially saying here that if I can't create a couple
of poxy sysfs files, I can't have this PCI bus at all? This seems like
a bad decision to me. I'd rather have a PCI bus without the files than
no PCI bus at all. By all means, we should whinge mightily if we can't
create the files so the sysadmin has a chance of figuring out why things
aren't quite working right, but I might have my root filesystem on a
device on that PCI bus.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, Aug 05, 2008 at 05:28:14AM -0600, Matthew Wilcox wrote:
> On Tue, Aug 05, 2008 at 09:14:07PM +1000, Simon Horman wrote:
> > Check the return value of device_create_bin_file in pci_create_bus,
> > unwind if necessary, and propagate any errors to the caller.
>
> Yes, but you're essentially saying here that if I can't create a couple
> of poxy sysfs files, I can't have this PCI bus at all? This seems like
> a bad decision to me. I'd rather have a PCI bus without the files than
> no PCI bus at all. By all means, we should whinge mightily if we can't
> create the files so the sysadmin has a chance of figuring out why things
> aren't quite working right, but I might have my root filesystem on a
> device on that PCI bus.
Are you suggesting just making pci_create_bus() have a big winge
using printk() but returning void regardless of what happens?
That sounds fine to me, though I guess it would also need to unwind
if the first call to device_create_bin_file() succeeds but the second
one doesn't.
On Tue, Aug 05, 2008 at 10:15:32PM +1000, Simon Horman wrote:
> On Tue, Aug 05, 2008 at 05:28:14AM -0600, Matthew Wilcox wrote:
> > Yes, but you're essentially saying here that if I can't create a couple
> > of poxy sysfs files, I can't have this PCI bus at all? This seems like
> > a bad decision to me. I'd rather have a PCI bus without the files than
> > no PCI bus at all. By all means, we should whinge mightily if we can't
> > create the files so the sysadmin has a chance of figuring out why things
> > aren't quite working right, but I might have my root filesystem on a
> > device on that PCI bus.
>
> Are you suggesting just making pci_create_bus() have a big winge
> using printk() but returning void regardless of what happens?
> That sounds fine to me, though I guess it would also need to unwind
> if the first call to device_create_bin_file() succeeds but the second
> one doesn't.
Sounds like the right course of action to me.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Tue, 5 Aug 2008, Sven Wegener wrote:
> On Tue, 5 Aug 2008, Simon Horman wrote:
>
> > Check the return value of device_create_bin_file in pci_create_bus,
> > unwind if neccessary, and propogate any errors to the caller.
> >
> > Signed-off-by: Simon Horman <[email protected]>
> >
> > ---
> >
> > drivers/pci/probe.c: In function `pci_create_bus':
> > drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> > drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
> >
> > # ia64-unknown-linux-gnu-gcc --version
> > ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
> > Copyright (C) 2004 Free Software Foundation, Inc.
> > This is free software; see the source for copying conditions. There is NO
> > warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> >
> > Index: linux-2.6/drivers/pci/probe.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/probe.c 2008-08-05 19:58:43.000000000 +1000
> > +++ linux-2.6/drivers/pci/probe.c 2008-08-05 19:59:15.000000000 +1000
> > @@ -53,26 +53,37 @@ EXPORT_SYMBOL(no_pci_devices);
> > * a per-bus basis. This routine creates the files and ties them into
> > * their associated read, write and mmap files from pci-sysfs.c
> > */
> > -static void pci_create_legacy_files(struct pci_bus *b)
> > +static int pci_create_legacy_files(struct pci_bus *b)
> > {
> > + int error;
> > +
> > b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
> > GFP_ATOMIC);
> > - if (b->legacy_io) {
> > - b->legacy_io->attr.name = "legacy_io";
> > - b->legacy_io->size = 0xffff;
> > - b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> > - b->legacy_io->read = pci_read_legacy_io;
> > - b->legacy_io->write = pci_write_legacy_io;
> > - device_create_bin_file(&b->dev, b->legacy_io);
> > -
> > - /* Allocated above after the legacy_io struct */
> > - b->legacy_mem = b->legacy_io + 1;
> > - b->legacy_mem->attr.name = "legacy_mem";
> > - b->legacy_mem->size = 1024*1024;
> > - b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> > - b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > - device_create_bin_file(&b->dev, b->legacy_mem);
> > + if (!b->legacy_io)
> > + return -ENOMEM;
> > +
> > + b->legacy_io->attr.name = "legacy_io";
> > + b->legacy_io->size = 0xffff;
> > + b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
> > + b->legacy_io->read = pci_read_legacy_io;
> > + b->legacy_io->write = pci_write_legacy_io;
> > + error = device_create_bin_file(&b->dev, b->legacy_io);
> > + if (error)
> > + return error;
>
> I'd release the memory here and NULLify legacy_io.
>
> > +
> > + /* Allocated above after the legacy_io struct */
> > + b->legacy_mem = b->legacy_io + 1;
> > + b->legacy_mem->attr.name = "legacy_mem";
> > + b->legacy_mem->size = 1024*1024;
> > + b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
> > + b->legacy_mem->mmap = pci_mmap_legacy_mem;
> > + error = device_create_bin_file(&b->dev, b->legacy_mem);
> > + if (error) {
> > + device_remove_bin_file(&b->dev, b->legacy_io);
> > + return error;
>
> Here too.
>
> Reason: If we fail to create the legacy_io file, legacy_mem will still be
> NULL, because it has been not initialized at that point. But we will try
> to remove it in pci_remove_legacy_files and in sysfs_remove_bin_file
> we're going to derefence it and blow up.
Uhm, this case can not happen in this version of the patch. We'll actually
fail registering the bus completely with this patch, so there should be no
chance that we'll ever go through pci_remove_legacy_files with legacy_io
!= NULL and legacy_mem == NULL. So no need to NULLify it. But with the
change Matthew suggested, we need to pay attention to it. Sorry for the
noise here.
> > }
> > +
> > + return 0;
> > }
> >
> > void pci_remove_legacy_files(struct pci_bus *b)
> > @@ -84,7 +95,7 @@ void pci_remove_legacy_files(struct pci_
> > }
> > }
> > #else /* !HAVE_PCI_LEGACY */
> > -static inline void pci_create_legacy_files(struct pci_bus *bus) { return; }
> > +static inline int pci_create_legacy_files(struct pci_bus *bus) { return 0; }
> > void pci_remove_legacy_files(struct pci_bus *bus) { return; }
> > #endif /* HAVE_PCI_LEGACY */
> >
> > @@ -1157,7 +1168,9 @@ struct pci_bus * pci_create_bus(struct d
> > goto dev_create_file_err;
> >
> > /* Create legacy_io and legacy_mem files for this bus */
> > - pci_create_legacy_files(b);
> > + error = pci_create_legacy_files(b);
> > + if (error)
> > + goto pci_create_legacy_files_err;
> >
> > b->number = b->secondary = bus;
> > b->resource[0] = &ioport_resource;
> > @@ -1167,6 +1180,8 @@ struct pci_bus * pci_create_bus(struct d
> >
> > return b;
> >
> > +pci_create_legacy_files_err:
> > + device_remove_file(&b->dev, &dev_attr_cpuaffinity);
> > dev_create_file_err:
> > device_unregister(&b->dev);
> > class_dev_reg_err:
Check the return value of device_create_bin_file in pci_create_bus and
unwind if necessary. Don't propagate error to caller, as failure to create
these files shouldn't prevent PCI from being initialised.
Cc: Sven Wegener <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Signed-off-by: Simon Horman <[email protected]>
---
Tue, 5 Aug 2008 21:14:07 +1000
* Revised the patch to free and NULLify b->legacy_io in
pci_create_legacy_files() on error. Thanks to
Sven Wegener and Michael Ellerman for pointing out this omission.
Wed, 06 Aug 2008 10:49:30 +1000
* Don't propagate error to caller, as failure to create these files
shouldn't prevent PCI from being initialised.
Thanks to Matthew Wilcox
This patch resolves the following warnings:
drivers/pci/probe.c: In function `pci_create_bus':
drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
# ia64-unknown-linux-gnu-gcc --version
ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c 2008-08-06 09:22:06.000000000 +1000
+++ linux-2.6/drivers/pci/probe.c 2008-08-06 09:41:09.000000000 +1000
@@ -52,27 +52,46 @@ EXPORT_SYMBOL(no_pci_devices);
* Some platforms allow access to legacy I/O port and ISA memory space on
* a per-bus basis. This routine creates the files and ties them into
* their associated read, write and mmap files from pci-sysfs.c
+ *
+ * On error unwind, but don't propogate the error to the caller
+ * as it is ok to set up the PCI bus without these files.
*/
static void pci_create_legacy_files(struct pci_bus *b)
{
+ int error;
+
b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
GFP_ATOMIC);
- if (b->legacy_io) {
- b->legacy_io->attr.name = "legacy_io";
- b->legacy_io->size = 0xffff;
- b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_io->read = pci_read_legacy_io;
- b->legacy_io->write = pci_write_legacy_io;
- device_create_bin_file(&b->dev, b->legacy_io);
-
- /* Allocated above after the legacy_io struct */
- b->legacy_mem = b->legacy_io + 1;
- b->legacy_mem->attr.name = "legacy_mem";
- b->legacy_mem->size = 1024*1024;
- b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_mem->mmap = pci_mmap_legacy_mem;
- device_create_bin_file(&b->dev, b->legacy_mem);
- }
+ if (!b->legacy_io)
+ return;
+
+ b->legacy_io->attr.name = "legacy_io";
+ b->legacy_io->size = 0xffff;
+ b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_io->read = pci_read_legacy_io;
+ b->legacy_io->write = pci_write_legacy_io;
+ error = device_create_bin_file(&b->dev, b->legacy_io);
+ if (error)
+ goto legacy_io_err;
+
+ /* Allocated above after the legacy_io struct */
+ b->legacy_mem = b->legacy_io + 1;
+ b->legacy_mem->attr.name = "legacy_mem";
+ b->legacy_mem->size = 1024*1024;
+ b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_mem->mmap = pci_mmap_legacy_mem;
+ error = device_create_bin_file(&b->dev, b->legacy_mem);
+ if (error)
+ goto legacy_mem_err;
+
+ return 0;
+
+legacy_mem_err:
+ device_remove_bin_file(&b->dev, b->legacy_io);
+legacy_io_err:
+ kfree(b->legacy_io);
+ b->legacy_io = NULL;
+ return;
}
void pci_remove_legacy_files(struct pci_bus *b)
On Wed, Aug 06, 2008 at 10:57:21AM +1000, Simon Horman wrote:
> Check the return value of device_create_bin_file in pci_create_bus and
> unwind if necessary. Don't propagate error to caller, as failure to create
> these files shouldn't prevent PCI from being initialised.
Much better ... one nit though:
> static void pci_create_legacy_files(struct pci_bus *b)
> {
[...]
> +
> + return 0;
I'm sure the compiler warns about that.
--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
Check the return value of device_create_bin_file in pci_create_bus and
unwind if necessary. Don't propagate error to caller, as failure to create
these files shouldn't prevent PCI from being initialised.
Cc: Sven Wegener <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Signed-off-by: Simon Horman <[email protected]>
---
Tue, 5 Aug 2008 21:14:07 +1000
* Revised the patch to free and NULLify b->legacy_io in
pci_create_legacy_files() on error. Thanks to
Sven Wegener and Michael Ellerman for pointing out this omission.
Wed, 06 Aug 2008 10:49:30 +1000
* Don't propagate error to caller, as failure to create these files
shouldn't prevent PCI from being initialised.
Thanks to Matthew Wilcox
Thu, 07 Aug 2008 09:28:56 +1000
* Remove spurious return value.
Amusingly this patch was introducing warnings :-)
This patch resolves the following warnings:
drivers/pci/probe.c: In function `pci_create_bus':
drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
# ia64-unknown-linux-gnu-gcc --version
ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c 2008-08-07 09:08:32.000000000 +1000
+++ linux-2.6/drivers/pci/probe.c 2008-08-07 09:14:53.000000000 +1000
@@ -52,27 +52,46 @@ EXPORT_SYMBOL(no_pci_devices);
* Some platforms allow access to legacy I/O port and ISA memory space on
* a per-bus basis. This routine creates the files and ties them into
* their associated read, write and mmap files from pci-sysfs.c
+ *
+ * On error unwind, but don't propogate the error to the caller
+ * as it is ok to set up the PCI bus without these files.
*/
static void pci_create_legacy_files(struct pci_bus *b)
{
+ int error;
+
b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
GFP_ATOMIC);
- if (b->legacy_io) {
- b->legacy_io->attr.name = "legacy_io";
- b->legacy_io->size = 0xffff;
- b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_io->read = pci_read_legacy_io;
- b->legacy_io->write = pci_write_legacy_io;
- device_create_bin_file(&b->dev, b->legacy_io);
-
- /* Allocated above after the legacy_io struct */
- b->legacy_mem = b->legacy_io + 1;
- b->legacy_mem->attr.name = "legacy_mem";
- b->legacy_mem->size = 1024*1024;
- b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_mem->mmap = pci_mmap_legacy_mem;
- device_create_bin_file(&b->dev, b->legacy_mem);
- }
+ if (!b->legacy_io)
+ return;
+
+ b->legacy_io->attr.name = "legacy_io";
+ b->legacy_io->size = 0xffff;
+ b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_io->read = pci_read_legacy_io;
+ b->legacy_io->write = pci_write_legacy_io;
+ error = device_create_bin_file(&b->dev, b->legacy_io);
+ if (error)
+ goto legacy_io_err;
+
+ /* Allocated above after the legacy_io struct */
+ b->legacy_mem = b->legacy_io + 1;
+ b->legacy_mem->attr.name = "legacy_mem";
+ b->legacy_mem->size = 1024*1024;
+ b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_mem->mmap = pci_mmap_legacy_mem;
+ error = device_create_bin_file(&b->dev, b->legacy_mem);
+ if (error)
+ goto legacy_mem_err;
+
+ return;
+
+legacy_mem_err:
+ device_remove_bin_file(&b->dev, b->legacy_io);
+legacy_io_err:
+ kfree(b->legacy_io);
+ b->legacy_io = NULL;
+ return;
}
void pci_remove_legacy_files(struct pci_bus *b)
On Wed, Aug 06, 2008 at 07:19:01AM -0600, Matthew Wilcox wrote:
> On Wed, Aug 06, 2008 at 10:57:21AM +1000, Simon Horman wrote:
> > Check the return value of device_create_bin_file in pci_create_bus and
> > unwind if necessary. Don't propagate error to caller, as failure to create
> > these files shouldn't prevent PCI from being initialised.
>
> Much better ... one nit though:
>
> > static void pci_create_legacy_files(struct pci_bus *b)
> > {
> [...]
> > +
> > + return 0;
>
> I'm sure the compiler warns about that.
Amusingly, yes.
--
Horms -> looks for somewhere to hide
On Thu, 2008-08-07 at 09:30 +1000, Simon Horman wrote:
> Check the return value of device_create_bin_file in pci_create_bus and
> unwind if necessary. Don't propagate error to caller, as failure to create
> these files shouldn't prevent PCI from being initialised.
>
> Cc: Sven Wegener <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Signed-off-by: Simon Horman <[email protected]>
I hate to prolong the pain, but wasn't the plan to print a big fat
warning so that it's clear the system is not 100% happy.
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
On Thu, Aug 07, 2008 at 10:30:26AM +1000, Michael Ellerman wrote:
> On Thu, 2008-08-07 at 09:30 +1000, Simon Horman wrote:
> > Check the return value of device_create_bin_file in pci_create_bus and
> > unwind if necessary. Don't propagate error to caller, as failure to create
> > these files shouldn't prevent PCI from being initialised.
> >
> > Cc: Sven Wegener <[email protected]>
> > Cc: Michael Ellerman <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Signed-off-by: Simon Horman <[email protected]>
>
> I hate to prolong the pain, but wasn't the plan to print a big fat
> warning so that it's clear the system is not 100% happy.
Sorry, I fogot about that bit.
Check the return value of device_create_bin_file in pci_create_bus and
unwind if necessary. Don't propagate error to caller, as failure to create
these files shouldn't prevent PCI from being initialised. Instead, just
log a warning.
Cc: Sven Wegener <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Signed-off-by: Simon Horman <[email protected]>
---
Tue, 5 Aug 2008 21:14:07 +1000
* Revised the patch to free and NULLify b->legacy_io in
pci_create_legacy_files() on error.
Wed, 06 Aug 2008 10:49:30 +1000
* Don't propagate error to caller, as failure to create these files
shouldn't prevent PCI from being initialised.
Thu, 07 Aug 2008 09:28:56 +1000
* Remove spurious return value.
Amusingly this patch was introducing warnings :-)
Thu, 07 Aug 2008 11:15:32 +1000
* Print a warning if legacy file initialisation fails.
This patch resolves the following warnings:
drivers/pci/probe.c: In function `pci_create_bus':
drivers/pci/probe.c:66: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
drivers/pci/probe.c:74: warning: ignoring return value of `device_create_bin_file', declared with attribute warn_unused_result
# ia64-unknown-linux-gnu-gcc --version
ia64-unknown-linux-gnu-gcc (GCC) 3.4.5
Copyright (C) 2004 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c 2008-08-07 09:08:32.000000000 +1000
+++ linux-2.6/drivers/pci/probe.c 2008-08-07 11:21:31.000000000 +1000
@@ -52,27 +52,49 @@ EXPORT_SYMBOL(no_pci_devices);
* Some platforms allow access to legacy I/O port and ISA memory space on
* a per-bus basis. This routine creates the files and ties them into
* their associated read, write and mmap files from pci-sysfs.c
+ *
+ * On error unwind, but don't propogate the error to the caller
+ * as it is ok to set up the PCI bus without these files.
*/
static void pci_create_legacy_files(struct pci_bus *b)
{
+ int error;
+
b->legacy_io = kzalloc(sizeof(struct bin_attribute) * 2,
GFP_ATOMIC);
- if (b->legacy_io) {
- b->legacy_io->attr.name = "legacy_io";
- b->legacy_io->size = 0xffff;
- b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_io->read = pci_read_legacy_io;
- b->legacy_io->write = pci_write_legacy_io;
- device_create_bin_file(&b->dev, b->legacy_io);
-
- /* Allocated above after the legacy_io struct */
- b->legacy_mem = b->legacy_io + 1;
- b->legacy_mem->attr.name = "legacy_mem";
- b->legacy_mem->size = 1024*1024;
- b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
- b->legacy_mem->mmap = pci_mmap_legacy_mem;
- device_create_bin_file(&b->dev, b->legacy_mem);
- }
+ if (!b->legacy_io)
+ goto kzalloc_err;
+
+ b->legacy_io->attr.name = "legacy_io";
+ b->legacy_io->size = 0xffff;
+ b->legacy_io->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_io->read = pci_read_legacy_io;
+ b->legacy_io->write = pci_write_legacy_io;
+ error = device_create_bin_file(&b->dev, b->legacy_io);
+ if (error)
+ goto legacy_io_err;
+
+ /* Allocated above after the legacy_io struct */
+ b->legacy_mem = b->legacy_io + 1;
+ b->legacy_mem->attr.name = "legacy_mem";
+ b->legacy_mem->size = 1024*1024;
+ b->legacy_mem->attr.mode = S_IRUSR | S_IWUSR;
+ b->legacy_mem->mmap = pci_mmap_legacy_mem;
+ error = device_create_bin_file(&b->dev, b->legacy_mem);
+ if (error)
+ goto legacy_mem_err;
+
+ return;
+
+legacy_mem_err:
+ device_remove_bin_file(&b->dev, b->legacy_io);
+legacy_io_err:
+ kfree(b->legacy_io);
+ b->legacy_io = NULL;
+kzalloc_err:
+ printk(KERN_WARNING "pci: warning: could not create legacy I/O port "
+ "and ISA memory resources to sysfs\n");
+ return;
}
void pci_remove_legacy_files(struct pci_bus *b)
On Wednesday, August 6, 2008 9:56 pm Simon Horman wrote:
> Check the return value of device_create_bin_file in pci_create_bus and
> unwind if necessary. Don't propagate error to caller, as failure to create
> these files shouldn't prevent PCI from being initialised. Instead, just
> log a warning.
>
> Cc: Sven Wegener <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Signed-off-by: Simon Horman <[email protected]>
Applied, thanks for your persistence, Simon! :)
Thanks,
Jesse
On Thu, Aug 07, 2008 at 09:50:50AM -0700, Jesse Barnes wrote:
> On Wednesday, August 6, 2008 9:56 pm Simon Horman wrote:
> > Check the return value of device_create_bin_file in pci_create_bus and
> > unwind if necessary. Don't propagate error to caller, as failure to create
> > these files shouldn't prevent PCI from being initialised. Instead, just
> > log a warning.
> >
> > Cc: Sven Wegener <[email protected]>
> > Cc: Michael Ellerman <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Signed-off-by: Simon Horman <[email protected]>
>
> Applied, thanks for your persistence, Simon! :)
Thanks, sorry for not getting it right much earlier.