RE: [PATCH V2 2/2] Xen ACPI memory hotplug

[Liu, Jinsong]

Agree, w/ minor comments below.


Konrad Rzeszutek Wilk wrote:
>> +
>> +	/*
>> +	 * Early boot code has recognized memory area by EFI/E820.
>> +	 * If DSDT shows these memory devices on boot, hotplug is not
>> necessary +	 * for them. So, it just returns until completion of
>> this driver's +	 * start up.
> 
> "So it just returns until completion of this drivers's start up."
> 
> Can you change that to be:
> "Return OK until this driver starts up."
> 
> But then.. how can this function be called with
> acpi_hotmem_initialized=false? Is it b/c of the acpi_walk_namespace
> call? How about you state that: 
> 
> "This can be done via the acpi_walk_namespace which is called during
> early boot and acpi_hotmem_initialized is set _after_ that call
> has completed."
> 

The action for booting existed memory and hot-added memory is different.
For booting existed memory devices, we don't need do xen_hotadd_memory.
For hot-added memory devices, we need do xen_hotadd_memory, hypercall to hypervisor to add memory.

I've updated comments as below to be more clear:
        /*
         * For booting existed memory devices, early boot code has recognized
         * memory area by EFI/E820. If DSDT shows these memory devices on boot,
         * hotplug is not necessary for them.
         * For hot-added memory devices during runtime, it need hypercall to
         * Xen hypervisor to add memory.
         */


>> +static void __exit xen_acpi_memory_device_exit(void) +{
>> +	acpi_status status;
>> +
>> +	if (!xen_initial_domain())
>> +		return;
>> +
>> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
>> +				     ACPI_UINT32_MAX, +				    
>> acpi_memory_deregister_notify_handler, +				     NULL, NULL, NULL);
>> +	if (ACPI_FAILURE(status))
>> +		pr_warn(PREFIX "walk_namespace failed\n");
>> +
>> +	acpi_bus_unregister_driver(&xen_acpi_memory_device_driver); +
>> +	/*
>> +	 * stub reserve space again to prevent any chance of native
>> +	 * driver loading, though not much meaning in real life
> 
> not much meaning in real life? What does that mean?
> 

Hmm, what I want to say here is, in real life module driver seldomly unloaded.
Anyway, the comments is ambiguous, I've dropped the word 'though not much meaning in real life'.

Thanks,
Jinsong

>> +	 */
>> +	acpi_bus_register_driver(&xen_stub_memory_device_driver); +	return;
>> +}
>> +
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/