On 14.06.2018 17:00, Igor Mammedov wrote: > On Wed, 13 Jun 2018 16:50:54 +0200 > David Hildenbrand <da...@redhat.com> wrote: > >> On 13.06.2018 16:07, David Hildenbrand wrote: >>> On 13.06.2018 15:03, Igor Mammedov wrote: >>>> On Mon, 11 Jun 2018 14:16:51 +0200 >>>> David Hildenbrand <da...@redhat.com> wrote: >>>> >>>>> We already verify when realizing that the memdev property has been >>>>> set. We have no more accesses to get_memory_region() before the device >>>>> is realized. >>>> this stems from assumption that get_memory_region shouldn't be called >>>> before devices realize is executed, which I don't agree to begin with. >>>> >>>> However wrt error handling, we should probably leave device internal error >>>> up to devices and make check for error only in pre_plug handler >>>> (since pre_plug was successful, the rest machine helpers would have >>>> access to the same region until device is removed. >>>> >>> >>> Something like a generic Device "validate()"/"pre_realize()" function, >>> that can be called before realize() and validates all properties >>> (+initializes derived properties) would be something I could agree to. >>> >>> Then we could drop all error handling from access functions (as they >>> have been validated early during pre_plug()) >>> >>> Moving all checks out of realize into pre_plug() looks ugly, because we >>> have implementation details split across c-files. >>> >> >> I am thinking about something like this >> >> From ee8da4f5774ff5fa190466674e36ee99020033c4 Mon Sep 17 00:00:00 2001 >> From: David Hildenbrand <da...@redhat.com> >> Date: Wed, 13 Jun 2018 16:41:12 +0200 >> Subject: [PATCH RFC] device: add "prepare()" function >> >> The realize() function usually does several things. It validates >> properties, inititalizes state based on properties and performs >> e.g. registrations in the system that require "unrealize()" to be called >> when removing the device. >> >> In a pre_plug hotplug handler, we usually want to access certain >> properties or derived properties, e.g. to perform advanced checks >> (for resource asignment). Now we have the problem, that realize() was >> not called yet once we reach pre_plug(). So we theoretically have to >> duplicate checks and add error paths for cases that can >> theoretically never happen. > I care less about duplicate checks in completely different parts of code, > and I'd even insist on device:realize checks being self contained and not > rely on any other external checks performed by users of device. And vice > verse layer that uses device should do it's own checks when necessary > and not rely on device's verification. That way loosely coupled code > wouldn't fall apart whenever we drop or change checks in one of the parts. > > The only case where I'd make concession is minimizing error handling > on hot path for performance reasons and this is not the case here. > >> Let's add the "prepare()" callback, which can be used to validate >> properties and inititalize some state based on these. It will be called >> once all static properties have been inititalized, but before the >> pre_plug hotplug handler is activated. The pre_plug handler can than >> rely on the fact that basic checks already were performed. > > pre_plug isn't part of device, it's a separate part that might vary > depending on machine and which might modify device properties along > the way and then exaggerating we would need 'prepare2()' and after > that 'pre_plug2()' and ...
That's how two parties (device vs hotplug handler) work together to get results done ... Just like inititalize() -> realized() vs. pre_plug -> plug(). There has to be some hand shaking. > > Hence I dislike idea of adding more callbacks. I'd rather have a property > setter do the job of initializing state of device when necessary instead > of adding more callbacks. Which is in nvdimm case a bit more code compared > to doing it in realize() but should be possible to implement. Although I strongly disagree (especially about the fact of calling class functions *anytime* after a device has been created but not initialized) I will implement it like that for now (otherwise I won't be making any progress here but instead be creating more and more problems to solve). nvdimm will only have static properties. When realizing or when calling get_memory_region(), properties will be checked and the nvdimm_mr inititalized, if not already done. -- Thanks, David / dhildenb
