linux-kernel - RE: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver

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]

Date:	Fri, 13 Aug 2010 08:34:07 -0500
From:	"Murphy, Dan" <dmurphy@...com>
To:	"V, Hemanth" <hemanthv@...com>, Jonathan Cameron <jic23@....ac.uk>,
	Andrew Morton <akpm@...ux-foundation.org>
CC:	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>
Subject: RE: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver

Hemanth
I have a few comments on this patch.

+static ssize_t cma3000_store_attr_mdfftmr(struct device *dev,
+					struct device_attribute *attr,
+					const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct cma3000_accl_data *data = platform_get_drvdata(pdev);
+	unsigned long val;
+	int error;
+
+	error = strict_strtoul(buf, 0, &val);
+	if (error)
+		return error;
+
+	mutex_lock(&data->mutex);
+	data->pdata.mdfftmr = val;
+
+	disable_irq(data->client->irq);
You should use disable_irq_nosync here.  This may not work properly on SMP.

> +	if (val == CMARANGE_2G) {
> +		ctrl |= CMA3000_RANGE2G;
> +		data->pdata.g_range = CMARANGE_2G;
> +	} else if (val == CMARANGE_8G) {
> +		ctrl |= CMA3000_RANGE8G;
> +		data->pdata.g_range = CMARANGE_8G;

Why are you modifying the platform data?  Why not just keep it in a global or a glocal structure and modify it that way?
> +	} else {
> +		error = -EINVAL;
> +		goto err_op_failed;
> +	}
> +
> +	g_range = data->pdata.g_range;
> +	fuzz_x = data->pdata.fuzz_x;
> +	fuzz_y = data->pdata.fuzz_y;
> +	fuzz_z = data->pdata.fuzz_z;
Why are you storing these locally and then using them once can't we eliminate these completely and just pass the platform data values into the set params?
> +
> +	disable_irq(data->client->irq);
You should use disable_irq_nosync here.  This may not work properly on SMP.

> +	cma3000_set(data, CMA3000_CTRL, ctrl, "ctrl");
> +
> +	input_set_abs_params(data->input_dev, ABS_X, -g_range,
> +				g_range, fuzz_x, 0);
> +	input_set_abs_params(data->input_dev, ABS_Y, -g_range,
> +				g_range, fuzz_y, 0);
> +	input_set_abs_params(data->input_dev, ABS_Z, -g_range,
> +				g_range, fuzz_z, 0);
Don't necessarily agree with modifying the parameters for the input device on the fly.  Some implementations may be a read once on init and do not go back and check this.


+		ret = request_threaded_irq(data->client->irq, NULL,
+					cma3000_thread_irq,
+					irqflags | IRQF_ONESHOT,
+					data->client->name, data);

This is implemented wrong.  You are doing a lot of processing in the IRQ context here.  Especially calls out to a peripheral.  The NULL should be your handler thread where you do all the device processing.

Also this implementation only suggests that the HW has the IRQ connected what about devices that the IRQ line was not connected?

I have changed this driver to adjust all the issues above.  Let me know if you want the patches or the files.

Dan

-----Original Message-----
From: linux-input-owner@...r.kernel.org [mailto:linux-input-owner@...r.kernel.org] On Behalf Of V, Hemanth
Sent: Friday, August 13, 2010 7:47 AM
To: Jonathan Cameron; Andrew Morton
Cc: linux-input@...r.kernel.org; linux-kernel@...r.kernel.org; linux-omap@...r.kernel.org
Subject: Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver

----- Original Message ----- 
From: "Jonathan Cameron" <jic23@....ac.uk>
To: "Hemanth V" <hemanthv@...com>
Cc: <linux-input@...r.kernel.org>; <linux-kernel@...r.kernel.org>; 
<linux-omap@...r.kernel.org>
Sent: Friday, May 21, 2010 5:27 PM
Subject: Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver


> On 05/21/10 07:52, Hemanth V wrote:
>> From: Hemanth V <hemanthv@...com>
>> Date: Thu, 20 May 2010 20:18:17 +0530
>> Subject: [PATCH] input: CMA3000 Accelerometer Driver
>>
>> This patch adds support for CMA3000 Tri-axis accelerometer, which
>> supports Motion detect, Measurement and Free fall modes.
>> CMA3000 supports both I2C/SPI bus for communication, currently the
>> driver supports I2C based communication.
>>
>> Driver reports acceleration data through input subsystem and supports
>> sysfs for configuration changes.
>>
>> This is V2 of patch, which fixes open source review comments
>>
>
> Hi,
>
> The driver is nice and clean.
>
> Still leaving aside the long argued question of whether this should be
> in input.  If you care for my views on that there are plenty or other
> threads! There are a few things I'd still like to suggest:
>

Jonathan, Andrew

Is there any way forward for this patch, I am unable to induce any response
from the input maintainer

Thanks
Hemanth 

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/