| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <53f7024e.4498420a.1bbf.79ce@mx.google.com>
Date: Fri, 22 Aug 2014 16:41:36 +0800
From: "Dudley Du" <dudley.dulixin@...il.com>
To: "'Dmitry Torokhov'" <dmitry.torokhov@...il.com>
Cc: "'Rafael J. Wysocki'" <rjw@...ysocki.net>,
"'Benson Leung'" <bleung@...gle.com>,
"'Patrik Fimml'" <patrikf@...gle.com>,
<linux-input@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
"Dudley Du" <dudl@...ress.com>
Subject: RE: [PATCH v4 4/14] input: cyapa: add cyapa key function interfaces in sysfs system
> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@...il.com]
> Sent: Friday, August 22, 2014 2:29 AM
> To: Dudley Du
> Cc: Rafael J. Wysocki; Benson Leung; Patrik Fimml; linux-input@...r.kernel.org;
> linux-kernel@...r.kernel.org; Dudley Du
> Subject: Re: [PATCH v4 4/14] input: cyapa: add cyapa key function interfaces
> in sysfs system
>
> On Thu, Jul 17, 2014 at 02:51:04PM +0800, Dudley Du wrote:
> > Add key basic function interfaces in cyapa driver in sysfs system,
> > these interfaces are commonly used in pre- and after production, and
> > for trackpad device state checking, manage and firmware image updating.
> > These interfaces including firmware_version and product_id interfaces
> > for reading firmware version and trackpad device product id values,
> > and including update_fw interface to command firmware image update
> > process. Also including baseline and calibrate interfaces, so can
> > read and check the trackpad device states. If the baseline values are
> > invalid, then can use calibrate interface to recover it.
> > TEST=test on Chromebooks.
> >
> > Signed-off-by: Dudley Du <dudl@...ress.com>
> > ---
> > drivers/input/mouse/cyapa.c | 190
> +++++++++++++++++++++++++++++++++++++++++++
> > drivers/input/mouse/cyapa.h | 27 ++++++
> > 2 files changed, 217 insertions(+)
> >
> > diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> > index 6a2783b..53c9d59 100644
> > --- a/drivers/input/mouse/cyapa.c
> > +++ b/drivers/input/mouse/cyapa.c
> > @@ -388,6 +388,78 @@ static void cyapa_detect(struct cyapa *cyapa)
> > }
> > }
> >
> > +static int cyapa_firmware(struct cyapa *cyapa, const char *fw_name)
> > +{
> > + struct device *dev = &cyapa->client->dev;
> > + int ret;
> > + const struct firmware *fw;
> > +
> > + ret = request_firmware(&fw, fw_name, dev);
> > + if (ret) {
> > + dev_err(dev, "Could not load firmware from %s, %d\n",
> > + fw_name, ret);
> > + return ret;
> > + }
> > +
> > + if (cyapa->ops->check_fw) {
> > + ret = cyapa->ops->check_fw(cyapa, fw);
> > + if (ret) {
> > + dev_err(dev, "Invalid CYAPA firmware image: %s\n",
> > + fw_name);
> > + goto done;
> > + }
> > + } else {
> > + dev_err(dev, "Unknown status, operation forbidden, gen=%d\n",
> > + cyapa->gen);
> > + ret = -ENOTSUPP;
>
> I'd say EIO or EINVAL.
Thanks, I will set it to EINVAL in next release.
>
> > + goto done;
> > + }
> > +
> > + /*
> > + * Resume the potentially suspended device because doing FW
> > + * update on a device not in the FULL mode has a chance to
> > + * fail.
> > + */
> > + pm_runtime_get_sync(dev);
> > +
> > + if (cyapa->ops->bl_enter) {
> > + ret = cyapa->ops->bl_enter(cyapa);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->bl_activate) {
> > + ret = cyapa->ops->bl_activate(cyapa);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->bl_initiate) {
> > + ret = cyapa->ops->bl_initiate(cyapa, fw);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->update_fw) {
> > + ret = cyapa->ops->update_fw(cyapa, fw);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > + if (cyapa->ops->bl_verify_app_integrity) {
> > + ret = cyapa->ops->bl_verify_app_integrity(cyapa);
> > + if (ret)
> > + goto err_detect;
> > + }
> > +
> > +err_detect:
> > + pm_runtime_put_noidle(dev);
> > +
> > +done:
> > + release_firmware(fw);
> > + return ret;
> > +}
> > +
> > /*
> > * Sysfs Interface.
> > */
> > @@ -584,6 +656,120 @@ static void cyapa_start_runtime(struct cyapa *cyapa)
> > static void cyapa_start_runtime(struct cyapa *cyapa) {}
> > #endif /* CONFIG_PM_RUNTIME */
> >
> > +static ssize_t cyapa_show_fm_ver(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int ret;
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > + ret = scnprintf(buf, PAGE_SIZE, "%d.%d\n", cyapa->fw_maj_ver,
> > + cyapa->fw_min_ver);
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t cyapa_show_product_id(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + int ret;
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > + ret = scnprintf(buf, PAGE_SIZE, "%s\n", cyapa->product_id);
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret;
> > +}
> > +
> > +static ssize_t cyapa_update_fw_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > + const char *fw_name;
> > + int ret;
> > +
> > + /* Do not allow paths that step out of /lib/firmware */
> > + if (strstr(buf, "../") != NULL)
> > + return -EINVAL;
>
> This should go away, it does not help anything.
Thanks. It will be removed in next release.
>
> > +
> > + if (!strncmp(buf, "1", count) || !strncmp(buf, "1\n", count))
> > + fw_name = CYAPA_FW_NAME;
> > + else
> > + fw_name = buf;
>
> I'd rather you either required firmware name to be passed always or settle on
> given name. Otherwise why can't I call my firmware file '1'?
>
Thanks. I will modify to require firmware name to be passed always.
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + ret = cyapa_firmware(cyapa, fw_name);
> > + if (ret)
> > + dev_err(dev, "firmware update failed, %d\n", ret);
> > + else
> > + dev_dbg(dev, "firmware update succeeded\n");
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > +
> > + /* Redetect trackpad device states. */
> > + cyapa_detect_async(cyapa, 0);
> > +
> > + return ret ? ret : count;
> > +}
> > +
> > +static ssize_t cyapa_calibrate_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + if (!cyapa->ops->calibrate_store) {
> > + dev_err(dev, "Calibrate operation not permitted.\n");
> > + ret = -ENOTSUPP;
>
> Permitted as in supported or forbidden?
The "permitted" means not supported, because device state is unknown.
I will modify the word "permitted" to "supported".
Thanks.
>
> > + } else
> > + ret = cyapa->ops->calibrate_store(dev, attr, buf, count);
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret < 0 ? ret : count;
> > +}
> > +
> > +static ssize_t cyapa_show_baseline(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct cyapa *cyapa = dev_get_drvdata(dev);
> > + ssize_t ret;
> > +
> > + mutex_lock(&cyapa->state_sync_lock);
> > +
> > + if (!cyapa->ops->show_baseline) {
> > + dev_err(dev, "Calibrate operation not permitted.\n");
> > + ret = -ENOTSUPP;
> > + } else
> > + ret = cyapa->ops->show_baseline(dev, attr, buf);
> > +
> > + mutex_unlock(&cyapa->state_sync_lock);
> > + return ret;
> > +}
> > +
> > +static DEVICE_ATTR(firmware_version, S_IRUGO, cyapa_show_fm_ver, NULL);
> > +static DEVICE_ATTR(product_id, S_IRUGO, cyapa_show_product_id, NULL);
> > +static DEVICE_ATTR(update_fw, S_IWUSR, NULL, cyapa_update_fw_store);
> > +static DEVICE_ATTR(baseline, S_IRUGO, cyapa_show_baseline, NULL);
> > +static DEVICE_ATTR(calibrate, S_IWUSR, NULL, cyapa_calibrate_store);
> > +
> > +static struct attribute *cyapa_sysfs_entries[] = {
> > + &dev_attr_firmware_version.attr,
> > + &dev_attr_product_id.attr,
> > + &dev_attr_update_fw.attr,
> > + &dev_attr_baseline.attr,
> > + &dev_attr_calibrate.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group cyapa_sysfs_group = {
> > + .attrs = cyapa_sysfs_entries,
> > +};
> > +
> > void cyapa_detect_async(void *data, async_cookie_t cookie)
> > {
> > struct cyapa *cyapa = (struct cyapa *)data;
> > @@ -680,6 +866,9 @@ static int cyapa_probe(struct i2c_client *client,
> > goto err_uninit_tp_modules;
> > }
> >
> > + if (sysfs_create_group(&client->dev.kobj, &cyapa_sysfs_group))
> > + dev_warn(dev, "error creating sysfs entries.\n");
> > +
> > #ifdef CONFIG_PM_SLEEP
> > if (device_can_wakeup(dev) &&
> > sysfs_merge_group(&client->dev.kobj, &cyapa_power_wakeup_group))
> > @@ -704,6 +893,7 @@ static int cyapa_remove(struct i2c_client *client)
> > struct cyapa *cyapa = i2c_get_clientdata(client);
> >
> > pm_runtime_disable(&client->dev);
> > + sysfs_remove_group(&client->dev.kobj, &cyapa_sysfs_group);
> >
> > #ifdef CONFIG_PM_SLEEP
> > sysfs_unmerge_group(&client->dev.kobj, &cyapa_power_wakeup_group);
> > diff --git a/drivers/input/mouse/cyapa.h b/drivers/input/mouse/cyapa.h
> > index 9c1f0b91..567ab08 100644
> > --- a/drivers/input/mouse/cyapa.h
> > +++ b/drivers/input/mouse/cyapa.h
> > @@ -171,6 +171,22 @@ struct cyapa;
> > typedef bool (*cb_sort)(struct cyapa *, u8 *, int);
> >
> > struct cyapa_dev_ops {
> > + int (*check_fw)(struct cyapa *, const struct firmware *);
> > + int (*bl_enter)(struct cyapa *);
> > + int (*bl_activate)(struct cyapa *);
> > + int (*bl_initiate)(struct cyapa *, const struct firmware *);
> > + int (*update_fw)(struct cyapa *, const struct firmware *);
> > + int (*bl_verify_app_integrity)(struct cyapa *);
> > + int (*bl_deactivate)(struct cyapa *);
> > +
> > + ssize_t (*show_baseline)(struct device *,
> > + struct device_attribute *, char *);
> > + ssize_t (*calibrate_store)(struct device *,
> > + struct device_attribute *, const char *, size_t);
> > +
> > + int (*read_fw)(struct cyapa *);
> > + int (*read_raw_data)(struct cyapa *);
> > +
> > int (*initialize)(struct cyapa *cyapa);
> > int (*uninitialize)(struct cyapa *cyapa);
> >
> > @@ -243,6 +259,17 @@ struct cyapa {
> > */
> > struct mutex state_sync_lock;
> >
> > + /* Per-instance debugfs root */
> > + struct dentry *dentry_dev;
> > +
> > + /* Buffer to store firmware read using debugfs */
> > + struct mutex debugfs_mutex;
> > + u8 *fw_image;
> > + size_t fw_image_size;
> > + /* Buffer to store sensors' raw data */
> > + u8 *tp_raw_data;
> > + size_t tp_raw_data_size;
> > +
> > const struct cyapa_dev_ops *ops;
> > };
> >
> > --
> > 1.7.9.5
> >
> >
>
> Thanks.
>
> --
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists