On 21/10/2021 09:44, Oleksij Rempel wrote:

On Thu, Oct 21, 2021 at 09:41:35AM +0200, Daniel Lezcano wrote:

quoted

On 21/10/2021 09:20, Oleksij Rempel wrote:

quoted

Hi Petr,

On Wed, Oct 20, 2021 at 05:53:03PM +0200, Petr Benes wrote:

quoted

On Wed, 20 Oct 2021 at 07:05, Oleksij Rempel [off-list ref] wrote:

quoted

Hi Petr and Michal,

I forgot to add you for v2 in CC. Please test/review this version.

Hi Oleksij,

It works good. with PM as well as without PM. The only minor issue I found is,
that the first temperature reading (when the driver probes) fails. That is
(val & soc_data->temp_valid_mask) == 0) holds true. How does
pm_runtime_resume_and_get() behave in imx_thermal_probe()?
Does it go through imx_thermal_runtime_resume() with usleep_range()?

On the first temperature reading, the PM and part of HW is not
initialized. Current probe sequence is racy and has at least following
issues:
- thermal_zone_device_register is executed before HW init was completed.
  It kind of worked before my patch, becaus part of reinit was done by
  temperature init. It  worked, since the irq_enabled flag was not set,
  but potentially would run enable_irq() two times if device is
  overheated on probe.
- the imx_thermal core is potentially disable after first race
  condition:
  CPU0					CPU1
  thermal_zone_device_register()
					imx_get_temp()
  					irq_enabled == false
						power_up
						read_temp
  power_up
  						power_down
  irq_enabled = true;

  ... at this point imx_thermal is powered down for some amount of time,
  over temperature IRQ will not be triggered for some amount of time.

- if some part after thermal_zone_device_register() would fail or
  deferred, the worker polling temperature will run in to NULL pointer.
  This issue already happened...

After migrating to runtime PM, one of issues started to be visible even
on normal conditions.
I'll send one more patch with reworking probe sequence.

Are you planning to send a v3 with this patch? Or a separate patch?

I'm OK with both variants. What do you prefer?

I'll do i on top of PM patch to reduce refactoring overhead, if you OK
about it.

I prefer you resend a couple of patches but change the subject of this
patch to something like "thermal/drivers/imx: Fix disabled sensor after
handling trip temperature" in order to reflect the problem, not the
solution.

btw: nice fix



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel