Message-ID: <20231223142148.jl6poaw2eqottzdg@pali>
Date: Sat, 23 Dec 2023 15:21:48 +0100
From: Pali Rohár <pali@...nel.org>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Paul Menzel <pmenzel@...gen.mpg.de>, Jean Delvare <jdelvare@...e.com>,
	Andi Shyti <andi.shyti@...nel.org>, Wolfram Sang <wsa@...nel.org>,
	linux-i2c@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
	Kai-Heng Feng <kai.heng.feng@...onical.com>,
	Marius Hoch <mail@...iushoch.de>,
	Mario Limonciello <mario.limonciello@....com>,
	Dell.Client.Kernel@...l.com, Greg KH <gregkh@...uxfoundation.org>
Subject: Re: Ideas for a generic solution to support accelerometer lis3lv02d
 in Dell laptops/notebooks?

On Saturday 23 December 2023 14:40:09 Hans de Goede wrote:
> Hi,
> 
> On 12/23/23 13:53, Pali Rohár wrote:
> > On Saturday 23 December 2023 13:45:32 Hans de Goede wrote:
> >> 2. Add a "probe_i2c_address" bool module option and when this
> >>    is set try to read the WHO_AM_I register, see
> >>    drivers/misc/lis3lv02d/lis3lv02d.c
> >>    and if this succeeds and gives a known model id then
> >>    continue with the found i2c_address. This should first
> >>    try address 0x29 which seems to be the most common and
> >>    then try 0x18 and then give up.
> > 
> > This is the main problem of the whole email thread. How to figure out
> > the correct smbus device address.
> 
> Ack.
> 
> > And we really must not poke random address during kernel boot time.
> > I think in the past was there enough problems linux kernel broke some HW
> > or made system unbootable just because it tried to read something from
> > some random undocumented address.
> > 
> > Please do not try random unverified address on all machines.
> 
> Right, that is why this sits behind a module option. Also note
> that the 0x29 / 0x18 addresses are typically used by some
> sensor ic, which are typically safe to probe.
> 
> > smbus is not really bus which provides discovering and identifying
> > devices on the bus.
> 
> I know I have worked on the lm_sensors project and the
> sensors-detect script in the past. Generally speaking
> though i2c probing is not that dangerous. But one can
> get unlucky ...

I think that manual probing after user confirmation is acceptable. But
automatic one without any user confirmation after kernel upgrade for all
existing and also all future machines is not something which I can say
that is safe.

I have experience when broken bytes in EDID eeprom and monitor
completely stopped working. It was after manual probing of some eeprom
driver (so driver of the correct class). If kernel/X11 logs did not
print warnings about EDID checksum I would not be able to debug & repair
it.

So one can be really unlucky if something happen in the future, like
changing laptop board wiring, changing accelerator chipset or whatever.

I have also experience with i2c device which have broken first i2c
single byte read (first after wakeup from low power state) and product
errata was to put data line to some position for at least some period of
time before doing the real data transfer.

I rather do not want to image what can happen if similar hw bug is in
the i2c multiplexer after which are connected more i2c devices...

After those experiences I want to be really safe about what is kernel
going to do automatically after the boot.

> We should probably first do 2 single byte i2c-reads
> (not smbus byte reads but plain i2c reads) if there
> is a i2c device there with the standard smbus register
> model where there is a 8 bit register address pointer
> then reading 2 times in a row will read 2 different
> registers (the internal register address pointer will
> increment) so we should get 2 different values.
> 
> If we get the same value twice then whatever is
> present on address 0x29 or 0x18 does not follow
> the standard smbus register addressing and we should
> refrain from doing a smbus-byte-read, which first
> sends the register-address to read so involves
> an actual i2c-*write*.
> 
> The combination of determining that normal smbus
> register addressing is used + only doing reads
> should make probing pretty safe. And the probing
> will only happen when the module option is set
> in the first place.

Hiding all above logic behind module parameter which is disabled by
default sounds safe. I think that this is something against which should
not be too much disagreements (expecting that module parameter will have
correct description with warning, just to be safe).

But well, my guess is that people would like to see accelerometer to
work out-of-the-box without configuring anything.

And this is possible only by communication with Dell. Dell designers
should have some ideas how it is suppose to work. And reinventing the
wheel from scratch in Linux kernel is not the best option.

> Regards,
> 
> Hans
> 
> 
> 
> 

Powered by blists - more mailing lists