Re: [PATCH v3 32/33] smiapp: Add driver.

Sylwester Nawrocki
2012-02-29T10:00:40Z

Hi,

On 02/29/2012 10:35 AM, Laurent Pinchart wrote:
>>>> + if (sensor->pixel_array->ctrl_handler.error) {
>>>> + dev_err(&client->dev,
>>>> + "pixel array controls initialization failed (%d)\n",
>>>> + sensor->pixel_array->ctrl_handler.error);
>>>
>>> Shouldn't you call v4l2_ctrl_handler_free() here ?
>>
>> Yes. Fixed.
>>
>>>> + return sensor->pixel_array->ctrl_handler.error;
>>>> + }
>>>> +
>>>> + sensor->pixel_array->sd.ctrl_handler =
>>>> + &sensor->pixel_array->ctrl_handler;
>>>> +
>>>> + v4l2_ctrl_cluster(2, &sensor->hflip);
>>>
>>> Shouldn't you move this before the control handler check ?
>>
>> Why? It can't fail.
>
> I thought it could fail. You could then leave it here, but it would be easier
> from a maintenance point of view to check the error code after completing all
> control-related initialization, as it would avoid introducing a bug if for
> some reason the v4l2_ctrl_cluster() function needs to return an error later.

By calling v4l2_ctrl_cluster() after the control handler check you're sure
sensor->hflip is a pointer to a valid control. In case the HFLIP control
creation fails and you try to cluster that, unpredictable things may happen.
Well, predictable, e.g. BUG_ON() in v4l2_ctrl_cluster(). :-)

So using v4l2_ctrl_cluster() before checking ctrl_handler.error would require
validating the control pointer or maybe something more.

--

Regards,
Sylwester