lkml.org 
[lkml]   [2015]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [char-misc-next 3/6] mei: wd: implement MEI iAMT watchdog driver
From
Date
On 12/01/2015 03:55 AM, Winkler, Tomas wrote:
[ ... ]

>>> +/**
>>> + * struct mei_wdt_dev - watchdog device wrapper
>>> + *
>>> + * @wdd: watchdog device
>>> + * @wdt: back pointer to mei_wdt driver
>>> + * @refcnt: reference counter
>>> + */
>>> +struct mei_wdt_dev {
>>> + struct watchdog_device wdd;
>>> + struct mei_wdt *wdt;
>>> + struct kref refcnt;
>>> +};
>>> +
>>> +/**
>>> + * struct mei_wdt - mei watchdog driver
>>> + *
>>> + * @cldev: mei watchdog client device
>>> + * @mwd: watchdog device wrapper
>>> + * @state: watchdog internal state
>>> + * @timeout: watchdog current timeout
>>> + */
>>> +struct mei_wdt {
>>> + struct mei_cl_device *cldev;
>>> + struct mei_wdt_dev *mwd;
>>> + enum mei_wdt_state state;
>>> + u16 timeout;
>>> +};
>>
>> 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.

>>> +
>>> +struct mei_wdt_hdr {
>>> + u8 command;
>>> + u8 bytecount;
>>> + u8 subcommand;
>>> + u8 versionnumber;
>>> +};
>>> +
>>> +struct mei_wdt_start_request {
>>> + struct mei_wdt_hdr hdr;
>>> + u16 timeout;
>>> + u8 reserved[17];
>>> +} __packed;
>>> +
>>> +struct mei_wdt_stop_request {
>>> + struct mei_wdt_hdr hdr;
>>> +} __packed;
>>> +
>>> +/**
>>> + * mei_wdt_ping - send wd start command
>>> + *
>>> + * @wdt: mei watchdog device
>>> + *
>>> + * Return: number of bytes sent on success,
>>> + * negative errno code on failure
>>> + */
>>> +static int mei_wdt_ping(struct mei_wdt *wdt)
>>> +{
>>> + struct mei_wdt_start_request req;
>>> + const size_t req_len = sizeof(req);
>>> +
>>> + memset(&req, 0, req_len);
>>> + req.hdr.command = MEI_MANAGEMENT_CONTROL;
>>> + req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
>>> subcommand);
>>> + req.hdr.subcommand = MEI_MC_START_WD_TIMER_REQ;
>>> + req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
>>> + req.timeout = wdt->timeout;
>>> +
>>> + return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_stop - send wd stop command
>>> + *
>>> + * @wdt: mei watchdog device
>>> + *
>>> + * Return: number of bytes sent on success,
>>> + * negative errno code on failure
>>> + */
>>> +static int mei_wdt_stop(struct mei_wdt *wdt)
>>> +{
>>> + struct mei_wdt_stop_request req;
>>> + const size_t req_len = sizeof(req);
>>> +
>>> + memset(&req, 0, req_len);
>>> + req.hdr.command = MEI_MANAGEMENT_CONTROL;
>>> + req.hdr.bytecount = req_len - offsetof(struct mei_wdt_hdr,
>>> subcommand);
>>> + req.hdr.subcommand = MEI_MC_STOP_WD_TIMER_REQ;
>>> + req.hdr.versionnumber = MEI_MC_VERSION_NUMBER;
>>> +
>>> + return mei_cldev_send(wdt->cldev, (u8 *)&req, req_len);
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_start - wd start command from the watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + *
>>> + * Return: 0 on success or -ENODEV;
>>> + */
>>> +static int mei_wdt_ops_start(struct watchdog_device *wdd)
>>> +{
>>> + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +
>>> + if (!mwd)
>>> + return -ENODEV;
>>
>> This can only happen because you call watchdog_set_drvdata() after
>> watchdog device registration. If that happens, the system is in
>> really bad shape.
>
> mei_wdt_dev can destroyed during
> driver operation if the device is unprovisioned, but still you the
> condition should not happen unless we have a bug. We can put WARN_ON()
> there.
>

The calling code should take care of that and not call those functions
after unregistration. More on that below.

>>
>> I would suggest to move the call to watchdog_set_drvdata() ahead
>> of watchdog_register_device() and drop those checks.
>>
>>> +
>>> + mwd->wdt->state = MEI_WDT_START;
>>> + wdd->timeout = mwd->wdt->timeout;
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_stop - wd stop command from the watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + *
>>> + * Return: 0 if success, negative errno code for failure
>>> + */
>>> +static int mei_wdt_ops_stop(struct watchdog_device *wdd)
>>> +{
>>> + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> + struct mei_wdt *wdt;
>>> + int ret;
>>> +
>>> + if (!mwd)
>>> + return -ENODEV;
>>> +
>>> + wdt = mwd->wdt;
>>> +
>>> + if (wdt->state != MEI_WDT_RUNNING)
>>> + return 0;
>>> +
>>> + wdt->state = MEI_WDT_STOPPING;
>>> +
>>> + ret = mei_wdt_stop(wdt);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + wdt->state = MEI_WDT_IDLE;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_ping - wd ping command from the watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + *
>>> + * Return: 0 if success, negative errno code on failure
>>> + */
>>> +static int mei_wdt_ops_ping(struct watchdog_device *wdd)
>>> +{
>>> + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> + struct mei_wdt *wdt;
>>> + int ret;
>>> +
>>> + if (!mwd)
>>> + return -ENODEV;
>>> +
>>> + wdt = mwd->wdt;
>>> +
>>> + if (wdt->state != MEI_WDT_START &&
>>> + wdt->state != MEI_WDT_RUNNING)
>>
>> Unnecessary continuation line ?
> Looks more readable to me. but we can also straight the condition.
>>
>>> + return 0;
>>> +
>>> + ret = mei_wdt_ping(wdt);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + wdt->state = MEI_WDT_RUNNING;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * mei_wdt_ops_set_timeout - wd set timeout command from the
>>> watchdog core.
>>> + *
>>> + * @wdd: watchdog device
>>> + * @timeout: timeout value to set
>>> + *
>>> + * Return: 0 if success, negative errno code for failure
>>> + */
>>> +static int mei_wdt_ops_set_timeout(struct watchdog_device *wdd,
>>> + unsigned int timeout)
>>> +{
>>> + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> + struct mei_wdt *wdt;
>>> +
>>> + if (!mwd)
>>> + return -ENODEV;
>>> +
>>> + wdt = mwd->wdt;
>>> +
>>> + /* 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.

>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void mei_wdt_release(struct kref *ref)
>>> +{
>>> + struct mei_wdt_dev *mwd = container_of(ref, struct
>>> mei_wdt_dev, refcnt);
>>> +
>>> + kfree(mwd);
>>> +}
>>> +
>>> +static void mei_wdt_ops_ref(struct watchdog_device *wdd)
>>> +{
>>> + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +
>>> + kref_get(&mwd->refcnt);
>>> +}
>>> +
>>> +static void mei_wdt_ops_unref(struct watchdog_device *wdd)
>>> +{
>>> + struct mei_wdt_dev *mwd = watchdog_get_drvdata(wdd);
>>> +
>>> + kref_put(&mwd->refcnt, mei_wdt_release);
>>> +}
>>> +
>>> +static const struct watchdog_ops wd_ops = {
>>> + .owner = THIS_MODULE,
>>> + .start = mei_wdt_ops_start,
>>> + .stop = mei_wdt_ops_stop,
>>> + .ping = mei_wdt_ops_ping,
>>> + .set_timeout = mei_wdt_ops_set_timeout,
>>> + .ref = mei_wdt_ops_ref,
>>> + .unref = mei_wdt_ops_unref,
>>> +};
>>> +
>>> +static struct watchdog_info wd_info = {
>>> + .identity = INTEL_AMT_WATCHDOG_ID,
>>> + .options = WDIOF_KEEPALIVEPING |
>>> + WDIOF_SETTIMEOUT |
>>> + WDIOF_ALARMONLY,
>>> +};
>>> +
>>> +static int mei_wdt_register(struct mei_wdt *wdt)
>>> +{
>>> + struct mei_wdt_dev *mwd;
>>> + struct device *dev;
>>> + int ret;
>>> +
>>> + if (!wdt || !wdt->cldev)
>>> + return -EINVAL;
>>> +
>>> + dev = &wdt->cldev->dev;
>>> +
>>> + mwd = kzalloc(sizeof(struct mei_wdt_dev), GFP_KERNEL);
>>> + if (!mwd)
>>> + return -ENOMEM;
>>> +
>>> + mwd->wdt = wdt;
>>> + mwd->wdd.info = &wd_info;
>>> + mwd->wdd.ops = &wd_ops;
>>> + mwd->wdd.parent = dev;
>>> + mwd->wdd.timeout = MEI_WDT_DEFAULT_TIMEOUT;
>>> + mwd->wdd.min_timeout = MEI_WDT_MIN_TIMEOUT;
>>> + mwd->wdd.max_timeout = MEI_WDT_MAX_TIMEOUT;
>>> + kref_init(&mwd->refcnt);
>>> +
>>> + ret = watchdog_register_device(&mwd->wdd);
>>> + if (ret) {
>>> + dev_err(dev, "unable to register watchdog device =
>>> %d.\n", ret);
>>> + kref_put(&mwd->refcnt, mei_wdt_release);
>>> + return ret;
>>> + }
>>> +
>>> + wdt->mwd = mwd;
>>> + watchdog_set_drvdata(&mwd->wdd, mwd);
>>> + return 0;
>>> +}
>>> +
>>> +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.

>>> +}
>>
>> 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).

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.

Thanks,
Guenter



\
 
 \ /
  Last update: 2015-12-01 17:21    [W:0.101 / U:1.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site