ACPI power-resources not turned off after failure of device-driver's runtime resume function?

ACPI power-resources not turned off after failure of device-driver's runtime resume function?

[Hans de Goede]

Hi Rafael,

While testing error handling in the atomisp driver, deliberately
making a camera-sensor driver not work by using gpioset to disable
a regulator, I noticed the following:

The ACPI device pm_domain runtime_resume function looks like this:

int acpi_subsys_runtime_resume(struct device *dev)
{
        int ret = acpi_dev_resume(dev);

        return ret ? ret : pm_generic_runtime_resume(dev);
}

Combined with the runtime_error flag blocking any futher
runtime-pm get() / put() calls (making them fail with -EINVAL)
this means that if the device's driver runtime resume
function called by pm_generic_runtime_resume() fails,
nothing will undo the acpi_dev_resume() leaving e.g.
ACPI power-resouces associated with dev in the on state.

I guess a possible fix would be to change
acpi_subsys_runtime_resume() to e.g.:

int acpi_subsys_runtime_resume(struct device *dev)
{
        int ret;

	ret = acpi_dev_resume(dev);
	if (ret)
		return ret;
		
	ret = pm_generic_runtime_resume(dev);
	if (ret)
		acpi_dev_suspend(dev);

        return ret;
}

but I'm not sure if this is the correct fix ?

Or is the assumption simply that if things go wrong it
is best to just leave everything as it us to avoid making
things worse? Similar to how this will result in the
runtime_error flag getting set disallowing further
runtime-pm get()/put() calls ?

###

About the runtime_error flag I also noticed that if I enable
the deliberately disabled regulator after first testing
the error handling then subsequent attempts to stream from
the sensor will fail because pm_runtime_get_sync() fails
with -EINVAL due to the runtime_error flag. I guess this is
deliberate and drivers can then e.g. queue a workqueue item
to do a full reset and then clear the runtime_error after that?

And it seems that the runtime-error also needs to be cleared
followed by a successful pm-runtime get() + put() pair 
to release the ACPI power-resources.

So I guess that drivers where errors may be intermittent and
the runtime-resume should be retried the next time, something
like this should be used: ? 

        ret = pm_runtime_resume_and_get(&sensor->client->dev);
        if (ret) {
                /* clear runtime error to retry resume the next time */
                pm_runtime_set_suspended(&sensor->client->dev);
                return ret;
        }

Regards,

Hans