[PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Hans Verkuil]

sd->entity.graph_obj.mdev can be NULL when this function is called, and
that breaks existing drivers (rcar-vin, but probably others as well).

Check if sd->entity.num_pads is non-zero instead since that doesn't depend
on mdev.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in v4l2_subdev_call()")
Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
Changes in v2:

- Don't change the coding style, just fix the bug.
---
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 21fb90d66bfc..25c73c13cc7e 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -124,7 +124,7 @@ static inline int check_which(__u32 which)
 static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
 {
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (sd->entity.graph_obj.mdev) {
+	if (sd->entity.num_pads) {
 		if (pad >= sd->entity.num_pads)
 			return -EINVAL;
 		return 0;

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Janusz Krzysztofik]

Hi Sakari,

On Monday, July 1, 2019 9:50:55 A.M. CEST Sakari Ailus wrote:
> Hi Hans,
> 
> On Sat, Jun 29, 2019 at 03:08:23PM +0200, Hans Verkuil wrote:
> > On 6/29/19 2:57 PM, Hans Verkuil wrote:
> > > On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
> > >> Hi Hans,
> > >> 
> > >> On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
> > >>> sd->entity.graph_obj.mdev can be NULL when this function is called,
> > >>> and
> > >>> that breaks existing drivers (rcar-vin, but probably others as well).
> > >>> 
> > >>> Check if sd->entity.num_pads is non-zero instead since that doesn't
> > >>> depend
> > >>> on mdev.
> > >>> 
> > >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > >>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >>> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
> > >>> v4l2_subdev_call()") ---
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> b/drivers/media/v4l2-core/v4l2-subdev.c index
> > >>> 21fb90d66bfc..bc32fc1e0c0b
> > >>> 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > >>> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
> > >>> 
> > >>>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> > >>>  {
> > >>>  #if defined(CONFIG_MEDIA_CONTROLLER)
> > >>> 
> > >>> -	if (sd->entity.graph_obj.mdev) {
> > >>> -		if (pad >= sd->entity.num_pads)
> > >>> -			return -EINVAL;
> > >>> -		return 0;
> > >>> -	}
> > >>> +	if (sd->entity.num_pads)
> > >> 
> > >> This reverts a change introduced on Sakari's request in v7 of the
> > >> series which is the source of the regression.  The intention was to
> > >> fail if num_pads == 0 on a valid media entity. Maybe we should still
> > >> keep that restriction and fail in case mdev is not NULL? In other
> > >> words:
> > >> 
> > >> -	if (sd->entity.num_pads)
> > >> +	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)
> > > 
> > > If an entity has no pads, then it doesn't have pad ops either and this
> > > function would never be called.
> > 
> > Sakari, do you think it is ever possible that an entity may have pad ops,
> > but num_pads goes at some point to 0?
> > 
> > If so, then we can check for sd->entity.function != 0. All MC subdevs must
> > set that to a non-0 value, otherwise the core will issue a WARN_ON. I
> > think
> > it is the only reliable indicator that can be used.
> 
> I don't think checking the num_pads field is is a great way to test whether
> an entity supports the pad ops; I don't have a better proposal either as it
> seems that some drivers call these ops before registering the subdev.
> 
> There are sub-device drivers that expose their own device node and thus
> work with MC-enabled master drivers but have no pad ops: this is what makes
> the fundamental difference in API support, it's not the number of pads. We
> just happen to see this in pad ops only (I guess).
> 
> Currently the drivers may set the HAS_DEVNODE flag (that really tells about
> MC compatibility) just before registering the sub-device. This might be a
> better test for allowing pad ops, but the name of the flag is somewhat
> misleading. I wouldn't want to start testing this now however, it'd risk
> exposing these issues to a much wider audience.
> 
> I guess in practice testing for num_pads still works; it however does leave
> some gray area between MC-enabled sub-device drivers and the rest. 

That's why I proposed to cover that area by still checking for a non-NULL mdev 
member alternatively.

Are you sure HAS_DEVNODE flag is applicable only for media entities? AFAICS 
subdev drivers may expose some IOCTLs regardless of CONFIG_MEDIA_CONTROLLER 
and CONFIG_V4L2_SUBDEV_API settings.

Thanks,
Janusz


> Perhaps
> something to improve in the future, but for a later kernel release.
> 
> So,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Sakari Ailus]

Hi Hans,

On Sat, Jun 29, 2019 at 03:08:23PM +0200, Hans Verkuil wrote:
> On 6/29/19 2:57 PM, Hans Verkuil wrote:
> > On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
> >> Hi Hans,
> >>
> >> On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
> >>> sd->entity.graph_obj.mdev can be NULL when this function is called, and
> >>> that breaks existing drivers (rcar-vin, but probably others as well).
> >>>
> >>> Check if sd->entity.num_pads is non-zero instead since that doesn't depend
> >>> on mdev.
> >>>
> >>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
> >>> v4l2_subdev_call()") ---
> >>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
> >>> 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
> >>>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> >>>  {
> >>>  #if defined(CONFIG_MEDIA_CONTROLLER)
> >>> -	if (sd->entity.graph_obj.mdev) {
> >>> -		if (pad >= sd->entity.num_pads)
> >>> -			return -EINVAL;
> >>> -		return 0;
> >>> -	}
> >>> +	if (sd->entity.num_pads)
> >>
> >> This reverts a change introduced on Sakari's request in v7 of the series which 
> >> is the source of the regression.  The intention was to fail if num_pads == 0 
> >> on a valid media entity. Maybe we should still keep that restriction and fail 
> >> in case mdev is not NULL? In other words: 
> >>
> >> -	if (sd->entity.num_pads)
> >> +	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)
> > 
> > If an entity has no pads, then it doesn't have pad ops either and this function
> > would never be called.
> 
> Sakari, do you think it is ever possible that an entity may have pad ops,
> but num_pads goes at some point to 0?
> 
> If so, then we can check for sd->entity.function != 0. All MC subdevs must
> set that to a non-0 value, otherwise the core will issue a WARN_ON. I think
> it is the only reliable indicator that can be used.

I don't think checking the num_pads field is is a great way to test whether
an entity supports the pad ops; I don't have a better proposal either as it
seems that some drivers call these ops before registering the subdev.

There are sub-device drivers that expose their own device node and thus
work with MC-enabled master drivers but have no pad ops: this is what makes
the fundamental difference in API support, it's not the number of pads. We
just happen to see this in pad ops only (I guess).

Currently the drivers may set the HAS_DEVNODE flag (that really tells about
MC compatibility) just before registering the sub-device. This might be a
better test for allowing pad ops, but the name of the flag is somewhat
misleading. I wouldn't want to start testing this now however, it'd risk
exposing these issues to a much wider audience.

I guess in practice testing for num_pads still works; it however does leave
some gray area between MC-enabled sub-device drivers and the rest. Perhaps
something to improve in the future, but for a later kernel release.

So,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Hans Verkuil]

