On Wed, May 29, 2019 at 10:57:50AM +0200, Igor Mammedov wrote: >On Wed, 29 May 2019 08:32:14 +0800 >Wei Yang <richardw.y...@linux.intel.com> wrote: > >> On Tue, May 28, 2019 at 02:26:27PM +0200, Igor Mammedov wrote: >> >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. >> > >> >> Agree. It is not proper to let hotplug_handler_pre_plug() take care about >> machine code. >> >> >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. >> > >> >> Confused. >> >> hotplug_handler_pre_plug() doesn't reserve resources. > > >it's not currently, but if it would it would not work with your patch properly >or break unexpectedly since whoever would change hotplug_handler_pre_plug() >might not notice that machine code need to be taken care of. > >Try to consider devices and machine as separate libraries. Which should >in reasonable limits be independent and work through documented interfaces. >In that case likehood of breaking something would be less than relying on >current code impl./call order with implicit inter-dependencies. >
So the logic here is check machine then device, right? I think this is reasonable. To be honest, this rule is not that obvious. Anyway, I think what you mentioned make sense. Thanks -- Wei Yang Help you, Help me