On Tue, 28 May 2019 09:35:48 +0800
Wei Yang <richardw.y...@linux.intel.com> wrote:
> On Mon, May 27, 2019 at 02:21:14PM +0200, Igor Mammedov wrote:
> >On Thu, 11 Apr 2019 15:17:39 +0800
> >Wei Yang <richardw.y...@linux.intel.com> wrote:
> >
> >> pc_memory_pre_plug() is called during hotplug for both pc-dimm and
> >> nvdimm. This is more proper to check apci hotplug capability before
> >> check nvdimm specific capability.
> >not sure what this about.
> >Currently we are checking if ACPI is enabled
> > if (!pcms->acpi_dev || !acpi_enabled) { ...
> >before nvdimm check and it looks better to me that we cancel
> >nvdimm hotplug earlier than passing it to
> > hotplug_handler_pre_plug(pcms->acpi_dev, dev, &local_err)
> >with this patch ACPI device handler will be called before
> >nvdimm check happens, so it's +1 unnecessary call chain in
> >the case of nvdimm, which I'd rather not have.
> >
> >Are there any issues with current call flow?
> >(commit message doesn't really explaining why we need this patch)
> >
>
> My idea is to check more generic requirement and then specific one.
>
> For example, the call flow looks like this:
>
> pc_memory_pre_plug
>
> piix4_device_pre_plug_cb | ich9_pm_device_pre_plug_cb
> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> !lpc->pm.acpi_memory_hotplug.is_enabled)
>
> if (is_nvdimm && !ms->nvdimms_state->is_enabled)
>
>
> In hotplug_handler_pre_plug(), it checks the acpi hotplug capability. And then
> if it has memory hotplug capability and is nvdimm, we check whether nvdimm is
> enabled.
I don't think pc_memory_pre_plug() should rely on what
hotplug_handler_pre_plug()
checks or does. Similarly the later is taking care of whatever piix4 needs to
care
and shouldn't care about what machine code does.
Moreover when hotplug_handler_pre_plug() starts to reserve resources, then
if you move check as suggested you'd need to rollback all that
hotplug_handler_pre_plug() done to gracefully abort hotplug.
So I'd leave the code as it is now, since it doesn't depend on concrete
hotplug_handler_pre_plug() implementation and won't break if
hotplug_handler_pre_plug() will start consuming resources (which could
happen and you won't even notice it since changed code is in piix4/q35
files when reviewing patches).
> This is why I suggest to change the order here. No functional issue for
> current code.
>
