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 <om.turyx@xxxxxxxxx>
>
> --
>
> 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:
pgpD7oBj3DjlX.pgp
Description: PGP signature
