Re: [RFC/PATCH 05/10] media: Reference count and power handling

[Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>]

Hans Verkuil wrote:
> 
>> Hi Hans,
>>
>> And thanks for the comments!
>>
>> Hans Verkuil wrote:
>> ...
>>>> +/*
>>>> + * Apply use count change to an entity and change power state based on
>>>> + * new use count.
>>>> + */
>>>> +static int media_entity_power_apply_one(struct media_entity *entity,
>>>> int change)
>>>> +{
>>>> +	int ret = 0;
>>>> +
>>>> +	if (entity->use_count == 0 && change > 0 &&
>>>> +	    entity->ops && entity->ops->set_power) {
>>>> +		ret = entity->ops->set_power(entity, 1);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	media_entity_use_apply_one(entity, change);
>>>> +
>>>> +	if (entity->use_count == 0 && change < 0 &&
>>>> +	    entity->ops && entity->ops->set_power)
>>>> +		ret = entity->ops->set_power(entity, 0);
>>>
>>> Shouldn't this code be executed before the call to
>>> media_entity_use_apply_one()?
>>> Or at least call media_entity_use_apply_one(entity, -change) in case of
>>> an
>>> error? Since it failed to power off the entity it should ensure that the
>>> use_count
>>> remains > 0.

Forgot to answer this one.

The first entity->ops->set_power() is called to power on the device.
This will be done when the use_count was 0 and change is positive, i.e.
we're asked to power on the entity. The actual use count is not changed
in case of a failure.

The latter entity->ops->set_power() is called to power the device off.
media_entity_use_apply_one() is called first to apply the change since
the change isn't necessarily -1 or 1.The change was already applied to
the entity->use_count that's why the comparison against 0. And the
change was negative, i.e. the device is to be powered off.

So I don't think there's an error in use_count calculation. But the
second set_power() to power the device off shouldn't set ret at all and
the function should return zero at the end.

Then I think it'd be correct. Things can currently go wrong when
media_entity_power_apply() is called from
media_entity_node_power_change() with a negative change.

>>
>> My assumption originally was that powering the device off always
>> succeeds. Things become interesting if powering off really fails. For
>> example, the power state is related to open file handles. Do you deny
>> closing the file handle if the related power state change isn't possible?
>>
>> It's indeed a good question what should be done in that case. Some
>> things do go wrong there anyway. I could think of leaving it for drivers
>> themselves to handle if there's something that can be done.
>>
>> The power state change for a device can involve an I2C transaction, for
>> example, which really can fail in practice.
> 
> There are two issue here: my comment above was limited to the fact that as
> far as I can see the use_count will be off by one if the power off fails.
> That should be fixed.
>
> The other issue is what to do when power off fails. There are a few
> possible schemes:
> 
> 1) Just power off everything, ignoring any failures (although they should
> be reported in the kernel log). Probably what you want in practice.
> 2) Revert to the previous state (that's what happening in your code).
> Sounds nice, but what do you do next?

In one case, yes, as far as I can see, entities could have been powered
on again as a result of a failed power off. But that's a bug. :-)

> 3) Add a boolean to choose whether to forcefully power off everything
> (i.e. case 1), or report an error and restore the state (case 2).
> 
> Frankly, I'm in favor of the simple solution: case 1. When you get i2c
> errors when powering off you are probably experiencing lots of other
> problems as well.

I fully agree.

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com