Re: [char-misc-next v2 7/7] watchdog: mei_wdt: re-register device on event

[Guenter Roeck <linux@roeck-us.net>]

On 12/17/2015 06:49 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 registers the watchdog device with the kernel.
> In the same manner the feature can be deactivated without reboot
> in that case the watchdog device should be unregistered.
>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---

I am not really happy about the watchdog device appearing and disappearing
dynamically. This wreaks havoc with any standard watchdog application.

Isn't there a better way to handle this ? How about just registering the
watchdog device and return an error in the access functions if it is disabled ?

Thanks,
Guenter

> V2: rework un/registration in runtime
>
>   drivers/watchdog/mei_wdt.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c
> index 00d1b8e630b7..cd79920c473b 100644
> --- a/drivers/watchdog/mei_wdt.c
> +++ b/drivers/watchdog/mei_wdt.c
> @@ -87,6 +87,7 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state)
>    * @state: watchdog internal state
>    * @resp_required: ping required response
>    * @response: ping response
> + * @unregister: unregister worker
>    * @timeout: watchdog current timeout
>    *
>    * @dbgfs_dir: debugfs dir entry
> @@ -101,6 +102,7 @@ struct mei_wdt {
>   	enum mei_wdt_state state;
>   	bool resp_required;
>   	struct completion response;
> +	struct work_struct unregister;
>   	u16 timeout;
>
>   #if IS_ENABLED(CONFIG_DEBUG_FS)
> @@ -237,6 +239,13 @@ static void mei_wdt_unregister(struct mei_wdt *wdt)
>   	mutex_unlock(&wdt->reg_lock);
>   }
>
> +static void mei_wdt_unregister_work(struct work_struct *work)
> +{
> +	struct mei_wdt *wdt = container_of(work, struct mei_wdt, unregister);
> +
> +	mei_wdt_unregister(wdt);
> +}
> +
>   /**
>    * mei_wdt_register - register with the watchdog subsystem
>    *
> @@ -350,8 +359,13 @@ static void mei_wdt_event_rx(struct mei_cl_device *cldev)
>   		return;
>   	}
>
> -	if (wdt->state == MEI_WDT_RUNNING)
> +	if (wdt->state == MEI_WDT_RUNNING) {
> +		if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
> +			wdt->state = MEI_WDT_NOT_REQUIRED;
> +			schedule_work(&wdt->unregister);
> +		}
>   		goto out;
> +	}
>
>   	if (wdt->state == MEI_WDT_PROBE) {
>   		if (res.wdstate & MEI_WDT_WDSTATE_NOT_REQUIRED) {
> @@ -372,6 +386,21 @@ out:
>   		complete(&wdt->response);
>   }
>
> +/*
> + * mei_wdt_notify_event - callback for event notification
> + *
> + * @cldev: bus device
> + */
> +static void mei_wdt_notify_event(struct mei_cl_device *cldev)
> +{
> +	struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev);
> +
> +	if (wdt->state != MEI_WDT_NOT_REQUIRED)
> +		return;
> +	wdt->state = MEI_WDT_IDLE;
> +	mei_wdt_register(wdt);
> +}
> +
>   /**
>    * mei_wdt_event - callback for event receive
>    *
> @@ -384,6 +413,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_notify_event(cldev);
>   }
>
>   /**
> @@ -543,6 +575,7 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>   	wdt->resp_required = mei_cldev_ver(cldev) > 0x1;
>   	mutex_init(&wdt->reg_lock);
>   	wdt->is_registered = false;
> +	INIT_WORK(&wdt->unregister, mei_wdt_unregister_work);
>   	init_completion(&wdt->response);
>
>   	mei_cldev_set_drvdata(cldev, wdt);
> @@ -553,9 +586,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev,
>   		goto err_out;
>   	}
>
> -	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;
>   	}
> @@ -599,6 +636,8 @@ static int mei_wdt_remove(struct mei_cl_device *cldev)
>   	if (!completion_done(&wdt->response))
>   		complete(&wdt->response);
>
> +	cancel_work_sync(&wdt->unregister);
> +
>   	mei_wdt_unregister(wdt);
>
>   	kref_put(&wdt->refcnt, mei_wdt_release);
>