linux-kernel - Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor

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: <6e2c9f59-296c-7382-6490-8e344f3e0210@gmail.com>
Date:   Mon, 8 May 2023 15:40:05 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor

Hi Andy,

On 5/8/23 15:23, Andy Shevchenko wrote:
> On Fri, May 05, 2023 at 04:56:47AM +0000, Vaittinen, Matti wrote:
>> On 5/4/23 17:33, Andy Shevchenko wrote:
>>> On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +config ROHM_BU27008
>>>> +	tristate "ROHM BU27008 color (RGB+C/IR) sensor"
>>>> +	depends on I2C
>>>> +	select REGMAP_I2C
>>>> +	select IIO_GTS_HELPER
>>>> +	help
>>>> +	  Enable support for the ROHM BU27008 color sensor.
>>>> +	  The ROHM BU27008 is a sensor with 5 photodiodes (red, green,
>>>> +	  blue, clear and IR) with four configurable channels. Red and
>>>> +	  green being always available and two out of the rest three
>>>> +	  (blue, clear, IR) can be selected to be simultaneously measured.
>>>> +	  Typical application is adjusting LCD backlight of TVs,
>>>> +	  mobile phones and tablet PCs.
>>>
>>> Module name?
>>
>> We have discussed this several times already.
>>
>> https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@fi.rohmeurope.com/
>>
>> Module name is completely irrelevant when selecting a kernel configuration.
> 
> This option is also selectable by user.

I don't think the name is selectable. Yes, user selects whether to 
compile driver as a module or in-kernel - but the module name is 
completely irrelevant what comes to this decision.

> ...
> 
>>> Do you need regmap lock? If so, why (since you have mutex)?
>>
>> I believe you know that regmap uses a default lock when no external lock
>> is given. So, I assume you mean that maybe we could set
>> 'disable_locking' for the regmap here.
> 
> Correct.
> 
>> It's nice to be occasionally pushed to think "out of the box". And yes,
>> disabling regmap lock is really out of my "normal box" :)
>>
>> I didn't go through all of the code yet, but I think pretty much all of
>> the sequences which end up to register writes are indeed protected by
>> the mutex. (Well, probe is not but it is expected to only update one bit
>> while rest of the register should stay fixed).
>>
>> It may be we could live without regmap_lock when driver is in it's
>> current state, but I am not convinced the performance improvement is
>> worth the risk. Having regmap unprotected is not common, and it is also
>> not easy to spot when making changes to the driver. In my opinion it is
>> a bit like asking for a nose-bleed unless there is really heavy reasons
>> to drop the lock... In this case, having the regmap_lock (which is
>> pretty much never locked because we have the mutex as you said) is
>> probably not a penalty that matters.
> 
> Basically you try to justify a hidden mine field in case somebody will think
> "oh, we are protected by regmap lock, so why to bother call mutex_lock()" and
> at the end it become a subtle bugs in the code. With disable_locking = true
> I can see that code author _carefully thought through_ the locking schema and
> understands the hardware and the code.

I added the disable_locking = true in v5 - but I am not convinced that 
was a great idea. I am afraid disabling regmap lock is the hidden 
minefield for average users. I didn't grep the kernel for it but I am 
afraid the percentage of regmap users who disable locking is very low. 
Thus, I'd say this is unexpected to many and may lead to bugs although I 
try to watch out for them. Well, time will tell.

> P.S. I'm wondering why your lines of text have a single trailing whitespace
> but the last line.

I guess it must be Thunderbird client then. Well, at least it can send 
out plain-text decently well while working with the exchange servers 
used by ROHM as well as with the gmail. I am not super happy with 
Thunderbird, it tends to eat way more resources I wished it did, but it 
is a working compromise for me. I am interested in hearing if anyone 
knows a way to configure the Thunderbird to drop these extra spaces.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~