On 6/29/19 3:13 PM, Janusz Krzysztofik wrote:
> On Saturday, June 29, 2019 2:57:09 P.M. CEST Hans Verkuil wrote:
>> On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
>>> Hi Hans,
>>>
>>> On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
>>>> sd->entity.graph_obj.mdev can be NULL when this function is called, and
>>>> that breaks existing drivers (rcar-vin, but probably others as well).
>>>>
>>>> Check if sd->entity.num_pads is non-zero instead since that doesn't
>>>> depend
>>>> on mdev.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
>>>> v4l2_subdev_call()") ---
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
>>>> 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
>>>>
>>>>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
>>>>  {
>>>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>>>>
>>>> -	if (sd->entity.graph_obj.mdev) {
>>>> -		if (pad >= sd->entity.num_pads)
>>>> -			return -EINVAL;
>>>> -		return 0;
>>>> -	}
>>>> +	if (sd->entity.num_pads)
>>>
>>> This reverts a change introduced on Sakari's request in v7 of the series
>>> which is the source of the regression.  The intention was to fail if
>>> num_pads == 0 on a valid media entity. Maybe we should still keep that
>>> restriction and fail in case mdev is not NULL? In other words:
>>>
>>> -	if (sd->entity.num_pads)
>>> +	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)
>>
>> If an entity has no pads, then it doesn't have pad ops either and this
>> function would never be called.
> 
> Unless this is a subdevice which doesn't support MC, was updated in the past 
> to use pad ops instead of depreciated video ops, so it actually has pad ops 
> even if it has num_pads == 0, and is built by a user with 
> CONFIG_MEDIA_CONTROLLER=y for some reason.

