linux-kernel - Re: [PATCH v3 12/25] bus: mhi: ep: Add support for ring management

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]

Date:   Fri, 18 Feb 2022 09:47:44 -0600
From:   Alex Elder <elder@...aro.org>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc:     mhi@...ts.linux.dev, quic_hemantk@...cinc.com,
        quic_bbhatt@...cinc.com, quic_jhugo@...cinc.com,
        vinod.koul@...aro.org, bjorn.andersson@...aro.org,
        dmitry.baryshkov@...aro.org, quic_vbadigan@...cinc.com,
        quic_cang@...cinc.com, quic_skananth@...cinc.com,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 12/25] bus: mhi: ep: Add support for ring management

On 2/18/22 9:23 AM, Manivannan Sadhasivam wrote:
>>>
>>> I'm pretty sure I mentioned this before...  I don't really like these
>>> "DWORD" macros that simply write compute register values to write
>>> out to the TREs.  A TRE is a structure, not a set of registers.  And
>>> a whole TRE can be written or read in a single ARM instruction in
>>> some cases--but most likely you need to define it as a structure
>>> for that to happen.
>>>
>>> struct mhi_tre {
>>> 	__le64 addr;
>>> 	__le16 len_opcode
>>> 	__le16 reserved;
>>> 	__le32 flags;
>>> };
>> Changing the TRE structure requires changes to both host and endpoint
>> stack. So I'll tackle this as an improvement later.
>>
>> Added to TODO list.
> Just did a comparision w/ IPA code and I convinced myself that this conversion
> should happen now itself. So please ignore my above comment.

This might not be that much work, but if it is, I somewhat
apologize for that.  Still, I believe the code will be better
as a result, so I'm not *that* sorry.

If you do this though, I would recommend you do it as a
separate, prerequisite bit of work.  Your series is too
long, and making it longer by adding this will just delay
*everything* a bit more.  So, I'd advise updating the
existing host code this way first, then adapt your patch
series to do things the new way.

Alternatively, do this later (as you earlier said you would),
and don't delay this series any more.  If it works, it works,
and you can always improve it in the future.

And now that your series is getting closer to golden, maybe
you can break it into a few smaller series?  I don't know,
that also can lead to some confusion, so I won't strongly
advocate that.  But it's something to consider for future
work regardless.

					-Alex