linux-kernel - Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS cut-down versions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d4daa87f-3556-4b46-a0e0-d90f9d14a097@ideasonboard.com>
Date: Sat, 3 May 2025 11:44:12 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Devarsh Thakkar <devarsht@...com>
Cc: praneeth@...com, vigneshr@...com, aradhya.bhatia@...ux.dev,
 s-jain1@...com, r-donadkar@...com, j-choudhary@...com, h-shenoy@...com,
 jyri.sarha@....fi, airlied@...il.com, maarten.lankhorst@...ux.intel.com,
 mripard@...nel.org, tzimmermann@...e.de, dri-devel@...ts.freedesktop.org,
 simona@...ll.ch, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Subject: Re: [PATCH v5 2/3] drm/tidss: Update infrastructure to support K3 DSS
 cut-down versions

On 02/05/2025 14:52, Devarsh Thakkar wrote:
> Hi,
> 
> <snip>
>>> It uses hw_id i.e. 1 for all vid irqstatus related registers since it is
>>> accessing am65x common region register space which has vid on idx0 which
>>> we want to skip for am62l.
>>>
>>> For dispc_plane_enable(), the caller uses
>>>> 0, for dispc_k3_vid_write_irqstatus(), the caller uses 1...
>>>
>>> Yes above is correct, and I think that's how it is supposed to be.
>>
>> No it's not. Both functions have "hw_plane" parameter, yet they require
>> a different value to be used even when referring to the same plane.
>>
>>>> With a quick look at the code, changing the callers to pass the "old
>>>> style" hw_plane as the parameter to those irq functions, and the
>>>> functions internally get the hw_id, would solve most of the problems.
>>>
>>> I don't follow above, hw_plane has 0 so it should not be used for
>>> programming irq related functions which expect idx 1 as explained above.
>>
>> We have various functions in tidss_dispc.c that have hw_plane as a
>> parameter. But the caller is supposed to know that for some functions
>> hw_plane is a plane index from 0, and for some it's the hw_id from
>> vid_info[].
>>
>>> There's still dispc_k3_set_irqenable() which manages 'main_disable' and
>>>> needs the hw_id, but maybe that's fine, even if a bit confusing.
>>>>
>>>
>>> I still feel there is no inherent bug here, but let me know if you want
>>> me to put some debug prints or get register dump so that we can double
>>> confirm.
>>
>> I'm not saying there's a bug. I'm saying it's bad code and will cause
>> confusion and bugs in the future.
>>
> 
> Ok I see what you mean to say.....although functionally it is working
> fine but from readability point of view it is confusing since both
> functions use same argument name i.e hw_plane in two different contexts.
> In that case, I would propose to use hw_id as arg name for all
> dispc_k3_vid* functions, will that be okay ?

I'd prefer to have all the dispc functions take the same kind of index.

  Tomi