On 07/26/2015 12:00 PM, Guennadi Liakhovetski wrote: > Hi Hans, > > On Fri, 24 Jul 2015, Hans Verkuil wrote: > >> Guennadi, >> >> I want to make a pull request for this patch series. This patch is the only >> outstanding one. > > Right, sorry for the delay. Replying to your explanation below: > >> Or do you have to review more patches? I only got Acks for patches 1 and >> 2. >> >> Regards, >> >> Hans >> >> On 06/22/2015 09:29 AM, Hans Verkuil wrote: >>> On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote: >>>> Hi Hans, >>>> >>>> On Mon, 22 Jun 2015, Hans Verkuil wrote: >>>> >>>>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote: >>>>>> On Mon, 15 Jun 2015, Hans Verkuil wrote: >>>>>> >>>>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx> >>>>>>> >>>>>>> When the standard changes the VACTIVE and VDELAY values need to be updated. >>>>>>> >>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> >>>>>>> --- >>>>>>> drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++- >>>>>>> 1 file changed, 28 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c >>>>>>> index df66417..e939c24 100644 >>>>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c >>>>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c >>>>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client) >>>>>>> "tw9910 Product ID %0x:%0x\n", id, priv->revision); >>>>>>> >>>>>>> priv->norm = V4L2_STD_NTSC; >>>>>>> + priv->scale = &tw9910_ntsc_scales[0]; >>>>>> >>>>>> Why do you need this? So far everywhere in the code priv->scale is either >>>>>> checked or set before use. Don't see why an additional initialisation is >>>>>> needed. >>>>> >>>>> If you just start streaming without explicitly setting up formats (which is >>>>> allowed), then priv->scale is still NULL. >>>> >>>> Yes, it can well be NULL, but it is also unused. Before it is used it will >>>> be set, while it is unused it is allowed to stay NULL. >>> >>> No. If you start streaming without the set_fmt op having been called, then >>> s_stream will return an error since priv->scale is NULL. This is wrong. Since >>> this driver defaults to NTSC the initial setup should be for NTSC and it should >>> be ready for streaming. >>> >>> So priv->scale *is* used: in s_stream. And it is not necessarily set before use. >>> E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this >>> case. It's how I found this bug. >>> >>> It's a trivial one liner to ensure a valid priv->scale pointer. > > Yes, you're right, now I see how this can happen. But there's also another > possibility: if S_FMT fails priv->scale will be set to NULL. If you then > directly start streaming wouldn't the same problem arise? Or is it valid > to fail STREAMON after a failed S_FMT? If it is, then of course The only way S_FMT (or really tw9910_set_frame) can fail is if there are I/O errors. And then it is OK for STREAMON to fail (you're typically in deep shit anyway if this happens). Regards, Hans > > Acked-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx> > > If that's invalid, then maybe a more extensive fix is needed? > > Thanks > Guennadi > >>> Regards, >>> >>> Hans >>> >>>> >>>> Thanks >>>> Guennadi >>>> >>>>> V4L2 always assumes that there is some initial format configured, and this line >>>>> enables that for this driver (NTSC). >>>>> >>>>> Regards, >>>>> >>>>> Hans > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
