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 17:43, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Dec 01, 2025 at 04:35:23PM +0100, Hans Verkuil wrote:
>> 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.
> 
> It is fragile, yes. I can't point to a place where this happens but the
> driver relies on every mbus code ever used to be on that table, and this
> not holding will result in NULL dereference. In other words, it's not easy
> at all to figure out it won't happen.
> 

I think I prefer to stick to my original patch, keeping the sentinel.

I don't want to spend too much time on this, it was just a bunch of patches
that I accumulated while working on the vb2 wait_prepare/finish callback
removal. There are a lot more issues that v4l2-compliance found that I
never spend time on.

I'll put the sentinel back and post a new v2 series later this week.

Regards,

	Hans