linux-kernel - Re: [PATCH] input: add driver for Bosch Sensortec's BMA150 accelerometer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110531172320.0fcadfef@lxorguk.ukuu.org.uk>
Date:	Tue, 31 May 2011 17:23:20 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Eric Andersson <eric.andersson@...xphere.com>
Cc:	dmitry.torokhov@...il.com, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, zhengguang.guo@...ch-sensortec.com,
	stefan.nilsson@...xphere.com,
	Albert Zhang <xu.zhang@...ch-sensortec.com>
Subject: Re: [PATCH] input: add driver for Bosch Sensortec's BMA150
 accelerometer

See the driver I posted some time ago. It's in rather better state than
this one and as it's been through most of the review process and also
handles the BMA023 and SMB380 so I think would be a better place to start
from. I'll report that driver in a moment.

Alan


Some of the first immediately obvious questions though - in particular
the lack of clear locking ...


> +struct bma150acc {
> +	s16	x,
> +		y,
> +		z;
> +};

Why does this need to be a struct ?

> +	if ((mode != BMA150_MODE_NORMAL) &&
> +	    (mode != BMA150_MODE_SLEEP) &&
> +	    (mode != BMA150_MODE_WAKE_UP))
> +		return -EINVAL;

How can any other mode get passed ?

> +
> +	data1 = i2c_smbus_read_byte_data(client, BMA150_WAKE_UP_REG);
> +	if (data1 < 0)
> +		return ret;
> +
> +	data1 = (data1 & ~BMA150_WAKE_UP_MSK) |
> +		((mode << BMA150_WAKE_UP_POS) & BMA150_WAKE_UP_MSK);
> +
> +	data2 = i2c_smbus_read_byte_data(client, BMA150_SLEEP_REG);
> +	if (data2 < 0)
> +		return ret;
> +
> +	data2 = (data2 & ~BMA150_SLEEP_MSK) |
> +		(((mode>>1) << BMA150_SLEEP_POS) & BMA150_SLEEP_MSK);
> +
> +	ret = i2c_smbus_write_byte_data(client, BMA150_WAKE_UP_REG, data1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, BMA150_SLEEP_REG, data2);
> +	if (ret < 0)
> +		return ret;

What locks this SMBUS transaction against others

> +static int bma150_set_range(struct i2c_client *client, unsigned char range)
> +{
> +	int ret;
> +	unsigned char data;
> +
> +	if (range > BMA150_RANGE_8G)
> +		return -EINVAL;

This should be actual values not a register range

> +
> +	data = i2c_smbus_read_byte_data(client, BMA150_RANGE_REG);
> +	if (data < 0)
> +		return ret;
> +
> +	data = (data & ~BMA150_RANGE_MSK) |
> +		((range << BMA150_RANGE_POS) & BMA150_RANGE_MSK);
> +
> +	ret = i2c_smbus_write_byte_data(client, BMA150_RANGE_REG, data);

What locks this ?

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Why this pointless if ?

> +}
> +
> +static int bma150_get_range(struct i2c_client *client, unsigned char *range)
> +{
> +	int ret;
> +	unsigned char data;
> +
> +	data = i2c_smbus_read_byte_data(client, BMA150_RANGE_REG);
> +	if (data < 0)
> +		return ret;
> +
> +	*range = (data & BMA150_RANGE_MSK) >> BMA150_RANGE_POS;
> +	return 0;

See comments above

> +}
> +
> +static int bma150_set_bandwidth(struct i2c_client *client, unsigned char bw)

Similar problem


> +static int bma150_read_accel_xyz(struct i2c_client *client,
> +		struct bma150acc *acc)
> +{
> +	unsigned char data[6];
> +	int ret = i2c_smbus_read_i2c_block_data(client,
> +			BMA150_ACC_X_LSB_REG, 6, data);
> +	if (ret != 6)
> +		return -EIO;
> +
> +	acc->x = ((0xC0 & data[0]) >> 6) | (data[1] << 2);
> +	acc->y = ((0xC0 & data[2]) >> 6) | (data[3] << 2);
> +	acc->z = ((0xC0 & data[4]) >> 6) | (data[5] << 2);
> +
> +	/* sign extension */
> +	acc->x = (s16) (acc->x << 6) >> 6;
> +	acc->y = (s16) (acc->y << 6) >> 6;
> +	acc->z = (s16) (acc->z << 6) >> 6;
> +
> +	return 0;

Why the separate function and struct

> +}
> +
> +static void bma150_work_func(struct work_struct *work)
> +{

Threaded IRQ ?


> +static ssize_t bma150_mode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct bma150_data *bma150 = i2c_get_clientdata(client);
> +
> +	return sprintf(buf, "%d\n", bma150->mode);

API definition ?


> +	schedule_delayed_work(&data->work,
> +			msecs_to_jiffies(atomic_read(&data->delay)));
> +

Why the atomic read ?


And there are a ton more issues unfixed in this driver

--
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/