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: <ead31912-d1e5-4813-99a7-5cd2754672ef@ti.com>
Date: Fri, 2 May 2025 17:22:23 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.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

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 ?

Regards
Devarsh