[RFC] Make entity to interface links immutable

[RFC] Make entity to interface links immutable

[Hans Verkuil]

While working on v4l2-compliance I noticed that entity to interface links
have just the MEDIA_LNK_FL_ENABLED flag set.

Shouldn't we also set the MEDIA_LNK_FL_IMMUTABLE? After all, you cannot change
an entity-interface link. It feels inconsistent not to have this flag.

I also propose that media_create_intf_link() drops the last flags argument:
it can set the link flags directly since they are always the same anyway.

Regards,

	Hans

Re: [RFC] Make entity to interface links immutable

[Mauro Carvalho Chehab]

Em Mon, 2 Jul 2018 10:18:37 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> While working on v4l2-compliance I noticed that entity to interface links
> have just the MEDIA_LNK_FL_ENABLED flag set.
> 
> Shouldn't we also set the MEDIA_LNK_FL_IMMUTABLE? After all, you cannot change
> an entity-interface link. It feels inconsistent not to have this flag.

It could make sense for non-hybrid devices, but this may not be true
for hybrid ones. See below.

> I also propose that media_create_intf_link() drops the last flags argument:
> it can set the link flags directly since they are always the same anyway.

When we came with this design, the idea is that an interface can be 
disabled/enabled at runtime, if the entity it links can't be used,
because the hardware is busy doing something else. 

That could happen with hybrid devices, where the analog part could
be consuming resources that would be needed for the digital part.

Disabling the link at runtime has an advantage that it makes easier
to check - as open() syscalls could just use it to return -EBUSY,
instead of doing a complete graph analysis. Also, applications can
test it directly, in order to have a hint if a device is ready for
usage.

That was one of the approaches we considered at the design, but I
don't remember if Shuah's patch series actually used it or not,
as I don't look at her pending patches for a long time. I suspect
she took a different approach.

Anyway, before touching it, I'd like to see her patches merged,
and do some tests with real case scenarios before changing it.


Thanks,
Mauro

Re: [RFC] Make entity to interface links immutable

[Hans Verkuil]

On 02/07/18 11:41, Mauro Carvalho Chehab wrote:
> Em Mon, 2 Jul 2018 10:18:37 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> While working on v4l2-compliance I noticed that entity to interface links
>> have just the MEDIA_LNK_FL_ENABLED flag set.
>>
>> Shouldn't we also set the MEDIA_LNK_FL_IMMUTABLE? After all, you cannot change
>> an entity-interface link. It feels inconsistent not to have this flag.
> 
> It could make sense for non-hybrid devices, but this may not be true
> for hybrid ones. See below.
> 
>> I also propose that media_create_intf_link() drops the last flags argument:
>> it can set the link flags directly since they are always the same anyway.
> 
> When we came with this design, the idea is that an interface can be 
> disabled/enabled at runtime, if the entity it links can't be used,
> because the hardware is busy doing something else. 
> 
> That could happen with hybrid devices, where the analog part could
> be consuming resources that would be needed for the digital part.
> 
> Disabling the link at runtime has an advantage that it makes easier
> to check - as open() syscalls could just use it to return -EBUSY,
> instead of doing a complete graph analysis. Also, applications can
> test it directly, in order to have a hint if a device is ready for
> usage.
> 
> That was one of the approaches we considered at the design, but I
> don't remember if Shuah's patch series actually used it or not,
> as I don't look at her pending patches for a long time. I suspect
> she took a different approach.
> 
> Anyway, before touching it, I'd like to see her patches merged,
> and do some tests with real case scenarios before changing it.

Hmm, this also highlights another deficiency in the spec. Currently
the description of IMMUTABLE says:

"The link enabled state can’t be modified at runtime. An immutable link
 is always enabled."

But in the plan above the link can change, but only the driver can
enable/disable the link. It makes no sense AFAICS for userspace to
enable/disable an interface link.

If I am right that it makes no sense for userspace to disable an interface
link, then we should update the spec to clarify that this is not allowed
(in fact, it is not possible today anyway). And we should also clarify
that the driver can disable an interface link and what that means.

If userspace CAN in some circumstances disable an interface link, then
we need to add something like a READ_ONLY flag to signal whether or not
userspace can change an interface link. If it is READ_ONLY, then only the
driver can change that.

That said, given that today there are no drivers that disable an interface
link, I still think that we should mark them all as IMMUTABLE. That flag
can be removed when we actually let drivers change this.

It would be consistent with the current usage of interface links.

Regards,

	Hans

Re: [RFC] Make entity to interface links immutable

[Mauro Carvalho Chehab]

