linux-kernel - Re: [PATCHv2 4/7] input/cma3000

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:	Tue, 18 Oct 2011 14:50:50 +0100
From:	Jonathan Cameron <jic23@....ac.uk>
To:	Ricardo Ribalda Delgado <ricardo.ribalda@...il.com>
CC:	dmitry.torokhov@...il.com, sameo@...ux.intel.com,
	peter.ujfalusi@...com, aghayal@...eaurora.org, david@...deman.nu,
	Shubhrajyoti@...com, saaguirre@...com, hemanthv@...com,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv2 4/7] input/cma3000_d0x: Add CMA3000 spi support

On 10/18/11 14:43, Ricardo Ribalda Delgado wrote:
> Hello Jonathan
> 
>   First of all, thanks for your messages :).
> 
>> To make my point about these functions being more complex than needed
>> in more detail....
>>
>> If this were two functions and you drop the zero and 1 mask
>> (which I'm not convinced make any sense. I've also killed the message.
>> We both agree it is the wrong way to go, so post a patch fixing the i2c
>> interface as well.
> 
> Of course your functions are much more simpler and beautiful than the
> fat one I wrote, no doubt about it :). Just three comments
> 
> - Checking the one mask and the zero mask is the only way we have to
> know if the chip is still there, The absense of that reply should
> trigger an IO error or at least a retry. As you point out, the
> zero/one mask is only violated on startup.  I just wanted to make it
> more risk free, but if you believe it is more clear that way, lets
> remove it
It's somewhat unconventional to verify the existence of a chip like this.
Usually you assume that if it was there once it still is unless there
is a very good reason to think otherwise.  Worth doing an initial check
in your spi_probe and indeed verify there against these known bits.
No need to do it every time though.
> 
> - I am not very fun of kmallocing data per write, specially when it is
> part of the irq handler, and you expect this to be low latency. What
> about allocating a buffer on init time, and use it with a mutex?
That's absolutely fine and the right way to do it. You could poke it
into the cma3000_accl_data then use the cachline aligned magic. Its
is tiny so I doubt anyone will mind the overhead for the i2c side of
things.
> 
> -I dont like the push error message to the bottom, but that will mean
> a rewrite of the cma3000 driver, shall I go for it?
I would. Though probably worth getting Hemanth to say if he minds first
given it's his driver!
> 
> 
> Thanks again, and  I will post the new version when you reply this :)
> 
> 

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