| 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
| ||
|
Date: Mon, 30 Nov 2009 16:25:12 +0100
From: Éric Piel <eric.piel@...mplin-utc.net>
To: Samu Onkalo <samu.p.onkalo@...ia.com>
Cc: linux-kernel@...r.kernel.org, lm-sensors@...sensors.org
Subject: Re: [PATCH v3 1/5] lis3: Selftest support
Op 30-11-09 15:56, Éric Piel schreef:
> Op 19-11-09 10:27, Samu Onkalo schreef:
>> Implement selftest feature as specified by chip manufacturer.
>> Control: read selftest sysfs entry
>> Response: "OK x y z" or "FAIL x y z"
>> where x, y, and z are difference between selftest mode and normal mode.
>> Test is passed when values are within acceptance limit values.
>>
>> Acceptance limits are provided via platform data. See chip spesifications
>> for acceptance limits. If limits are not properly set, OK / FAIL decision is
>> meaningless. However, userspace application can still make decision based on
>> the numeric x, y, z values.
>>
>> Selftest is meant for HW diagnostic purposes. It is not meant to be called
>> during normal use of the chip. It may cause false interrupt events.
>> Selftest mode delays polling of the normal results but it doesn't cause
>> wrong values. Chip must be in static state during selftest.
>> Any acceration during the test causes most probably failure.
> This patch looks good to me, but please add the info of this changelog
> into the documentation (in patch 4). If users have to start digging into
> the git changelog to find out what does a feature, we are nasty ;-)
Oops, I hadn't noticed your modification in patch 5, sorry! It's good...
but now I noticed a little enhancement you could do in this patch :-) .
See below...
>
> Signed-off-by: Éric Piel <eric.piel@...mplin-utc.net>
>
>> Signed-off-by: Samu Onkalo <samu.p.onkalo@...ia.com>
>> ---
>> drivers/hwmon/lis3lv02d.c | 72 ++++++++++++++++++++++++++++++++++++++++++++-
>> drivers/hwmon/lis3lv02d.h | 14 +++++++-
>> include/linux/lis3lv02d.h | 3 ++
>> 3 files changed, 86 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>> index 39b9ac8..69314f9 100644
>> --- a/drivers/hwmon/lis3lv02d.c
>> +++ b/drivers/hwmon/lis3lv02d.c
>> @@ -106,9 +106,11 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
>> {
>> int position[3];
>>
>> + mutex_lock(&lis3->mutex);
>> position[0] = lis3->read_data(lis3, OUTX);
>> position[1] = lis3->read_data(lis3, OUTY);
>> position[2] = lis3->read_data(lis3, OUTZ);
>> + mutex_unlock(&lis3->mutex);
>>
>> *x = lis3lv02d_get_axis(lis3->ac.x, position);
>> *y = lis3lv02d_get_axis(lis3->ac.y, position);
>> @@ -133,6 +135,60 @@ static int lis3lv02d_get_odr(void)
>> return val;
>> }
>>
>> +static int lis3lv02d_selftest(struct lis3lv02d *lis3, s16 results[3])
>> +{
>> + u8 reg;
>> + s16 x, y, z;
>> + u8 selftest;
>> + int ret;
>> +
>> + mutex_lock(&lis3->mutex);
>> + if (lis3_dev.whoami == WAI_12B)
>> + selftest = CTRL1_ST;
>> + else
>> + selftest = CTRL1_STP;
>> +
>> + lis3->read(lis3, CTRL_REG1, ®);
>> + lis3->write(lis3, CTRL_REG1, (reg | selftest));
>> + msleep(lis3->pwron_delay / lis3lv02d_get_odr());
>> +
>> + /* Read directly to avoid axis remap */
>> + x = lis3->read_data(lis3, OUTX);
>> + y = lis3->read_data(lis3, OUTY);
>> + z = lis3->read_data(lis3, OUTZ);
>> +
>> + /* back to normal settings */
>> + lis3->write(lis3, CTRL_REG1, reg);
>> + msleep(lis3->pwron_delay / lis3lv02d_get_odr());
>> +
>> + results[0] = x - lis3->read_data(lis3, OUTX);
>> + results[1] = y - lis3->read_data(lis3, OUTY);
>> + results[2] = z - lis3->read_data(lis3, OUTZ);
>> +
>> + ret = 0;
>> + if (lis3->pdata) {
>> + int i;
>> + for (i = 0; i < 3; i++) {
>> + /* Check minimum acceptance limits */
>> + if (results[i] < lis3->pdata->st_min_limits[i]) {
>> + ret = -EIO;
>> + goto fail;
>> + }
>> +
>> + /* Check maximum acceptance limits */
>> + if (results[i] > lis3->pdata->st_max_limits[i]) {
>> + ret = -EIO;
>> + goto fail;
>> + }
>> + }
>> + }
>> +
Please merge these two if's to something like:
if ((results[i] < lis3->pdata->st_min_limits[i]) ||
(results[i] > lis3->pdata->st_max_limits[i]) {
>> + /* test passed */
>> +fail:
>> + mutex_unlock(&lis3->mutex);
>> + return ret;
>> +}
>> +
>> void lis3lv02d_poweroff(struct lis3lv02d *lis3)
>> {
>> /* disable X,Y,Z axis and power down */
>> @@ -365,6 +421,17 @@ void lis3lv02d_joystick_disable(void)
>> EXPORT_SYMBOL_GPL(lis3lv02d_joystick_disable);
>>
>> /* Sysfs stuff */
>> +static ssize_t lis3lv02d_selftest_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int result;
>> + s16 values[3];
>> +
>> + result = lis3lv02d_selftest(&lis3_dev, values);
>> + return sprintf(buf, "%s %d %d %d\n", result == 0 ? "OK" : "FAIL",
>> + values[0], values[1], values[2]);
>> +}
>> +
>> static ssize_t lis3lv02d_position_show(struct device *dev,
>> struct device_attribute *attr, char *buf)
>> {
>> @@ -394,12 +461,14 @@ static ssize_t lis3lv02d_rate_show(struct device *dev,
>> return sprintf(buf, "%d\n", lis3lv02d_get_odr());
>> }
>>
>> +static DEVICE_ATTR(selftest, S_IRUSR, lis3lv02d_selftest_show, NULL);
>> static DEVICE_ATTR(position, S_IRUGO, lis3lv02d_position_show, NULL);
>> static DEVICE_ATTR(calibrate, S_IRUGO|S_IWUSR, lis3lv02d_calibrate_show,
>> lis3lv02d_calibrate_store);
>> static DEVICE_ATTR(rate, S_IRUGO, lis3lv02d_rate_show, NULL);
>>
>> static struct attribute *lis3lv02d_attributes[] = {
>> + &dev_attr_selftest.attr,
>> &dev_attr_position.attr,
>> &dev_attr_calibrate.attr,
>> &dev_attr_rate.attr,
>> @@ -455,6 +524,8 @@ int lis3lv02d_init_device(struct lis3lv02d *dev)
>> return -EINVAL;
>> }
>>
>> + mutex_init(&dev->mutex);
>> +
>> lis3lv02d_add_fs(dev);
>> lis3lv02d_poweron(dev);
>>
>> @@ -507,4 +578,3 @@ EXPORT_SYMBOL_GPL(lis3lv02d_init_device);
>> MODULE_DESCRIPTION("ST LIS3LV02Dx three-axis digital accelerometer driver");
>> MODULE_AUTHOR("Yan Burman, Eric Piel, Pavel Machek");
>> MODULE_LICENSE("GPL");
>> -
>> diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
>> index c57f21f..166794c 100644
>> --- a/drivers/hwmon/lis3lv02d.h
>> +++ b/drivers/hwmon/lis3lv02d.h
>> @@ -98,7 +98,7 @@ enum lis3_who_am_i {
>> WAI_6B = 0x52, /* 6 bits: LIS331DLF - not supported */
>> };
>>
>> -enum lis3lv02d_ctrl1 {
>> +enum lis3lv02d_ctrl1_12b {
>> CTRL1_Xen = 0x01,
>> CTRL1_Yen = 0x02,
>> CTRL1_Zen = 0x04,
>> @@ -107,8 +107,17 @@ enum lis3lv02d_ctrl1 {
>> CTRL1_DF1 = 0x20,
>> CTRL1_PD0 = 0x40,
>> CTRL1_PD1 = 0x80,
>> - CTRL1_DR = 0x80, /* Data rate on 8 bits */
>> };
>> +
>> +/* Delta to ctrl1_12b version */
>> +enum lis3lv02d_ctrl1_8b {
>> + CTRL1_STM = 0x08,
>> + CTRL1_STP = 0x10,
>> + CTRL1_FS = 0x20,
>> + CTRL1_PD = 0x40,
>> + CTRL1_DR = 0x80,
>> +};
>> +
>> enum lis3lv02d_ctrl2 {
>> CTRL2_DAS = 0x01,
>> CTRL2_SIM = 0x02,
>> @@ -218,6 +227,7 @@ struct lis3lv02d {
>> unsigned long misc_opened; /* bit0: whether the device is open */
>>
>> struct lis3lv02d_platform_data *pdata; /* for passing board config */
>> + struct mutex mutex; /* Serialize poll and selftest */
>> };
>>
>> int lis3lv02d_init_device(struct lis3lv02d *lis3);
>> diff --git a/include/linux/lis3lv02d.h b/include/linux/lis3lv02d.h
>> index 8970135..f1ca0dc 100644
>> --- a/include/linux/lis3lv02d.h
>> +++ b/include/linux/lis3lv02d.h
>> @@ -55,6 +55,9 @@ struct lis3lv02d_platform_data {
>> s8 axis_z;
>> int (*setup_resources)(void);
>> int (*release_resources)(void);
>> + /* Limits for selftest are specified in chip data sheet */
>> + s16 st_min_limits[3]; /* min pass limit x, y, z */
>> + s16 st_max_limits[3]; /* max pass limit x, y, z */
>> };
>>
>> #endif /* __LIS3LV02D_H_ */
>
>
--
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