Bug 209603

Summary: hp_accel takes more than 1 second to resume
Product: Drivers Reporter: Zhang Rui (rui.zhang)
Component: Platform_x86Assignee: Hans de Goede (jwrdegoede)
Status: RESOLVED CODE_FIX    
Severity: normal CC: andy.shevchenko
Priority: P2    
Hardware: All   
OS: Linux   
Kernel Version: all Subsystem:
Regression: No Bisected commit-id:
Bug Depends on:    
Bug Blocks: 178231    
Attachments: pmgraph output for the slow S3 resume

Description Zhang Rui 2020-10-10 04:10:31 UTC
Created attachment 292913 [details]
pmgraph output for the slow S3 resume

On a HP KBL platform I have, the hp_accel driver takes more than 1.1 second to resume from S3 or freeze state. And most of the time (1 second) is spent in evaluating the _INI method.
So I checked the code a little bit, below is the code path
lis3lv02d_resume() -> lis3lv02d_poweron() -> lis3->init() -> lis3lv02d_acpi_init() -> _INI
According to the ACPI spec, _INI method can only be invoked during initialization time.
I'm not sure why we need this in the .resume() callback, but this seems to be an ACPI spec violation to me.
Raised the issue here and hopefully we can get this fixed.
Comment 1 Hans de Goede 2020-10-27 15:02:18 UTC
Quick self intro: I have take over drivers/platform/x86 maintainership from Andy; and I'm working my way through the bugzilla backlog.

I guess that calling _INI on resume could very well be against the ACPI spec, but I believe that more drivers are doing it. Still if skipping the _INI call on resume shaves a second of the resume call then we should probably indeed only do it on power-on (and resume-from-hibernate) I would certainly welcome a patch to do that.

p.s. on the subject of the lis3lv02d code have you seen this oops/WARN_ON ? : 

https://bugzilla.redhat.com/show_bug.cgi?id=1813080

The backtrace is from a resume-from-hibernate, but it happens on boot and on regular resume too (the backtrace's for those are in private bugs). I ve started writing a fix for this: https://github.com/jwrdegoede/linux-sunxi/commit/4627dfbad4d005625071b6e491886b627435144f

But I never finished it due to an unresponsive maintainer ...

This mostly (but not always) seems to trigger on resume, so it is somewhat related. I guess that the driver may simply be doing too much on resume.
Comment 2 Hans de Goede 2020-10-27 16:50:28 UTC
Andy, I'm answering here instead of github, since my git tree does not has stable commit ids it is better to discuss things here:

> I'm wondering why we are still using old non-IIO driver? Can we switch to
> IIO-based one?

Is there an IIO based driver for these devices? AFAIK the drivers/misc driver was originally written for the HP implementation of these sensors (which was mostly for HDD fall protection) and the code ties into the HP ACPI tables, so switching to a new generic driver might not be so easy. There are a lot of HP models which use this driver, so I'm afraid there is a pretty big chance of regressions if we make drastic changes.
Comment 3 Andy Shevchenko 2020-10-27 17:49:22 UTC
(In reply to Hans de Goede from comment #2)
> Andy, I'm answering here instead of github, since my git tree does not has
> stable commit ids it is better to discuss things here:
> 
> > I'm wondering why we are still using old non-IIO driver? Can we switch to
> > IIO-based one?
> 
> Is there an IIO based driver for these devices?

Of course I have checked before replying here.
https://elixir.bootlin.com/linux/latest/source/drivers/iio/accel/st_accel_i2c.c


> AFAIK the drivers/misc
> driver was originally written for the HP implementation of these sensors
> (which was mostly for HDD fall protection) and the code ties into the HP
> ACPI tables, so switching to a new generic driver might not be so easy.
> There are a lot of HP models which use this driver, so I'm afraid there is a
> pretty big chance of regressions if we make drastic changes.

I see, but I think we might consider to go with an IIO driver for new models / laptops.
Comment 4 Hans de Goede 2020-10-27 19:12:24 UTC
Using the iio driver on newer models does sound like a good idea (assuming someone can test this for us) but how do we make the new driver bind on newer models and stick with the old one on older models ?

So I just checked and it seems the old code binds to something which is described as the ACPI_MDPS_CLASS ? And it also registers a led-class device for a hdd-fall-protection LED ?

This is all somewhat messy and without manpower + hw access to clean it up I guess we are just stuck with putting a bandaid on it when necessary. Which is less then ideal.

Alternatively I wonder if we should not just disable this driver in a lot of cases, it is already being ignored by userspace for screen rotation since it is not that kind of accelerometer ...
Comment 5 Andy Shevchenko 2022-08-29 13:05:37 UTC
Long time no news, should we close this report? Or what is the latest status?
Comment 6 Hans de Goede 2022-08-29 13:14:55 UTC
(In reply to Andy Shevchenko from comment #5)
> Long time no news, should we close this report? Or what is the latest status?

This has very likely been fixed already by:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=79d341e26ebcdbc622348aaaab6f8f89b6fdb25f

So lets close this.