Hi Sergey, I recommend that you wait a bit until these two patches are merged: https://patchwork.linuxtv.org/patch/61897/ https://patchwork.linuxtv.org/patch/61898/ I'm about to post a PR for these (and others), so hopefully these will get merged soon. Regards, Hans On 16/04/2020 14:13, Hans Verkuil wrote: > On 16/04/2020 13:32, Sergey Senozhatsky wrote: >> On (20/04/16 10:53), Hans Verkuil wrote: >> [..] >>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c >>>> @@ -2869,6 +2869,9 @@ EXPORT_SYMBOL(v4l2_ctrl_add_handler); >>>> >>>> bool v4l2_ctrl_radio_filter(const struct v4l2_ctrl *ctrl) >>>> { >>>> + if (WARN_ON(!ctrl)) >>>> + return false; >>>> + >>>> if (V4L2_CTRL_ID2WHICH(ctrl->id) == V4L2_CTRL_CLASS_FM_TX) >>>> return true; >>>> if (V4L2_CTRL_ID2WHICH(ctrl->id) == V4L2_CTRL_CLASS_FM_RX) >>>> @@ -3794,7 +3797,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl) >>>> struct v4l2_ext_control c; >>>> >>>> /* It's a driver bug if this happens. */ >>>> - WARN_ON(!ctrl->is_int); >>>> + if (WARN_ON(!ctrl || !ctrl->is_int)) >>>> + return -EINVAL; >>> >>> Just return 0 here. The return value is the control's value, not an error code. >>> So all you can do here is return 0 in the absence of anything better. >> >> OK. >> >>>> + >>>> c.value = 0; >>>> get_ctrl(ctrl, &c); >>>> return c.value; >>>> @@ -4212,6 +4217,9 @@ EXPORT_SYMBOL(v4l2_s_ctrl); >>>> >>>> int __v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val) >>>> { >>>> + if (!ctrl) >>> >>> Change this to 'if (WARN_ON(!ctrl))' >>> >>> I don't think NULL pointers should be silently ignored: it really >>> indicates a driver bug. It it certainly a good idea to WARN instead. >> >> Should WARN_ON() be only in unlocked versions of ctrl API? It probably >> would make sense to add WARNs to both - e.g. to v4l2_ctrl_s_ctrl() and > > Yes, it should be done for both. > >> to __v4l2_ctrl_s_ctrl(). By the way, why don't locked and unlocked >> versions live together in v4l2-ctrls.c file? Any reason for, e.g., >> v4l2_ctrl_s_ctrl() to be in header and __v4l2_ctrl_s_ctrl() to be C-file? > > The v4l2_ctrl_s_ctrl() work fine as a static inline (only compiled if > they are actually used). But with an additional 'if (WARN_ON(!ctrl))' > it becomes a bit questionable. I would not be opposed if these static > inlines are now moved into the source code. > > Regards, > > Hans > >> >>> The same is true for the functions below. >> >> OK. >> >> -ss >> >