That's fine. Then it just checks if pad == 0, which is OK for such drivers.

The issue here is a MC-enabled subdev with pad ops, but that really has no pads
for some reason, so check_pad() would always have to return -EINVAL.

Regards,

	Hans

> 
> Thanks,
> Janusz
> 
>>
>>> Thanks,
>>> Janusz
>>>
>>>> +		return pad >= sd->entity.num_pads ? -EINVAL : 0;
>>>
>>> This and below look like coding style changes, not related strictly to the
>>> merit.  Shouldn't they rather be split into a separate patch?
>>
>> I'll post a v2, the diff is a lot smaller. I might post a follow-up patch
>> to use ? : since that's a lot shorter code.
>>
>> Regards,
>>
>> 	Hans
>>
>>> BTW, the current coding style follows original implementation of check_*
>>> functions present before that series was introduced.  Maybe it would be
>>> better to keep them unified, i.e., either as is or all updated with the
>>> new style.
>>>
>>> Thanks,
>>> Janusz
>>>
>>>>  #endif
>>>>  
>>>>  	/* allow pad 0 on subdevices not registered as media entities */
>>>>
>>>> -	if (pad > 0)
>>>> -		return -EINVAL;
>>>> -	return 0;
>>>> +	return pad ? -EINVAL : 0;
>>>>
>>>>  }
>>>>  
>>>>  static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
> 
> 
> 
> 

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Janusz Krzysztofik]

On Saturday, June 29, 2019 2:57:09 P.M. CEST Hans Verkuil wrote:
> On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
> > Hi Hans,
> > 
> > On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
> >> sd->entity.graph_obj.mdev can be NULL when this function is called, and
> >> that breaks existing drivers (rcar-vin, but probably others as well).
> >> 
> >> Check if sd->entity.num_pads is non-zero instead since that doesn't
> >> depend
> >> on mdev.
> >> 
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
> >> v4l2_subdev_call()") ---
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> >> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
> >> 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
> >> 
> >>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
> >>  {
> >>  #if defined(CONFIG_MEDIA_CONTROLLER)
> >> 
> >> -	if (sd->entity.graph_obj.mdev) {
> >> -		if (pad >= sd->entity.num_pads)
> >> -			return -EINVAL;
> >> -		return 0;
> >> -	}
> >> +	if (sd->entity.num_pads)
> > 
> > This reverts a change introduced on Sakari's request in v7 of the series
> > which is the source of the regression.  The intention was to fail if
> > num_pads == 0 on a valid media entity. Maybe we should still keep that
> > restriction and fail in case mdev is not NULL? In other words:
> > 
> > -	if (sd->entity.num_pads)
> > +	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)
> 
> If an entity has no pads, then it doesn't have pad ops either and this
> function would never be called.

Unless this is a subdevice which doesn't support MC, was updated in the past 
to use pad ops instead of depreciated video ops, so it actually has pad ops 
even if it has num_pads == 0, and is built by a user with 
CONFIG_MEDIA_CONTROLLER=y for some reason.

Thanks,
Janusz

> 
> > Thanks,
> > Janusz
> > 
> >> +		return pad >= sd->entity.num_pads ? -EINVAL : 0;
> > 
> > This and below look like coding style changes, not related strictly to the
> > merit.  Shouldn't they rather be split into a separate patch?
> 
> I'll post a v2, the diff is a lot smaller. I might post a follow-up patch
> to use ? : since that's a lot shorter code.
> 
> Regards,
> 
> 	Hans
> 
> > BTW, the current coding style follows original implementation of check_*
> > functions present before that series was introduced.  Maybe it would be
> > better to keep them unified, i.e., either as is or all updated with the
> > new style.
> > 
> > Thanks,
> > Janusz
> > 
> >>  #endif
> >>  
> >>  	/* allow pad 0 on subdevices not registered as media entities */
> >> 
> >> -	if (pad > 0)
> >> -		return -EINVAL;
> >> -	return 0;
> >> +	return pad ? -EINVAL : 0;
> >> 
> >>  }
> >>  
> >>  static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Hans Verkuil]

