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

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

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.

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.

...

>> +static void media_entity_power_disconnect(struct media_entity *one,
>> +					  struct media_entity *theother)
>> +{
>> +	int power_one = media_entity_count_node(one);
>> +	int power_theother = media_entity_count_node(theother);
>> +
>> +	media_entity_power_apply(one, -power_theother);
>> +	media_entity_power_apply(theother, -power_one);
> 
> Needs a comment why the return code is not checked.

Same reason here actually. Agreed on the comment.

Regards,

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