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

[Toshi Kani <toshi.kani@hp.com>]

On Fri, 2012-07-27 at 10:05 -0600, Bjorn Helgaas wrote:
> On Thu, Jul 26, 2012 at 8:39 PM, Toshi Kani <toshi.kani@hp.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@hp.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.

Good point.  Yes, we should print pr->id so that we can check if the id
value is wrong.  I will make this change later since I would likely
touch this file again. :)  For now, I'd like to settle the patches
unless there is a major issue.

> >> > @@ -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.

Right.  But we need to print ACPI device path in order to be able to
analyze from the log file.  Hence, I am keeping acpi_pr_<level> in the
error paths of the handler itself.  Any subsequent functions call
dev_<level>() when device is valid.  In this particular case,
acpi_driver_data() does not call dev_<level>() when its return value is
NULL, but most other cases are changed to call dev_<level>().

> >> >                         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.

That's a good point.  This CPU number is used for cpu# file
under /sys/devices/system/cpu, but I do not think there is a good way to
correlate this number to the device name.  This is also the case for all
CPUs launched at boot-time.  At boot-time, all CPUs are launched from
the MADT table, and the code has no knowledge about processor objects.
Typically, cpu# and device instance# are same at boot-time, though.  I
will think about this issue further.

Thanks,
-Toshi

> 
> Bjorn