On 6/29/19 2:57 PM, Hans Verkuil wrote:
> On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
>> Hi Hans,
>>
>> On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
>>> sd->entity.graph_obj.mdev can be NULL when this function is called, and
>>> that breaks existing drivers (rcar-vin, but probably others as well).
>>>
>>> Check if sd->entity.num_pads is non-zero instead since that doesn't depend
>>> on mdev.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
>>> v4l2_subdev_call()") ---
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>>> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
>>> 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
>>>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
>>>  {
>>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>>> -	if (sd->entity.graph_obj.mdev) {
>>> -		if (pad >= sd->entity.num_pads)
>>> -			return -EINVAL;
>>> -		return 0;
>>> -	}
>>> +	if (sd->entity.num_pads)
>>
>> This reverts a change introduced on Sakari's request in v7 of the series which 
>> is the source of the regression.  The intention was to fail if num_pads == 0 
>> on a valid media entity. Maybe we should still keep that restriction and fail 
>> in case mdev is not NULL? In other words: 
>>
>> -	if (sd->entity.num_pads)
>> +	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)
> 
> If an entity has no pads, then it doesn't have pad ops either and this function
> would never be called.

Sakari, do you think it is ever possible that an entity may have pad ops,
but num_pads goes at some point to 0?

If so, then we can check for sd->entity.function != 0. All MC subdevs must
set that to a non-0 value, otherwise the core will issue a WARN_ON. I think
it is the only reliable indicator that can be used.

Regards,

	Hans

> 
>>
>> Thanks,
>> Janusz
>>
>>> +		return pad >= sd->entity.num_pads ? -EINVAL : 0;
>>
>> This and below look like coding style changes, not related strictly to the 
>> merit.  Shouldn't they rather be split into a separate patch?
> 
> I'll post a v2, the diff is a lot smaller. I might post a follow-up patch
> to use ? : since that's a lot shorter code.
> 
> Regards,
> 
> 	Hans
> 
>>
>> BTW, the current coding style follows original implementation of check_* 
>> functions present before that series was introduced.  Maybe it would be better 
>> to keep them unified, i.e., either as is or all updated with the new style.
>>
>> Thanks,
>> Janusz
>>
>>>  #endif
>>>  	/* allow pad 0 on subdevices not registered as media entities */
>>> -	if (pad > 0)
>>> -		return -EINVAL;
>>> -	return 0;
>>> +	return pad ? -EINVAL : 0;
>>>  }
>>>
>>>  static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
>>
>>
>>
>>
> 

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Hans Verkuil]

On 6/29/19 2:06 PM, Janusz Krzysztofik wrote:
> Hi Hans,
> 
> On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
>> sd->entity.graph_obj.mdev can be NULL when this function is called, and
>> that breaks existing drivers (rcar-vin, but probably others as well).
>>
>> Check if sd->entity.num_pads is non-zero instead since that doesn't depend
>> on mdev.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
>> v4l2_subdev_call()") ---
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
>> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
>> 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
>>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
>>  {
>>  #if defined(CONFIG_MEDIA_CONTROLLER)
>> -	if (sd->entity.graph_obj.mdev) {
>> -		if (pad >= sd->entity.num_pads)
>> -			return -EINVAL;
>> -		return 0;
>> -	}
>> +	if (sd->entity.num_pads)
> 
> This reverts a change introduced on Sakari's request in v7 of the series which 
> is the source of the regression.  The intention was to fail if num_pads == 0 
> on a valid media entity. Maybe we should still keep that restriction and fail 
> in case mdev is not NULL? In other words: 
> 
> -	if (sd->entity.num_pads)
> +	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)

If an entity has no pads, then it doesn't have pad ops either and this function
would never be called.

> 
> Thanks,
> Janusz
> 
>> +		return pad >= sd->entity.num_pads ? -EINVAL : 0;
> 
> This and below look like coding style changes, not related strictly to the 
> merit.  Shouldn't they rather be split into a separate patch?

I'll post a v2, the diff is a lot smaller. I might post a follow-up patch
to use ? : since that's a lot shorter code.

Regards,

	Hans

> 
> BTW, the current coding style follows original implementation of check_* 
> functions present before that series was introduced.  Maybe it would be better 
> to keep them unified, i.e., either as is or all updated with the new style.
> 
> Thanks,
> Janusz
> 
>>  #endif
>>  	/* allow pad 0 on subdevices not registered as media entities */
>> -	if (pad > 0)
>> -		return -EINVAL;
>> -	return 0;
>> +	return pad ? -EINVAL : 0;
>>  }
>>
>>  static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)
> 
> 
> 
> 

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Janusz Krzysztofik]

