Re: [char-misc-next 6/6] mei: wd: re-register device on event

["Winkler, Tomas" <tomas.winkler@intel.com>]

On Mon, 2015-11-30 at 09:08 -0800, Guenter Roeck wrote:
> On 11/26/2015 04:31 AM, Tomas Winkler wrote:
> > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > 
> > For Intel SKL platform the ME device can inform the host via
> > asynchronous notification that the watchdog feature was activated
> > on the
> > device. The activation doesn't require reboot.
> > In that case the driver register the watchdog device with the
> > kernel.
> > 
> 
> Can the watchdog device also be disabled on the fly ?
Yes, it can be disabled also on the fly, but this is not delivered via
an event, this is handled via ping return value. 

> 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >   drivers/watchdog/mei_wdt.c | 26 ++++++++++++++++++++++++--
> >   1 file changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/watchdog/mei_wdt.c
> > b/drivers/watchdog/mei_wdt.c
> > index 47f0dc2e822a..ee3b5d12b1b2 100644
> > --- a/drivers/watchdog/mei_wdt.c
> > +++ b/drivers/watchdog/mei_wdt.c
> > @@ -279,6 +279,21 @@ out:
> >   		complete(&wdt->response);
> >   }
> > 
> > +/*
> > + * mei_wdt_event_notif - callback for notification
> > + *
> > + * @cldev: bus device
> > + */
> > +static void mei_wdt_event_notif(struct mei_cl_device *cldev)
> > +{
> > +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> > +
> > +	if (wdt->state == MEI_WDT_NOT_REQUIRED) {
> > +		mei_wdt_register(wdt);
> > +		wdt->state = MEI_WDT_IDLE;
> > +	}
> > +}
> > +
> >   /**
> >    * mei_wdt_event - callback for event receive
> >    *
> > @@ -291,6 +306,9 @@ static void mei_wdt_event(struct mei_cl_device
> > *cldev,
> >   {
> >   	if (events & BIT(MEI_CL_EVENT_RX))
> >   		mei_wdt_event_rx(cldev);
> > +
> > +	if (events & BIT(MEI_CL_EVENT_NOTIF))
> > +		mei_wdt_event_notif(cldev);
> >   }
> > 
> >   /**
> > @@ -466,9 +484,13 @@ static int mei_wdt_probe(struct mei_cl_device
> > *cldev,
> > 
> >   	wd_info.firmware_version = mei_cldev_ver(cldev);
> > 
> > -	ret = mei_cldev_register_event_cb(wdt->cldev,
> > BIT(MEI_CL_EVENT_RX),
> > +	ret = mei_cldev_register_event_cb(wdt->cldev,
> > +					  BIT(MEI_CL_EVENT_RX) |
> > +					  BIT(MEI_CL_EVENT_NOTIF),
> >   					  mei_wdt_event, NULL);
> > -	if (ret) {
> > +
> > +	/* on legacy devices notification is not supported */
> > +	if (ret && ret != -EOPNOTSUPP) {
> >   		dev_err(&cldev->dev, "Could not register event
> > ret=%d\n", ret);
> >   		goto err_disable;
> >   	}
> > 
> Doesn't that mean, though, that the RX event is not registered
> either,
> even if it is supported ? If so, should you retry and register only
> MEI_CL_EVENT_RX ?

The code is correct but we definitely need to think of better API 
Tomas