Am 14.09.18 um 16:34 schrieb Igor Mammedov: > On Thu, 13 Sep 2018 14:20:25 +0200 > Igor Mammedov <imamm...@redhat.com> wrote: > >> On Wed, 29 Aug 2018 17:36:09 +0200 >> David Hildenbrand <da...@redhat.com> wrote: >> >>> To factor out plugging and unplugging of memory device we need access to >>> the memory region. So let's replace get_region_size() by >>> get_memory_region(). > this part isn't clear and doesn't really answer "why" patch's been written > and how it will really help. >
Sure, I can add more details. >>> >>> If any memory device will in the future have multiple memory regions >>> that make up the total region size, it can simply use a wrapping memory >>> region to embed the other ones.> It might be better to document expectation >>> as part of get_memory_region() doc comment. Yes, I will extend that, too. > > > I'm not really getting reasoning behind this patch. > * Why get_region_size() doesn't work for you? > benefits I see is that's one less get_foo_size() callback, > so it becomes less confusing get_region_size() size is no longer necessary when factoring pre_plug/plug_unplug out. Especially we need in addition of the size: 1. the alignment of the region (patch #9) 2. the region for memory_region_add_subregion() to plug it (patch #10) 3. the region for memory_region_del_subregion() to unplug it (patch #11) > > * I get we might need access to memory region at MemoryDeviceClass level, > but why are you keeping PCDIMMDeviceClass::get_memory_region()? > I'd expect the later being generalized (moved) to MemoryDeviceClass > so we would end up with one level indirection that we have now. > Yes, this is the main reason behind it. I guess getting rid of PCDIMMDeviceClass::get_memory_region() makes it clear that this is the next step of factoring out. I could turn this patch into said "factor get_memory_region() out" patch and keep get_region_size() for now. Thanks! -- Thanks, David / dhildenb
