Re: [PATCH 03/14] media: omap3isp: isp_video_mbus_to_pix/pix_to_mbus fixes

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 01/12/2025 13:56, Sakari Ailus wrote:
> On Mon, Dec 01, 2025 at 12:22:12PM +0200, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Mon, Dec 01, 2025 at 09:27:55AM +0100, Hans Verkuil wrote:
>>> Hi Sakari,
>>>
>>> On 22/10/2025 09:48, Sakari Ailus wrote:
>>>> Hi Hans,
>>>>
>>>> Thanks for the set.
>>>>
>>>> On Fri, Oct 17, 2025 at 03:26:40PM +0200, Hans Verkuil wrote:
>>>>> The isp_video_mbus_to_pix/pix_to_mbus functions did not take
>>>>> the last empty entry { 0, } of the formats array into account.
>>>>>
>>>>> As a result, isp_video_mbus_to_pix would accept code 0 and
>>>>> isp_video_pix_to_mbus would select code 0 if no match was found.
>>>>>
>>>>> Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
>>>>> ---
>>>>>  drivers/media/platform/ti/omap3isp/ispvideo.c | 6 +++---
>>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/ti/omap3isp/ispvideo.c b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>> index 46609045e2c8..864d38140b87 100644
>>>>> --- a/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>> +++ b/drivers/media/platform/ti/omap3isp/ispvideo.c
>>>>> @@ -148,12 +148,12 @@ static unsigned int isp_video_mbus_to_pix(const struct isp_video *video,
>>>>>  	pix->width = mbus->width;
>>>>>  	pix->height = mbus->height;
>>>>>  
>>>>> -	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
>>>>> +	for (i = 0; i < ARRAY_SIZE(formats) - 1; ++i) {
>>>>
>>>> As it seems all users of the formats array depend on the size of the array
>>>> and not its contents, could we remove the sentinel entry from the array
>>>> instead?
>>>
>>> Probably, but see this comment just before the sentinel in the array:
>>>
>>>         /* Empty entry to catch the unsupported pixel code (0) used by the CCDC
>>>          * module and avoid NULL pointer dereferences.
>>>          */
>>>         { 0, }
>>>
>>> Now, I wonder if this comment is out of date, since I don't see code 0 being used
>>> by CDDC. If you can confirm that that's indeed the case, then I can drop the sentinel.
>>
>> Yes, please!
> 
> Actually it's omap3isp_video_format_info() I understand ispccdc.c relies
> not to return NULL. I might add a separate variable for that, to get rid of
> this obscure arrangement.

So ispccdc.c can call omap3isp_video_format_info with a code value of 0?
Can you give an example where that happens? If true, then this feels very
fragile.

Regards,

	Hans

> 
> You could also add:
> 
> Fixes: c51364cafa26 ("[media] omap3isp: ccdc: Add YUV input formats support")
>