Om Narasimhan wrote: > Hi, > I had submitted this patch sometime earlier. Submitting again after > fixing a bug in the patch. > I have not subscribed to linux-acpi. please CC me in replies. Please add a description here what you do in your patch. Especially if you're going to fix a bug it would be good if you shortly describe where it was and how it was wrong, e.g. "foo() was freeing struct bar on error, but nobody ever cared for the children of bar". > Signed Off by : Om Narasimhan <[email protected]> > > -- > > drivers/acpi/ac.c | 4 +--- > drivers/acpi/acpi_memhotplug.c | 14 ++++---------- > drivers/acpi/asus_acpi.c | 3 +-- > drivers/acpi/battery.c | 13 +++---------- > drivers/acpi/bus.c | 3 +-- > drivers/acpi/button.c | 4 +--- > drivers/acpi/container.c | 4 +--- > drivers/acpi/ec.c | 20 +++++--------------- > drivers/acpi/fan.c | 3 +-- > drivers/acpi/i2c_ec.c | 8 ++------ > drivers/acpi/osl.c | 2 +- > drivers/acpi/pci_bind.c | 21 +++++---------------- > drivers/acpi/pci_irq.c | 16 +++------------- > drivers/acpi/pci_link.c | 6 ++---- > drivers/acpi/pci_root.c | 3 +-- > drivers/acpi/power.c | 4 +--- > drivers/acpi/processor_core.c | 4 +--- > drivers/acpi/sbs.c | 4 +--- > drivers/acpi/thermal.c | 13 +++---------- > drivers/acpi/utils.c | 2 -- > 20 files changed, 38 insertions(+), 113 deletions(-) > > diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c > index 11abc7b..0d05d36 100644 > --- a/drivers/acpi/ac.c > +++ b/drivers/acpi/ac.c > @@ -221,11 +221,9 @@ static int acpi_ac_add(struct acpi_devic > if (!device) > return -EINVAL; > > - ac = kmalloc(sizeof(struct acpi_ac), GFP_KERNEL); > + ac = kzalloc(sizeof(struct acpi_ac), GFP_KERNEL); > if (!ac) > return -ENOMEM; > - memset(ac, 0, sizeof(struct acpi_ac)); > - > ac->device = device; > strcpy(acpi_device_name(device), ACPI_AC_DEVICE_NAME); > strcpy(acpi_device_class(device), ACPI_AC_CLASS); > diff --git a/drivers/acpi/acpi_memhotplug.c > b/drivers/acpi/acpi_memhotplug.c index 1dda370..4b1a77d 100644 > --- a/drivers/acpi/acpi_memhotplug.c > +++ b/drivers/acpi/acpi_memhotplug.c > @@ -292,7 +292,6 @@ static int acpi_memory_disable_device(st > int result; > struct acpi_memory_info *info, *n; > > - > /* > * Ask the VM to offline this memory range. > * Note: Assume that this function returns zero on success Depends on the maintainer, but usually it is preferred to do whitespace cleanups in their own patch. Now I have to dig through >600 lines of patch to find the bug you fixed. I would have split this one into 3 seperate pieces: whitespace, k[mz]alloc and the bug fix. This depends on the whishes of the maintainer, but usually this makes tracking of changes much easier as the first two can later be ignored more or less if someones trying to nail down a change in behaviour to a specific patch. > @@ -418,14 +412,14 @@ static int acpi_memory_device_add(struct > static int acpi_memory_device_remove(struct acpi_device *device, int type) > { > struct acpi_memory_device *mem_device = NULL; > - > + struct acpi_memory_info *info, *n; > > if (!device || !acpi_driver_data(device)) > return -EINVAL; > - > mem_device = (struct acpi_memory_device *)acpi_driver_data(device); > + list_for_each_entry_safe(info, n, &mem_device->res_list, list) > + kfree(info); > kfree(mem_device); > - > return 0; > } This has nothing to do with your patch, but isn't this check here a bit superfluous? Also the check in acpi_memory_device_add() to check for device? These functions should never be called if device is NULL. If it is this is a bug and it's better to BUG() or just oops here instead of hiding this bug. Also acpi_memory_device_remove() should never be called if acpi_memory_device_add() fails. But if this succeeded acpi_driver_data(device) will always be valid. If it is deleted later by anyone else this is also a bug and shouldn't be "ignored" silently. Opinions? > @@ -277,15 +274,12 @@ int acpi_pci_unbind(struct acpi_device * > char *pathname = NULL; > struct acpi_buffer buffer = { 0, NULL }; > > - > if (!device || !device->parent) > return -EINVAL; > > - pathname = (char *)kmalloc(ACPI_PATHNAME_MAX, GFP_KERNEL); > + pathname = (char *)kzalloc(ACPI_PATHNAME_MAX, GFP_KERNEL); > if (!pathname) > return -ENOMEM; > - memset(pathname, 0, ACPI_PATHNAME_MAX); > - > buffer.length = ACPI_PATHNAME_MAX; > buffer.pointer = pathname; > acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer); This btw. is an example where I would also do a whitespace change in a "real" patch. It doesn't matter here as this area is touched anyway. Not 100% correct, but who is? :) > @@ -332,11 +326,9 @@ acpi_pci_bind_root(struct acpi_device *d > struct acpi_buffer buffer = { 0, NULL }; > > > - pathname = (char *)kmalloc(ACPI_PATHNAME_MAX, GFP_KERNEL); > + pathname = (char *)kzalloc(ACPI_PATHNAME_MAX, GFP_KERNEL); > if (!pathname) > return -ENOMEM; > - memset(pathname, 0, ACPI_PATHNAME_MAX); > - > buffer.length = ACPI_PATHNAME_MAX; > buffer.pointer = pathname; > Kill that cast. kzalloc() returns void* which is assignement compatible with any pointer. > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > index feda034..b64c50a 100644 > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -85,21 +85,14 @@ acpi_pci_irq_add_entry(acpi_handle handl > { > struct acpi_prt_entry *entry = NULL; > > - > - if (!prt) > - return -EINVAL; > - > - entry = kmalloc(sizeof(struct acpi_prt_entry), GFP_KERNEL); > + entry = kzalloc(sizeof(struct acpi_prt_entry), GFP_KERNEL); > if (!entry) > return -ENOMEM; > - memset(entry, 0, sizeof(struct acpi_prt_entry)); > - > entry->id.segment = segment; > entry->id.bus = bus; > entry->id.device = (prt->address >> 16) & 0xFFFF; > entry->id.function = prt->address & 0xFFFF; > entry->pin = prt->pin; > - > /* > * Type 1: Dynamic > * --------------- You are changing the way how this code works here as you removed the check for prt. Another good example why to split up this patch as this might sometime really byte someone (haven't checked the code). > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 5753d06..bdb0353 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -902,13 +902,11 @@ acpi_thermal_write_trip_points(struct fi > int i = 0; > > > - limit_string = kmalloc(ACPI_THERMAL_MAX_LIMIT_STR_LEN, GFP_KERNEL); > + limit_string = kzalloc(ACPI_THERMAL_MAX_LIMIT_STR_LEN, GFP_KERNEL); > if (!limit_string) > return -ENOMEM; > > - memset(limit_string, 0, ACPI_THERMAL_MAX_LIMIT_STR_LEN); > - > - active = kmalloc(ACPI_THERMAL_MAX_ACTIVE * sizeof(int), GFP_KERNEL); > + active = kzalloc(ACPI_THERMAL_MAX_ACTIVE * sizeof(int), GFP_KERNEL); > if (!active) { > kfree(limit_string); > return -ENOMEM; *looking in the file* Eeks, what's that? if (!tz || (count > ACPI_THERMAL_MAX_LIMIT_STR_LEN - 1)) { What about if (!tz || (count >= ACPI_THERMAL_MAX_LIMIT_STR_LEN)) { Eike
Attachment:
pgpyZyJjPUMAB.pgp
Description: PGP signature
- References:
- acpi kmalloc to kzalloc conversion and a memory leak fix.
- From: "Om Narasimhan" <[email protected]>
- acpi kmalloc to kzalloc conversion and a memory leak fix.
- Prev by Date: Re: Same MCE on 4 working machines (was Re: Early boot hang on recent 2.6 kernels (> 2.6.3), on x86-64 with 16gb of RAM)
- Next by Date: Re: [patch 1/8] extend make headers_check to detect more problems
- Previous by thread: acpi kmalloc to kzalloc conversion and a memory leak fix.
- Next by thread: [patch 0/8] Re: All arch maintainers: 'make headers_check' fails on most architectures.
- Index(es):
 |