> >>
> >> Any special reason for having two data structures instead of one ?
> >> You could just move the variables from struct mei_wdt_dev into
> >> struct mei_wdt, no ?
> >
> > Yes, on newer platform mei_wdt_dev might be not present in case the the
> > device is not provisioned. This came to action in the following
> > patches.
> >
>
> It is still an implementation choice to keep the data structures separate.
> That mei_wdt_dev can be unregistered doesn't mean that the data structure
> has to be destroyed or allocated on registration.
That's correct, the issue is that if the MEI device resets or go through
suspend/resume the mei_wdt will be removed destroyed
Why watchdog daemon will still holds open handler to watchdog_device and will
releases it upon ping failure. This is where reference counter will come in.
Currently we do not support seamless reset.
> >>> +
> >>> + /* valid value is already checked by the caller */
> >>> + wdt->timeout = timeout;
> >>> + wdd->timeout = timeout;
> >>
> >> One of those seems unnecessary. Why keep the timeout twice ?
> >
> > We need two as wdd may not exists and we still need to send ping to
> > detect if the device is provisioned.
> >
>
> Ok, makes sense.
>
> >>> +
> >>> +static void mei_wdt_unregister(struct mei_wdt *wdt)
> >>> +{
> >>> + if (!wdt->mwd)
> >>> + return;
> >>> +
> >>> + watchdog_unregister_device(&wdt->mwd->wdd);
> >>> + kref_put(&wdt->mwd->refcnt, mei_wdt_release);
> >>> + wdt->mwd = NULL;
>
> If setting this to NULL was really needed, you would have a race condition
> here:
> wdt->mwd is set to NULL only after the pointer it points to was freed, leaving
> a small window where the code above could still access it.
Yes, good catch will fix that.
>
> >>> +}
> >>
> >> Seems to me that using two separate data structures instead of one
> >> adds a lot of complexity.
> >
> > It might be reduced but I'm not sure it can be significantly simpler.
> > It the reference counter will be part of watchdog_device it would be
> > simpler.
> >
>
> It would be there if struct watchdog_device was allocated by the
> watchdog core, which is not the case. I am still trying to create
> a situation where the local data structure (struct mei_wdt in this case)
> can be released while the watchdog device is still open
> (ie how to unprovision the watchdog device while in use).
This happen on device suspend/resume or device reset, this is not the case of
provisioning.
Currently the our bus layer doesn't support seamless reconnection.
> Once I figure that out, I hope I can find a way to protect it
> differently and drop the ref/unref callbacks. I suspect it may
> be necessary to separate struct watchdog_device into two parts:
> one used by drivers and one used by the watchdog core.
> The watchdog core would then manage its own data structure, and
> no longer depend on struct watchdog_device (owned by the driver)
> to actually be there. Different story, though.
I think that would somehow fit also our driver.
Thanks
Tomas