Hi Hans,

On Saturday, June 29, 2019 12:00:24 P.M. CEST Hans Verkuil wrote:
> sd->entity.graph_obj.mdev can be NULL when this function is called, and
> that breaks existing drivers (rcar-vin, but probably others as well).
> 
> Check if sd->entity.num_pads is non-zero instead since that doesn't depend
> on mdev.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in
> v4l2_subdev_call()") ---
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c
> b/drivers/media/v4l2-core/v4l2-subdev.c index 21fb90d66bfc..bc32fc1e0c0b
> 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
>  {
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (sd->entity.graph_obj.mdev) {
> -		if (pad >= sd->entity.num_pads)
> -			return -EINVAL;
> -		return 0;
> -	}
> +	if (sd->entity.num_pads)

This reverts a change introduced on Sakari's request in v7 of the series which 
is the source of the regression.  The intention was to fail if num_pads == 0 
on a valid media entity. Maybe we should still keep that restriction and fail 
in case mdev is not NULL? In other words: 

-	if (sd->entity.num_pads)
+	if (sd->entity.num_pads || sd->entity.graph_obj.mdev)

Thanks,
Janusz

> +		return pad >= sd->entity.num_pads ? -EINVAL : 0;

This and below look like coding style changes, not related strictly to the 
merit.  Shouldn't they rather be split into a separate patch?

BTW, the current coding style follows original implementation of check_* 
functions present before that series was introduced.  Maybe it would be better 
to keep them unified, i.e., either as is or all updated with the new style.

Thanks,
Janusz

>  #endif
>  	/* allow pad 0 on subdevices not registered as media entities */
> -	if (pad > 0)
> -		return -EINVAL;
> -	return 0;
> +	return pad ? -EINVAL : 0;
>  }
> 
>  static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)

Re: [PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Niklas Söderlund]

Hi Hans,

Thanks for your work.

On 2019-06-29 12:00:24 +0200, Hans Verkuil wrote:
> sd->entity.graph_obj.mdev can be NULL when this function is called, and
> that breaks existing drivers (rcar-vin, but probably others as well).
> 
> Check if sd->entity.num_pads is non-zero instead since that doesn't depend
> on mdev.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in v4l2_subdev_call()")

This fixes my problem,

Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 21fb90d66bfc..bc32fc1e0c0b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
>  static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
>  {
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (sd->entity.graph_obj.mdev) {
> -		if (pad >= sd->entity.num_pads)
> -			return -EINVAL;
> -		return 0;
> -	}
> +	if (sd->entity.num_pads)
> +		return pad >= sd->entity.num_pads ? -EINVAL : 0;
>  #endif
>  	/* allow pad 0 on subdevices not registered as media entities */
> -	if (pad > 0)
> -		return -EINVAL;
> -	return 0;
> +	return pad ? -EINVAL : 0;
>  }
> 
>  static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)

-- 
Regards,
Niklas Söderlund

[PATCH for v5.3] v4l2-subdev: fix regression in check_pad()

[Hans Verkuil]

sd->entity.graph_obj.mdev can be NULL when this function is called, and
that breaks existing drivers (rcar-vin, but probably others as well).

Check if sd->entity.num_pads is non-zero instead since that doesn't depend
on mdev.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Fixes: a8fa55078a77 ("media: v4l2-subdev: Verify arguments in v4l2_subdev_call()")
---
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 21fb90d66bfc..bc32fc1e0c0b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -124,16 +124,11 @@ static inline int check_which(__u32 which)
 static inline int check_pad(struct v4l2_subdev *sd, __u32 pad)
 {
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (sd->entity.graph_obj.mdev) {
-		if (pad >= sd->entity.num_pads)
-			return -EINVAL;
-		return 0;
-	}
+	if (sd->entity.num_pads)
+		return pad >= sd->entity.num_pads ? -EINVAL : 0;
 #endif
 	/* allow pad 0 on subdevices not registered as media entities */
-	if (pad > 0)
-		return -EINVAL;
-	return 0;
+	return pad ? -EINVAL : 0;
 }

 static int check_cfg(__u32 which, struct v4l2_subdev_pad_config *cfg)