| Message ID | 20240419-fix-cocci-v2-9-2119e692309c@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: Fix coccinelle warning/errors | expand |
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
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.
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 :-)
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
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 --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; }
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(-)