diff mbox series

[v2,09/26] media: v4l: async: refactor v4l2_async_create_ancillary_links

Message ID 20240419-fix-cocci-v2-9-2119e692309c@chromium.org (mailing list archive)
State Superseded
Headers show
Series media: Fix coccinelle warning/errors | expand

Commit Message

Ricardo Ribalda April 19, 2024, 9:47 a.m. UTC
Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
is not enabled.

This makes cocci happier:

drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/v4l2-core/v4l2-async.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Bryan O'Donoghue April 20, 2024, 11:23 p.m. UTC | #1
On 19/04/2024 10:47, Ricardo Ribalda wrote:
> Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
> is not enabled.
> 
> This makes cocci happier:
> 
> drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>   drivers/media/v4l2-core/v4l2-async.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 4bb073587817..915a9f3ea93c 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -316,9 +316,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
>   static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>   					     struct v4l2_subdev *sd)
>   {
> -	struct media_link *link = NULL;
> +	struct media_link *link;
>   
> -#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> +	if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
> +		return 0;
>   
>   	if (sd->entity.function != MEDIA_ENT_F_LENS &&
>   	    sd->entity.function != MEDIA_ENT_F_FLASH)
> @@ -326,8 +327,6 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>   
>   	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>   
> -#endif
> -
>   	return IS_ERR(link) ? PTR_ERR(link) : 0;
>   }
>   
> 

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

---
bod
Hans Verkuil April 24, 2024, 10:55 a.m. UTC | #2
On 19/04/2024 11:47, Ricardo Ribalda wrote:
> Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
> is not enabled.
> 
> This makes cocci happier:
> 
> drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 4bb073587817..915a9f3ea93c 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -316,9 +316,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
>  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>  					     struct v4l2_subdev *sd)
>  {
> -	struct media_link *link = NULL;
> +	struct media_link *link;
>  
> -#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> +	if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
> +		return 0;
>  
>  	if (sd->entity.function != MEDIA_ENT_F_LENS &&
>  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> @@ -326,8 +327,6 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>  
>  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>  
> -#endif
> -
>  	return IS_ERR(link) ? PTR_ERR(link) : 0;
>  }

I think I would prefer:

static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
					     struct v4l2_subdev *sd)
{
#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
	struct media_link *link;

	...

	return IS_ERR(link) ? PTR_ERR(link) : 0;
#else
	return 0;
#endif
}

Regards,

	Hans
Sakari Ailus April 24, 2024, 6:17 p.m. UTC | #3
Hi Hans,

On Wed, Apr 24, 2024 at 12:55:20PM +0200, Hans Verkuil wrote:
> On 19/04/2024 11:47, Ricardo Ribalda wrote:
> > Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
> > is not enabled.
> > 
> > This makes cocci happier:
> > 
> > drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319
> > 
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 4bb073587817..915a9f3ea93c 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -316,9 +316,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
> >  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> >  					     struct v4l2_subdev *sd)
> >  {
> > -	struct media_link *link = NULL;
> > +	struct media_link *link;
> >  
> > -#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> > +	if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
> > +		return 0;
> >  
> >  	if (sd->entity.function != MEDIA_ENT_F_LENS &&
> >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > @@ -326,8 +327,6 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> >  
> >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> >  
> > -#endif
> > -
> >  	return IS_ERR(link) ? PTR_ERR(link) : 0;
> >  }
> 
> I think I would prefer:
> 
> static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> 					     struct v4l2_subdev *sd)
> {
> #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> 	struct media_link *link;
> 
> 	...
> 
> 	return IS_ERR(link) ? PTR_ERR(link) : 0;
> #else
> 	return 0;
> #endif
> }
> 

Me, too.
Laurent Pinchart April 24, 2024, 6:46 p.m. UTC | #4
On Wed, Apr 24, 2024 at 06:17:31PM +0000, Sakari Ailus wrote:
> On Wed, Apr 24, 2024 at 12:55:20PM +0200, Hans Verkuil wrote:
> > On 19/04/2024 11:47, Ricardo Ribalda wrote:
> > > Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
> > > is not enabled.
> > > 
> > > This makes cocci happier:
> > > 
> > > drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319
> > > 
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-async.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > index 4bb073587817..915a9f3ea93c 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -316,9 +316,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
> > >  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > >  					     struct v4l2_subdev *sd)
> > >  {
> > > -	struct media_link *link = NULL;
> > > +	struct media_link *link;
> > >  
> > > -#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> > > +	if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
> > > +		return 0;
> > >  
> > >  	if (sd->entity.function != MEDIA_ENT_F_LENS &&
> > >  	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > > @@ -326,8 +327,6 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > >  
> > >  	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > >  
> > > -#endif
> > > -
> > >  	return IS_ERR(link) ? PTR_ERR(link) : 0;
> > >  }
> > 
> > I think I would prefer:
> > 
> > static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > 					     struct v4l2_subdev *sd)
> > {
> > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> > 	struct media_link *link;
> > 
> > 	...
> > 
> > 	return IS_ERR(link) ? PTR_ERR(link) : 0;
> > #else
> > 	return 0;
> > #endif
> > }
> > 
> 
> Me, too.

I actually prefer Ricardo's proposal :-)
Ricardo Ribalda April 29, 2024, 10:51 a.m. UTC | #5
Hi Hans

