linux-kernel - Re: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A 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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 14 Oct 2022 14:22:32 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
CC:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        "Lars-Peter Clausen" <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Jagath Jog J <jagathjog1996@...il.com>,
        Nikita Yushchenko <nikita.yoush@...entembedded.com>,
        Cosmin Tanislav <demonsingur@...il.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: [RFC PATCH v2 4/5] iio: accel: Support Kionix/ROHM KX022A
 accelerometer

On Tue, 11 Oct 2022 09:10:21 +0000
"Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com> wrote:

> On 10/10/22 09:15, Andy Shevchenko wrote:
> > On Sun, Oct 09, 2022 at 01:33:51PM +0100, Jonathan Cameron wrote:  
> >> On Thu, 6 Oct 2022 21:32:11 +0300 Andy Shevchenko 
> >> <andriy.shevchenko@...ux.intel.com> wrote:  
> >>> On Thu, Oct 06, 2022 at 05:38:14PM +0300, Matti Vaittinen wrote:  
> > 
> > ...
> >   
> >>>> +module_param(g_kx022a_use_buffer, bool, 0); 
> >>>> +MODULE_PARM_DESC(g_kx022a_use_buffer, +		 "Buffer samples. Use
> >>>> at own risk. Fifo must not overflow");  
> >>> 
> >>> Why?! We usually do not allow module parameters in the new code.  
> >> 
> >> Badly broken hardware - was my suggestion.  Alternatives if there 
> >> are usecases that need to use the fifo, but it can wedge hard in a
> >>  fashion that is impossible to prevent from the driver?  My gut 
> >> feeling is still drop the support entirely with a strong comment in
> >> the code that the hardware is broken in a fashion we don't know how
> >> to work around.  
> 
> I did some quick study regarding couple of other Kionix sensors. (like 
> KX122 and old KX022 - without the 'A'). It seems to me that the register 
> interfaces between many of the sensors are largely identical. Extending 
> the driver to support those seems pretty straightforward (scales and 
> resolution may need tweaking, as does the FIFO size) but register 
> contents and even offsets are largely identical.

Last kionix part I had was a kxsd9 and I don't recall that having a fifo
so obviously didn't hit this issue.

> 
> As said, it seems the Kionix sensors may have different size of internal 
> FIFOs, or even no FIFO at all. So, maybe we could provide a 
> "kionix,fifo-enable" flag (or even "kionix,fifo-size") from device-tree? 

For device where we don't have reports of this issue, that should be derived
from the compatible (or even better a whoami register if there is one).
The driver should know the fifo-size if it isn't discoverable.

> This would be a way to have the FIFO disabled by default and warn users 
> via dt-binding docs if they decide to explicitly enable the FIFO. 
> (Besides, I believe the FIFO is usable on at least some of the Kionix 
> sensors - because I've heard it is used on a few platforms).
> 
> This could give us safe defaults while not shutting the doors from those 
> who wish to use the FIFO. Sure we need a buy-in from Krzysztof / Rob, 
> but that may be less of an obstacle compared to the module param if Greg 
> is so strongly oppsoing those. (And the dt-property could also provide 
> some technical merites as these sensors seem to really have differencies 
> in FIFOs).

I'm dubious about having this for known broken parts - but I guess you
can propose it and see what the dt-maintainers say.

I don't want to see fifo size in the dt binding though.
> 
> > 
> > I also would drop this from upstream and if anybody curious, provide
> >  some kind of GitHub gist for that.  
> 
> Well, I think we all agree that downstream code hosted in some 
> unofficial github repositories are rarely that valuable. They're less 
> reliable, less tested, less reviewed, less secure and pretty much 
> impossible to maintain in a way that interested user could get a version 
> matching his preferred kernel.
> 
> There are reasons why I (people) keep sending the drivers to upstream - 
> and why some companies even spend $$ for that. Having this feature in 
> downstream repo is not nearly on same page for user's point of view as 
> is having the support upstream.

It's not really support if it comes with big warnings and potentially we
even taint the kernel of someone turns it on...

> 
> > Also it needs some communication
> >  with a vendor to clarify the things.  
> 
> I do communication with the vendor on a daily basis :] Nowadays Kionix 
> is part of ROHM, and Finland SWDC has been collaboration with Kionix 
> even before they "merged" (but I have not been part of the "sensor team" 
> back then).
> 
> Unfortunately, reaching the correct people inside the company is hard 
> and occasionally impossible - long story...

:)

Jonathan