Hi Sakari,
On Monday 19 July 2010 13:20:33 Sakari Ailus wrote:
> 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.
To summarize the discussion, what should I change here ? Just remove the "ret
= " in the second set_power call ?
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