Em Mon, 2 Jul 2018 12:38:23 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 02/07/18 11:41, Mauro Carvalho Chehab wrote:
> > Em Mon, 2 Jul 2018 10:18:37 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> While working on v4l2-compliance I noticed that entity to interface links
> >> have just the MEDIA_LNK_FL_ENABLED flag set.
> >>
> >> Shouldn't we also set the MEDIA_LNK_FL_IMMUTABLE? After all, you cannot change
> >> an entity-interface link. It feels inconsistent not to have this flag.  
> > 
> > It could make sense for non-hybrid devices, but this may not be true
> > for hybrid ones. See below.
> >   
> >> I also propose that media_create_intf_link() drops the last flags argument:
> >> it can set the link flags directly since they are always the same anyway.  
> > 
> > When we came with this design, the idea is that an interface can be 
> > disabled/enabled at runtime, if the entity it links can't be used,
> > because the hardware is busy doing something else. 
> > 
> > That could happen with hybrid devices, where the analog part could
> > be consuming resources that would be needed for the digital part.
> > 
> > Disabling the link at runtime has an advantage that it makes easier
> > to check - as open() syscalls could just use it to return -EBUSY,
> > instead of doing a complete graph analysis. Also, applications can
> > test it directly, in order to have a hint if a device is ready for
> > usage.
> > 
> > That was one of the approaches we considered at the design, but I
> > don't remember if Shuah's patch series actually used it or not,
> > as I don't look at her pending patches for a long time. I suspect
> > she took a different approach.
> > 
> > Anyway, before touching it, I'd like to see her patches merged,
> > and do some tests with real case scenarios before changing it.  
> 
> Hmm, this also highlights another deficiency in the spec. Currently
> the description of IMMUTABLE says:
> 
> "The link enabled state can’t be modified at runtime. An immutable link
>  is always enabled."
> 
> But in the plan above the link can change, but only the driver can
> enable/disable the link. It makes no sense AFAICS for userspace to
> enable/disable an interface link.
> 
> If I am right that it makes no sense for userspace to disable an interface
> link, then we should update the spec to clarify that this is not allowed
> (in fact, it is not possible today anyway). And we should also clarify
> that the driver can disable an interface link and what that means.
> 
> If userspace CAN in some circumstances disable an interface link, then
> we need to add something like a READ_ONLY flag to signal whether or not
> userspace can change an interface link. If it is READ_ONLY, then only the
> driver can change that.

Yes, this makes sense.

> 
> That said, given that today there are no drivers that disable an interface
> link, I still think that we should mark them all as IMMUTABLE. That flag
> can be removed when we actually let drivers change this.

Ok, let's do this, but without touching at the media_create_intf_link(),
as we need first to apply Shuah's patch, and see how we'll handle,
before start stripping code that will probably be needed.

> 
> It would be consistent with the current usage of interface links.
> 
> Regards,
> 
> 	Hans



Thanks,
Mauro

Re: [RFC] Make entity to interface links immutable

[Shuah Khan]

Hi Mauro and Hans,

On 07/02/2018 03:41 AM, Mauro Carvalho Chehab wrote:
> Em Mon, 2 Jul 2018 10:18:37 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> While working on v4l2-compliance I noticed that entity to interface links
>> have just the MEDIA_LNK_FL_ENABLED flag set.
>>
>> Shouldn't we also set the MEDIA_LNK_FL_IMMUTABLE? After all, you cannot change
>> an entity-interface link. It feels inconsistent not to have this flag.
> 
> It could make sense for non-hybrid devices, but this may not be true
> for hybrid ones. See below.
> 
>> I also propose that media_create_intf_link() drops the last flags argument:
>> it can set the link flags directly since they are always the same anyway.
> 
> When we came with this design, the idea is that an interface can be 
> disabled/enabled at runtime, if the entity it links can't be used,
> because the hardware is busy doing something else. 
> 
> That could happen with hybrid devices, where the analog part could
> be consuming resources that would be needed for the digital part.
> 
> Disabling the link at runtime has an advantage that it makes easier
> to check - as open() syscalls could just use it to return -EBUSY,
> instead of doing a complete graph analysis. Also, applications can
> test it directly, in order to have a hint if a device is ready for
> usage.
> 
> That was one of the approaches we considered at the design, but I
> don't remember if Shuah's patch series actually used it or not,
> as I don't look at her pending patches for a long time. I suspect
> she took a different approach.
> 
> Anyway, before touching it, I'd like to see her patches merged,
> and do some tests with real case scenarios before changing it.
> 

I have been sidetracked by USB over IP security vulnerability reports
and fixing them since last November. Sorry for not picking this work
back up.

I am back from a week of vacation and I will dig my patches and respond
with details in a couple of days.

thanks for your patience,
-- Shuah