linux-kernel - Re: [PATCH v3 2/4] ACPI: Update CPU hotplug messages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 27 Jul 2012 10:05:13 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Toshi Kani <toshi.kani@...com>
Cc:	lenb@...nel.org, linux-acpi@...r.kernel.org,
	linux-kernel@...r.kernel.org, joe@...ches.com,
	isimatu.yasuaki@...fujitsu.com, liuj97@...il.com,
	srivatsa.bhat@...ux.vnet.ibm.com, prarit@...hat.com,
	imammedo@...hat.com, vijaymohan.pandarathil@...com
Subject: Re: [PATCH v3 2/4] ACPI: Update CPU hotplug messages

On Thu, Jul 26, 2012 at 8:39 PM, Toshi Kani <toshi.kani@...com> wrote:
> On Thu, 2012-07-26 at 13:23 -0600, Bjorn Helgaas wrote:
>> On Wed, Jul 25, 2012 at 5:12 PM, Toshi Kani <toshi.kani@...com> wrote:

>> > @@ -560,8 +565,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>> >          */
>> >         if (per_cpu(processor_device_array, pr->id) != NULL &&
>> >             per_cpu(processor_device_array, pr->id) != device) {
>> > -               printk(KERN_WARNING "BIOS reported wrong ACPI id "
>> > -                       "for the processor\n");
>> > +               pr_warn("BIOS reported wrong ACPI id for the processor\n");
>>
>> And this.
>
> Changed to use dev_warn().

Is there additional information you could print here, like the pr->id?
 I don't understand the data structures here, so maybe there isn't.

>> > @@ -727,17 +731,19 @@ static void acpi_processor_hotplug_notify(acpi_handle handle,
>> >                                   "received ACPI_NOTIFY_EJECT_REQUEST\n"));
>> >
>> >                 if (acpi_bus_get_device(handle, &device)) {
>> > -                       pr_err(PREFIX "Device don't exist, dropping EJECT\n");
>> > +                       acpi_pr_err(handle,
>> > +                               "Device don't exist, dropping EJECT\n");
>> >                         break;
>> >                 }
>> >                 if (!acpi_driver_data(device)) {
>> > -                       pr_err(PREFIX "Driver data is NULL, dropping EJECT\n");
>> > +                       acpi_pr_err(handle,
>> > +                               "Driver data is NULL, dropping EJECT\n");
>>
>> And this.
>
> No change since it is called directly from the handler.

True, but by this point, we have a valid acpi_device *, don't we?  We
called acpi_driver_data(device), which requires "device" to be valid.

>> >                         break;
>> >                 }
>> >
>> >                 ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
>> >                 if (!ej_event) {
>> > -                       pr_err(PREFIX "No memory, dropping EJECT\n");
>> > +                       acpi_pr_err(handle, "No memory, dropping EJECT\n");
>>
>> And this.
>
> No change since it is called directly from the handler.
>
>> >                         break;
>> >                 }
>> >
>> > @@ -847,7 +853,7 @@ static acpi_status acpi_processor_hotadd_init(struct acpi_processor *pr)
>> >          * and do it when the CPU gets online the first time
>> >          * TBD: Cleanup above functions and try to do this more elegant.
>> >          */
>> > -       printk(KERN_INFO "CPU %d got hotplugged\n", pr->id);
>> > +       pr_info("CPU %d got hotplugged\n", pr->id);
>>
>> And this.  The caller (acpi_processor_get_info()) has an acpi_device
>> *, so we should be able to use it here.
>
> I think pr_info() is fine since it is a normal message and already has
> CPU number in the message.

Is there another message that correlates the device name
("ACPI0007:xx") with the CPU number?  That correlation seems useful.
My mindset is that a driver should *always* use dev_<level>() when
possible, but I won't belabor the point.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/