Your proposal is what I sent for v1:
https://lore.kernel.org/linux-media/20240415-fix-cocci-v1-9-477afb23728b@chromium.org/

I have no strong opinion for any of the two, please feel free to land
whatever version you prefer.


Regards

On Wed, 24 Apr 2024 at 20:46, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Apr 24, 2024 at 06:17:31PM +0000, Sakari Ailus wrote:
> > On Wed, Apr 24, 2024 at 12:55:20PM +0200, Hans Verkuil wrote:
> > > On 19/04/2024 11:47, Ricardo Ribalda wrote:
> > > > Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
> > > > is not enabled.
> > > >
> > > > This makes cocci happier:
> > > >
> > > > drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319
> > > >
> > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-async.c | 7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > > > index 4bb073587817..915a9f3ea93c 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > @@ -316,9 +316,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
> > > >  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > > >                                        struct v4l2_subdev *sd)
> > > >  {
> > > > - struct media_link *link = NULL;
> > > > + struct media_link *link;
> > > >
> > > > -#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> > > > + if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
> > > > +         return 0;
> > > >
> > > >   if (sd->entity.function != MEDIA_ENT_F_LENS &&
> > > >       sd->entity.function != MEDIA_ENT_F_FLASH)
> > > > @@ -326,8 +327,6 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > > >
> > > >   link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> > > >
> > > > -#endif
> > > > -
> > > >   return IS_ERR(link) ? PTR_ERR(link) : 0;
> > > >  }
> > >
> > > I think I would prefer:
> > >
> > > static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
> > >                                          struct v4l2_subdev *sd)
> > > {
> > > #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> > >     struct media_link *link;
> > >
> > >     ...
> > >
> > >     return IS_ERR(link) ? PTR_ERR(link) : 0;
> > > #else
> > >     return 0;
> > > #endif
> > > }
> > >
> >
> > Me, too.
>
> I actually prefer Ricardo's proposal :-)
>
> --
> Regards,
>
> Laurent Pinchart
Hans Verkuil May 4, 2024, 8:25 a.m. UTC | #6
On 29/04/2024 12:51, Ricardo Ribalda wrote:
> Hi Hans
> 
> Your proposal is what I sent for v1:
> https://lore.kernel.org/linux-media/20240415-fix-cocci-v1-9-477afb23728b@chromium.org/

I decided to go for the v1. I prefer it, and more importantly, Sakari as
maintainer of this code prefers it as well.

Regards,

	Hans

> 
> I have no strong opinion for any of the two, please feel free to land
> whatever version you prefer.
> 
> 
> Regards
> 
> On Wed, 24 Apr 2024 at 20:46, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> On Wed, Apr 24, 2024 at 06:17:31PM +0000, Sakari Ailus wrote:
>>> On Wed, Apr 24, 2024 at 12:55:20PM +0200, Hans Verkuil wrote:
>>>> On 19/04/2024 11:47, Ricardo Ribalda wrote:
>>>>> Return 0 without checking IS_ERR or PTR_ERR if CONFIG_MEDIA_CONTROLLER
>>>>> is not enabled.
>>>>>
>>>>> This makes cocci happier:
>>>>>
>>>>> drivers/media/v4l2-core/v4l2-async.c:331:23-30: ERROR: PTR_ERR applied after initialization to constant on line 319
>>>>>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>> ---
>>>>>  drivers/media/v4l2-core/v4l2-async.c | 7 +++----
>>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>>>> index 4bb073587817..915a9f3ea93c 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>>>> @@ -316,9 +316,10 @@ v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
>>>>>  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>>>>>                                        struct v4l2_subdev *sd)
>>>>>  {
>>>>> - struct media_link *link = NULL;
>>>>> + struct media_link *link;
>>>>>
>>>>> -#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>>>> + if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
>>>>> +         return 0;
>>>>>
>>>>>   if (sd->entity.function != MEDIA_ENT_F_LENS &&
>>>>>       sd->entity.function != MEDIA_ENT_F_FLASH)
>>>>> @@ -326,8 +327,6 @@ static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>>>>>
>>>>>   link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
>>>>>
>>>>> -#endif
>>>>> -
>>>>>   return IS_ERR(link) ? PTR_ERR(link) : 0;
>>>>>  }
>>>>
>>>> I think I would prefer:
>>>>
>>>> static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
>>>>                                          struct v4l2_subdev *sd)
>>>> {
>>>> #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>>>     struct media_link *link;
>>>>
>>>>     ...
>>>>
>>>>     return IS_ERR(link) ? PTR_ERR(link) : 0;
>>>> #else
>>>>     return 0;
>>>> #endif
>>>> }
>>>>
>>>
>>> Me, too.
>>
>> I actually prefer Ricardo's proposal :-)
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 4bb073587817..915a9f3ea93c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -316,9 +316,10 @@  v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
 static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
 					     struct v4l2_subdev *sd)
 {
-	struct media_link *link = NULL;
+	struct media_link *link;
 
-#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
+	if (!IS_ENABLED(CONFIG_MEDIA_CONTROLLER))
+		return 0;
 
 	if (sd->entity.function != MEDIA_ENT_F_LENS &&
 	    sd->entity.function != MEDIA_ENT_F_FLASH)
@@ -326,8 +327,6 @@  static int v4l2_async_create_ancillary_links(struct v4l2_async_notifier *n,
 
 	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
 
-#endif
-
 	return IS_ERR(link) ? PTR_ERR(link) : 0;
 }