On 13.06.2018 13:01, Igor Mammedov wrote: > On Mon, 11 Jun 2018 14:16:49 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> This might look like a step backwards, but it is not. get_memory_region() >> should not be called on uninititalized devices. In general, only >> properties should be access, but no "derived" satte like the memory >> region. >> >> 1. We need duplicate error checks if memdev is actually already set. >> realize() performs these checks, no need to duplicate. > it's not duplicate, if a machine doesn't access to memory region > in preplug time (hence doesn't check), then device itself would check it, > check won't be missed by accident. > (yep it's more code but more robust at the same time, so I'd leave it as is)
Checking at two places for the same thing == duplicate checks > >> 2. This is bad practise as one can see when looking at the NVDIMM >> implemetation. The call does not return sane data before the device >> is realized. Although spapr does not use NVDIMM, conceptually it is >> wrong. >> >> So let's just move this call to the right place. We can then cleanup >> get_memory_region(). > So I have to say no to this particular patch. > It is indeed a step backwards and it looks like workaround for broken nvdimm > impl. > > Firstly, memdev property must be set for dimm device and > a user accessing memory region first must check for error. > More details below. > You assume that any class function can be called at any time. And I don't think this is the way to go. -- Thanks, David / dhildenb
