acpi:glue.c Fix warning: variable 'ret' set but not used
in [Kernel]
|
Prev: security:ebitmap.c Fix warning: variable 'e_sft' set but not used
Next: block: defer the use of inline biovecs for discard requests
From: David Howells on 28 Jun 2010 08:50 Justin P. Mattock <justinmattock(a)gmail.com> wrote: > + if (ret) { > + printk(KERN_WARNING "dev%d: Failed to get exception\n", ret); > + } That's not a good warning because it's a meaningless string and you're passing the error number to the dev%d. Better would perhaps be: "dev%d: Failed to create physical_node sysfs link: %d\n" Note also that you're only checking the result of one sysfs_create_link(). You should probably check both. Also you're introducing a pair of unnecessary braces as there's only one statement in the if-body. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 28 Jun 2010 14:50 Justin P. Mattock <justinmattock(a)gmail.com> wrote: > would just removing ret be good or will things go out of whack because of no > ret Don't you then get warnings for not capturing the result? David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 29 Jun 2010 11:50 Justin P. Mattock <justinmattock(a)gmail.com> wrote: > + if (fn) { > + printk(KERN_WARNING "dev%d: Failed to create > firmware_node: %d\n", status, fn); > + }else if (pn) { > + printk(KERN_WARNING "dev%d: Failed to create > physical_node: %d\n", status, pn); > + return 0; > + } The if-statement should be correctly indented (it's inside another if-body, so needs to be one more tab over) and there needs to be a space before the else. You should probably split your printks up so they don't exceed 80 chars too, for example: printk(KERN_WARNING "dev%d: Failed to create physical_node: %d\n", status, pn); Also 'status' is probably the wrong thing to print as the number in "dev%d". If it worked, that should be unconditionally AE_OK, I think. Can you not use dev_warn() or similar instead or printk? David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 30 Jun 2010 05:20 Justin P. Mattock <justinmattock(a)gmail.com> wrote: > if (!ACPI_FAILURE(status)) { > - int ret; > > - ret = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj, > + fn = sysfs_create_link(&dev->kobj, &acpi_dev->dev.kobj, > "firmware_node"); > - ret = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj, > + pn = sysfs_create_link(&acpi_dev->dev.kobj, &dev->kobj, > "physical_node"); > + if (fn) { That new if-statement still needs indenting one more tab stop. It's indented the same as the previous if-statement, but is actually in the body of that previous if-statement. The body of the second if-statement should be indented one tab beyond the if, and else/else-if statements and the final closing brace should be indented level with the if: if (...) { body; } else if (...) { body; } else { body; } so that they line up vertically. > + dev_warn(dev, "dev:%p Failed to create firmware_node: %d\n", > + acpi_dev, fn); The "dev:%p " seems like it ought to be superfluous if you're using dev_warn(), and certainly, returning the pointer isn't really useful, I suspect. However, at this point you have two device struct pointers: dev and &acip_dev->dev, so printing them both is may be good. Perhaps something like: + dev_warn(&acpi_dev->dev, + "Failed to create firmware_node link to %s %s: %d\n", + dev_driver_string(dev), dev_name(dev), fn); David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 1 Jul 2010 05:40
Justin P. Mattock <justinmattock(a)gmail.com> wrote: > + if (fn) { > + dev_warn(&acpi_dev->dev, > + "Failed to create firmware_node link to %s %s: %d\n", > + dev_driver_string(dev), dev_name(dev), fn); > + } else if (pn) { > + dev_warn(&acpi_dev->dev, > + "Failed to create physical_node link to %s %s: %d\n", > + dev_driver_string(dev), dev_name(dev), pn); > + return AE_ERROR; > + } There's one more question to ask yourself: do you really need two dev_warn() statements? You could have just one that prints both error values: if (fn || pn) dev_warn(&acpi_dev->dev, "Failed to create link(s) to %s %s:" " fn=%d pn=%d\n", dev_driver_string(dev), dev_name(dev), fn, pn); Not sure it's worth going that far. You could reduce it still further: if (fn || pn) dev_warn(&acpi_dev->dev, "Failed to create link(s) to %s %s:" " %d\n", dev_driver_string(dev), dev_name(dev), fn ?: pn); Is it that important to know which failed to be created, or that both failed to be created? David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |