| Message ID | 4989A784.3030604@suse.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Frank Seidel wrote: > +++ b/drivers/acpi/pci_link.c > @@ -593,7 +593,7 @@ static int acpi_pci_link_allocate(struct > return -ENODEV; > } else { > acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING; > - printk(PREFIX "%s [%s] enabled at IRQ %d\n", > + printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n", > acpi_device_name(link->device), > acpi_device_bid(link->device), link->irq.active); > } > @@ -751,21 +751,21 @@ static int acpi_pci_link_add(struct acpi > acpi_device_bid(device)); > for (i = 0; i < link->irq.possible_count; i++) { > if (link->irq.active == link->irq.possible[i]) { > - printk(" *%d", link->irq.possible[i]); > + printk(KERN_INFO " *%d", link->irq.possible[i]); > found = 1; > } else > - printk(" %d", link->irq.possible[i]); > + printk(KERN_INFO " %d", link->irq.possible[i]); > } > > - printk(")"); > + printk(KERN_INFO ")"); > > if (!found) > - printk(" *%d", link->irq.active); > + printk(KERN_INFO " *%d", link->irq.active); > This patch looks broken to me, at least for some of your changes. For example, in the bit quoted above all printks together make up *one single* message, which means that only the _first_ of the printks should have the KERN_* prefix. printks that are continuations should not have the prefix. Cheers, FJP -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Frans Pop wrote: > This patch looks broken to me, at least for some of your changes. > For example, in the bit quoted above all printks together make up > *one single* message, which means that only the _first_ of the > printks should have the KERN_* prefix. printks that are continuations > should not have the prefix. Are you absolutely sure about that? The janitors todo lets me guess they should aswell me prefixed. Otherwise yes, then some of the changes wouldn't be needed. Thanks, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 4 Feb 2009, Frans Pop wrote: > Frank Seidel wrote: > > +++ b/drivers/acpi/pci_link.c > > @@ -593,7 +593,7 @@ static int acpi_pci_link_allocate(struct > > return -ENODEV; > > } else { > > acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING; > > - printk(PREFIX "%s [%s] enabled at IRQ %d\n", > > + printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n", > > acpi_device_name(link->device), > > acpi_device_bid(link->device), link->irq.active); > > } > > @@ -751,21 +751,21 @@ static int acpi_pci_link_add(struct acpi > > acpi_device_bid(device)); > > for (i = 0; i < link->irq.possible_count; i++) { > > if (link->irq.active == link->irq.possible[i]) { > > - printk(" *%d", link->irq.possible[i]); > > + printk(KERN_INFO " *%d", link->irq.possible[i]); > > found = 1; > > } else > > - printk(" %d", link->irq.possible[i]); > > + printk(KERN_INFO " %d", link->irq.possible[i]); > > } > > > > - printk(")"); > > + printk(KERN_INFO ")"); > > > > if (!found) > > - printk(" *%d", link->irq.active); > > + printk(KERN_INFO " *%d", link->irq.active); > > > > This patch looks broken to me, at least for some of your changes. > For example, in the bit quoted above all printks together make up > *one single* message, which means that only the _first_ of the > printks should have the KERN_* prefix. printks that are continuations > should not have the prefix. Actually they should, but the right prefix :-) Quoting include/linux/kernel.h: | /* | * Annotation for a "continued" line of log printout (only done after a | * line that had no enclosing \n). Only to be used by core/arch code | * during early bootup (a continued line is not SMP-safe otherwise). | */ | #define KERN_CONT "" Please also consider the note about SMP-safeness. With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone: +32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: Geert.Uytterhoeven@sonycom.com Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven wrote: > Actually they should, but the right prefix :-) > > Quoting include/linux/kernel.h: > ... Ah, no i see. And i missed the line (from janitors todo) that this should only be done for line beginnings (and now KERN_CONT for core/arch..). Thank you very much that pointer. Will keep it in mind for my further cleanups of this topic. Of course i will then also redo the above patch. Thanks, Frank -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 04 February 2009, Geert Uytterhoeven wrote: > > This patch looks broken to me, at least for some of your changes. > > For example, in the bit quoted above all printks together make up > > *one single* message, which means that only the _first_ of the > > printks should have the KERN_* prefix. printks that are continuations > > should not have the prefix. > > Actually they should, but the right prefix :-) Hmm. I was going by memory and from what I've seen in existing code, but also found this (somewhat old) post: https://lists.linux-foundation.org/pipermail/kernel-janitors/2005-October/014375.html > Quoting include/linux/kernel.h: > | /* > | * Annotation for a "continued" line of log printout (only done after a > | * line that had no enclosing \n). Only to be used by core/arch code > | * during early bootup (a continued line is not SMP-safe otherwise). > | */ > | #define KERN_CONT "" > > Please also consider the note about SMP-safeness. From that it looks like KERN_CONT should only be used in a very limited context, but I guess this example qualifies. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 4 Feb 2009, Frans Pop wrote: > On Wednesday 04 February 2009, Geert Uytterhoeven wrote: > > > This patch looks broken to me, at least for some of your changes. > > > For example, in the bit quoted above all printks together make up > > > *one single* message, which means that only the _first_ of the > > > printks should have the KERN_* prefix. printks that are continuations > > > should not have the prefix. > > > > Actually they should, but the right prefix :-) > > Hmm. I was going by memory and from what I've seen in existing code, but > also found this (somewhat old) post: > https://lists.linux-foundation.org/pipermail/kernel-janitors/2005-October/014375.html That one predates KERN_CONT. > > Quoting include/linux/kernel.h: > > | /* > > | * Annotation for a "continued" line of log printout (only done after a > > | * line that had no enclosing \n). Only to be used by core/arch code > > | * during early bootup (a continued line is not SMP-safe otherwise). > > | */ > > | #define KERN_CONT "" > > > > Please also consider the note about SMP-safeness. > > From that it looks like KERN_CONT should only be used in a very limited > context, but I guess this example qualifies. Yep, the general advise is to avoid using continuations. With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone: +32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: Geert.Uytterhoeven@sonycom.com Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/acpi/video.c +++ b/drivers/acpi/video.c @@ -1260,7 +1260,7 @@ static int acpi_video_bus_POST_info_seq_ printk(KERN_WARNING PREFIX "This indicates a BIOS bug. Please contact the manufacturer.\n"); } - printk("%llx\n", options); + printk(KERN_WARNING "%llx\n", options); seq_printf(seq, "can POST: <integrated video>"); if (options & 2) seq_printf(seq, " <PCI video>"); --- a/drivers/acpi/container.c +++ b/drivers/acpi/container.c @@ -163,7 +163,7 @@ static void container_notify_cb(acpi_han case ACPI_NOTIFY_BUS_CHECK: /* Fall through */ case ACPI_NOTIFY_DEVICE_CHECK: - printk("Container driver received %s event\n", + printk(KERN_WARNING "Container driver received %s event\n", (type == ACPI_NOTIFY_BUS_CHECK) ? "ACPI_NOTIFY_BUS_CHECK" : "ACPI_NOTIFY_DEVICE_CHECK"); status = acpi_bus_get_device(handle, &device); @@ -174,7 +174,8 @@ static void container_notify_cb(acpi_han kobject_uevent(&device->dev.kobj, KOBJ_ONLINE); else - printk("Failed to add container\n"); + printk(KERN_WARNING + "Failed to add container\n"); } } else { if (ACPI_SUCCESS(status)) { --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -400,12 +400,12 @@ int acpi_pci_irq_enable(struct pci_dev * dev_warn(&dev->dev, "PCI INT %c: no GSI", pin_name(pin)); /* Interrupt Line values above 0xF are forbidden */ if (dev->irq > 0 && (dev->irq <= 0xF)) { - printk(" - using IRQ %d\n", dev->irq); + printk(KERN_WARNING " - using IRQ %d\n", dev->irq); acpi_register_gsi(dev->irq, ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_LOW); return 0; } else { - printk("\n"); + printk(KERN_WARNING "\n"); return 0; } } --- a/drivers/acpi/pci_link.c +++ b/drivers/acpi/pci_link.c @@ -593,7 +593,7 @@ static int acpi_pci_link_allocate(struct return -ENODEV; } else { acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING; - printk(PREFIX "%s [%s] enabled at IRQ %d\n", + printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n", acpi_device_name(link->device), acpi_device_bid(link->device), link->irq.active); } @@ -751,21 +751,21 @@ static int acpi_pci_link_add(struct acpi acpi_device_bid(device)); for (i = 0; i < link->irq.possible_count; i++) { if (link->irq.active == link->irq.possible[i]) { - printk(" *%d", link->irq.possible[i]); + printk(KERN_INFO " *%d", link->irq.possible[i]); found = 1; } else - printk(" %d", link->irq.possible[i]); + printk(KERN_INFO " %d", link->irq.possible[i]); } - printk(")"); + printk(KERN_INFO ")"); if (!found) - printk(" *%d", link->irq.active); + printk(KERN_INFO " *%d", link->irq.active); if (!link->device->status.enabled) - printk(", disabled."); + printk(KERN_INFO ", disabled."); - printk("\n"); + printk(KERN_INFO "\n"); /* TBD: Acquire/release lock */ list_add_tail(&link->node, &acpi_link.entries); --- a/drivers/acpi/dock.c +++ b/drivers/acpi/dock.c @@ -984,7 +984,7 @@ static int dock_add(acpi_handle handle) ret = device_create_file(&dock_device->dev, &dev_attr_docked); if (ret) { - printk("Error %d adding sysfs file\n", ret); + printk(KERN_ERR "Error %d adding sysfs file\n", ret); platform_device_unregister(dock_device); kfree(dock_station); dock_station = NULL; @@ -992,7 +992,7 @@ static int dock_add(acpi_handle handle) } ret = device_create_file(&dock_device->dev, &dev_attr_undock); if (ret) { - printk("Error %d adding sysfs file\n", ret); + printk(KERN_ERR "Error %d adding sysfs file\n", ret); device_remove_file(&dock_device->dev, &dev_attr_docked); platform_device_unregister(dock_device); kfree(dock_station); @@ -1001,7 +1001,7 @@ static int dock_add(acpi_handle handle) } ret = device_create_file(&dock_device->dev, &dev_attr_uid); if (ret) { - printk("Error %d adding sysfs file\n", ret); + printk(KERN_ERR "Error %d adding sysfs file\n", ret); device_remove_file(&dock_device->dev, &dev_attr_docked); device_remove_file(&dock_device->dev, &dev_attr_undock); platform_device_unregister(dock_device); @@ -1011,7 +1011,7 @@ static int dock_add(acpi_handle handle) } ret = device_create_file(&dock_device->dev, &dev_attr_flags); if (ret) { - printk("Error %d adding sysfs file\n", ret); + printk(KERN_ERR "Error %d adding sysfs file\n", ret); device_remove_file(&dock_device->dev, &dev_attr_docked); device_remove_file(&dock_device->dev, &dev_attr_undock); device_remove_file(&dock_device->dev, &dev_attr_uid); --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -1861,9 +1861,9 @@ int __cpuinit acpi_processor_power_init( printk(KERN_INFO PREFIX "CPU%d (power states:", pr->id); for (i = 1; i <= pr->power.count; i++) if (pr->power.states[i].valid) - printk(" C%d[C%d]", i, + printk(KERN_INFO " C%d[C%d]", i, pr->power.states[i].type); - printk(")\n"); + printk(KERN_INFO ")\n"); #ifndef CONFIG_CPU_IDLE if (pr->id == 0) { --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -228,10 +228,10 @@ void acpi_os_vprintf(const char *fmt, va if (acpi_in_debugger) { kdb_printf("%s", buffer); } else { - printk("%s", buffer); + printk(KERN_WARNING "%s", buffer); } #else - printk("%s", buffer); + printk(KERN_WARNING "%s", buffer); #endif } --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -742,8 +742,9 @@ static int __cpuinit acpi_processor_star if (pr->flags.throttling) { printk(KERN_INFO PREFIX "%s [%s] (supports", acpi_device_name(device), acpi_device_bid(device)); - printk(" %d throttling states", pr->throttling.state_count); - printk(")\n"); + printk(KERN_INFO + " %d throttling states", pr->throttling.state_count); + printk(KERN_INFO ")\n"); } end: --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -679,7 +679,7 @@ static void acpi_power_off_prepare(void) static void acpi_power_off(void) { /* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */ - printk("%s called\n", __func__); + printk(KERN_DEBUG "%s called\n", __func__); local_irq_disable(); acpi_enable_wakeup_device(ACPI_STATE_S5); acpi_enter_sleep_state(ACPI_STATE_S5); @@ -706,7 +706,7 @@ int __init acpi_sleep_init(void) status = acpi_get_sleep_type_data(i, &type_a, &type_b); if (ACPI_SUCCESS(status)) { sleep_states[i] = 1; - printk(" S%d", i); + printk(KERN_INFO " S%d", i); } } @@ -720,7 +720,7 @@ int __init acpi_sleep_init(void) hibernation_set_ops(old_suspend_ordering ? &acpi_hibernation_ops_old : &acpi_hibernation_ops); sleep_states[ACPI_STATE_S4] = 1; - printk(" S4"); + printk(KERN_INFO " S4"); if (!nosigcheck) { acpi_get_table(ACPI_SIG_FACS, 1, (struct acpi_table_header **)&facs); @@ -733,11 +733,11 @@ int __init acpi_sleep_init(void) status = acpi_get_sleep_type_data(ACPI_STATE_S5, &type_a, &type_b); if (ACPI_SUCCESS(status)) { sleep_states[ACPI_STATE_S5] = 1; - printk(" S5"); + printk(KERN_INFO " S5"); pm_power_off_prepare = acpi_power_off_prepare; pm_power_off = acpi_power_off; } - printk(")\n"); + printk(KERN_INFO ")\n"); /* * Register the tts_notifier to reboot notifier list so that the _TTS * object can also be evaluated when the system enters S5.