From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751623AbbLVFSa (ORCPT ); Tue, 22 Dec 2015 00:18:30 -0500 Received: from bh-25.webhostbox.net ([208.91.199.152]:45291 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750705AbbLVFS0 (ORCPT ); Tue, 22 Dec 2015 00:18:26 -0500 Subject: Re: [char-misc-next v3 6/8] watchdog: mei_wdt: register wd device only if required To: Tomas Winkler , Greg Kroah-Hartman , Wim Van Sebroeck References: <1450739881-3923-1-git-send-email-tomas.winkler@intel.com> <1450739881-3923-7-git-send-email-tomas.winkler@intel.com> Cc: Alexander Usyskin , linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org From: Guenter Roeck Message-ID: <5678DD1F.6080300@roeck-us.net> Date: Mon, 21 Dec 2015 21:18:23 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1450739881-3923-7-git-send-email-tomas.winkler@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/21/2015 03:17 PM, Tomas Winkler wrote: > From: Alexander Usyskin > > For Intel Broadwell and newer platforms, the ME device can inform > the host whether the watchdog functionality is activated or not. > If the watchdog functionality is not activated then the watchdog interface > can be not registered and eliminate unnecessary pings and hence lower the > power consumption by avoiding waking up the device. > The feature can be deactivated also without reboot > in that case the watchdog device should be unregistered at runtime. > > Signed-off-by: Alexander Usyskin > Signed-off-by: Tomas Winkler > --- > V2: rework unregistration > V3: rebase; implement unregistraion also at runtime > > drivers/watchdog/mei_wdt.c | 183 +++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 169 insertions(+), 14 deletions(-) > > diff --git a/drivers/watchdog/mei_wdt.c b/drivers/watchdog/mei_wdt.c > index ab9aec218d69..3cd80aa75db1 100644 > --- a/drivers/watchdog/mei_wdt.c > +++ b/drivers/watchdog/mei_wdt.c > @@ -16,6 +16,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -38,24 +39,30 @@ > > /* Sub Commands */ > #define MEI_MC_START_WD_TIMER_REQ 0x13 > +#define MEI_MC_START_WD_TIMER_RES 0x83 > +#define MEI_WDT_STATUS_SUCCESS 0 > +#define MEI_WDT_WDSTATE_NOT_REQUIRED 0x1 > #define MEI_MC_STOP_WD_TIMER_REQ 0x14 > > /** > * enum mei_wdt_state - internal watchdog state > * > + * @MEI_WDT_PROBE: wd in probing stage > * @MEI_WDT_IDLE: wd is idle and not opened > * @MEI_WDT_START: wd was opened, start was called > * @MEI_WDT_RUNNING: wd is expecting keep alive pings > * @MEI_WDT_STOPPING: wd is stopping and will move to IDLE > + * @MEI_WDT_NOT_REQUIRED: wd device is not required > */ > enum mei_wdt_state { > + MEI_WDT_PROBE, > MEI_WDT_IDLE, > MEI_WDT_START, > MEI_WDT_RUNNING, > MEI_WDT_STOPPING, > + MEI_WDT_NOT_REQUIRED, > }; > > -#if IS_ENABLED(CONFIG_DEBUG_FS) > static const char *mei_wdt_state_str(enum mei_wdt_state state) > { > switch (state) { > @@ -71,7 +78,6 @@ static const char *mei_wdt_state_str(enum mei_wdt_state state) > return "unknown"; > } > } > -#endif /* CONFIG_DEBUG_FS */ > > struct mei_wdt; > > @@ -94,6 +100,10 @@ struct mei_wdt_dev { > * > * @cldev: mei watchdog client device > * @state: watchdog internal state > + * @resp_required: ping required response > + * @response: ping response completion > + * @unregister: unregister worker > + * @reg_lock: watchdog device registration lock > * @timeout: watchdog current timeout > * > * @dbgfs_dir: debugfs dir entry > @@ -103,6 +113,10 @@ struct mei_wdt { > > struct mei_cl_device *cldev; > enum mei_wdt_state state; > + bool resp_required; > + struct completion response; > + struct work_struct unregister; > + struct mutex reg_lock; > u16 timeout; > > #if IS_ENABLED(CONFIG_DEBUG_FS) > @@ -139,6 +153,19 @@ struct mei_wdt_start_request { > } __packed; > > /** > + * struct mei_wdt_start_response watchdog start/ping response > + * > + * @hdr: Management Control Command Header > + * @status: operation status > + * @wdstate: watchdog status bit mask > + */ > +struct mei_wdt_start_response { > + struct mei_mc_hdr hdr; > + u8 status; > + u8 wdstate; > +} __packed; > + > +/** > * struct mei_wdt_stop_request - watchdog stop > * > * @hdr: Management Control Command Header > @@ -277,13 +304,18 @@ static int mei_wdt_ops_ping(struct watchdog_device *wdd) > if (wdt->state != MEI_WDT_START && wdt->state != MEI_WDT_RUNNING) > return 0; > > + if (wdt->resp_required) > + init_completion(&wdt->response); > + > + wdt->state = MEI_WDT_RUNNING; > ret = mei_wdt_ping(wdt); > if (ret) > return ret; > > - wdt->state = MEI_WDT_RUNNING; The state is now set to RUNNING even if the ping failed. Is that on purpose ? > + if (wdt->resp_required) > + ret = wait_for_completion_killable(&wdt->response); > > - return 0; > + return ret; > } > > /** > @@ -358,15 +390,22 @@ static struct watchdog_info wd_info = { > */ > static void mei_wdt_unregister(struct mei_wdt *wdt) > { > - struct mei_wdt_dev *mwd = wdt->mwd; > + struct mei_wdt_dev *mwd; > > - if (!mwd) > - return; > + mutex_lock(&wdt->reg_lock); > + > + if (!wdt->mwd) > + goto out; > + Is this because the error on registration was ignored ? Trying to understand the rationale for this check. > + mwd = wdt->mwd; > > watchdog_unregister_device(&mwd->wdd); > + It would be better to make such changes in an earlier patch and avoid the whitespace change here. > wdt->mwd = NULL; > - wdt->state = MEI_WDT_IDLE; > kref_put(&mwd->refcnt, mei_wdt_release); > + > +out: > + mutex_unlock(&wdt->reg_lock); > } > > /** > @@ -387,9 +426,13 @@ static int mei_wdt_register(struct mei_wdt *wdt) > > dev = &wdt->cldev->dev; > > + mutex_lock(&wdt->reg_lock); > + > mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL); > - if (!mwd) > - return -ENOMEM; > + if (!mwd) { > + ret = -ENOMEM; > + goto out; > + } > > mwd->wdt = wdt; > mwd->wdd.info = &wd_info; > @@ -405,13 +448,104 @@ static int mei_wdt_register(struct mei_wdt *wdt) > if (ret) { > dev_err(dev, "unable to register watchdog device = %d.\n", ret); > kref_put(&mwd->refcnt, mei_wdt_release); > - return ret; > + goto out; > } > > wdt->mwd = mwd; > +out: > + mutex_unlock(&wdt->reg_lock); > return 0; Do you want to return ret here ? Otherwise some of the code above does not really make sense (ie setting ret). > } > > +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_event_rx - callback for data receive > + * > + * @cldev: bus device > + */ > +static void mei_wdt_event_rx(struct mei_cl_device *cldev) > +{ > + struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev); > + struct mei_wdt_start_response res; > + const size_t res_len = sizeof(res); > + int ret; > + > + ret = mei_cldev_recv(wdt->cldev, (u8 *)&res, res_len); > + if (ret < 0) { > + dev_err(&cldev->dev, "failure in recv %d\n", ret); Not objecting, just concerned. Can those error messages result in filling up the kernel log if the mei hardware has a problem ? From an operational perspective, is it acceptable to ignore the errors ? > + return; > + } > + > + /* Empty response can be sent on stop */ > + if (ret == 0) > + return; > + > + if (ret < sizeof(struct mei_mc_hdr)) { > + dev_err(&cldev->dev, "recv small data %d\n", ret); > + return; > + } > + > + if (res.hdr.command != MEI_MANAGEMENT_CONTROL || > + res.hdr.versionnumber != MEI_MC_VERSION_NUMBER) { > + dev_err(&cldev->dev, "wrong command received\n"); > + return; > + } > + > + if (res.hdr.subcommand != MEI_MC_START_WD_TIMER_RES) { > + dev_warn(&cldev->dev, "unsupported command %d :%s[%d]\n", > + res.hdr.subcommand, > + mei_wdt_state_str(wdt->state), > + wdt->state); > + return; > + } > + > + 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) { > + wdt->state = MEI_WDT_NOT_REQUIRED; > + } else { > + /* stop the ping register watchdog device */ Should this be "stop the watchdog" ? > + mei_wdt_stop(wdt); > + mei_wdt_register(wdt); > + } > + return; > + } > + > + dev_warn(&cldev->dev, "not in correct state %s[%d]\n", > + mei_wdt_state_str(wdt->state), wdt->state); > + > +out: > + if (!completion_done(&wdt->response)) > + complete(&wdt->response); > +} > + > +/** > + * mei_wdt_event - callback for event receive > + * > + * @cldev: bus device > + * @events: event mask > + * @context: callback context > + */ > +static void mei_wdt_event(struct mei_cl_device *cldev, > + u32 events, void *context) > +{ > + if (events & BIT(MEI_CL_EVENT_RX)) > + mei_wdt_event_rx(cldev); > +} > + > #if IS_ENABLED(CONFIG_DEBUG_FS) > > static ssize_t mei_dbgfs_read_state(struct file *file, char __user *ubuf, > @@ -482,8 +616,13 @@ static int mei_wdt_probe(struct mei_cl_device *cldev, > return -ENOMEM; > > wdt->timeout = MEI_WDT_DEFAULT_TIMEOUT; > - wdt->state = MEI_WDT_IDLE; > + wdt->state = MEI_WDT_PROBE; > wdt->cldev = cldev; > + wdt->resp_required = mei_cldev_ver(cldev) > 0x1; > + mutex_init(&wdt->reg_lock); > + init_completion(&wdt->response); > + INIT_WORK(&wdt->unregister, mei_wdt_unregister_work); > + > mei_cldev_set_drvdata(cldev, wdt); > > ret = mei_cldev_enable(cldev); > @@ -492,9 +631,19 @@ 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), > + mei_wdt_event, NULL); > + if (ret) { > + dev_err(&cldev->dev, "Could not register event ret=%d\n", ret); > + goto err_disable; > + } > + Can there be an event before the call to mei_wdt_register() ? If so, it that case handled correctly ? > wd_info.firmware_version = mei_cldev_ver(cldev); > > - ret = mei_wdt_register(wdt); > + if (wdt->resp_required) > + ret = mei_wdt_ping(wdt); > + else > + ret = mei_wdt_register(wdt); > if (ret) > goto err_disable; > > @@ -515,6 +664,12 @@ static int mei_wdt_remove(struct mei_cl_device *cldev) > { > struct mei_wdt *wdt = mei_cldev_get_drvdata(cldev); > > + /* Free the caller in case of fw initiated or unexpected reset */ > + if (!completion_done(&wdt->response)) > + complete(&wdt->response); > + > + cancel_work_sync(&wdt->unregister); > + > mei_wdt_unregister(wdt); > can there be an event callback after mei_wdt_unregister() but before mei_cldev_disable() ? If so, is the situation handled or would it result in a race condition ? [ I think it is ok in both cases, just trying to make sure I didn't miss anything ] > mei_cldev_disable(cldev); > @@ -530,7 +685,7 @@ static int mei_wdt_remove(struct mei_cl_device *cldev) > 0x89, 0x9D, 0xA9, 0x15, 0x14, 0xCB, 0x32, 0xAB) > > static struct mei_cl_device_id mei_wdt_tbl[] = { > - { .uuid = MEI_UUID_WD, .version = 0x1}, > + { .uuid = MEI_UUID_WD, .version = MEI_CL_VERSION_ANY }, > /* required last entry */ > { } > }; >