linux-kernel - Re: [PATCH net-next v2 7/8] net: ipa: fix two minor ipa

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 for Android: free password hash cracker in your pocket

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

Date: Tue, 23 Apr 2024 07:36:12 -0500
From: Alex Elder <elder@...aro.org>
To: Paolo Abeni <pabeni@...hat.com>, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org
Cc: mka@...omium.org, andersson@...nel.org, quic_cpratapa@...cinc.com,
 quic_avuyyuru@...cinc.com, quic_jponduru@...cinc.com,
 quic_subashab@...cinc.com, elder@...nel.org, netdev@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 7/8] net: ipa: fix two minor ipa_cmd problems

On 4/23/24 6:21 AM, Paolo Abeni wrote:
> On Fri, 2024-04-19 at 10:17 -0500, Alex Elder wrote:
>> In "ipa_cmd.h", ipa_cmd_data_valid() is declared, but that function
>> does not exist.  So delete that declaration.
>>
>> Also, for some reason ipa_cmd_init() never gets called.  It isn't
>> really critical--it just validates that some memory offsets and a
>> size can be represented in some register fields, and they won't fail
>> with current data.  Regardless, call the function in ipa_probe().
> 
> That name sounds confusing to me: I expect *init to allocate/set
> something that will need some reverse operation at shutdown/removal.
> What about a possible follow-up renaming the function to
> ipa_cmd_validate() or the like?

In the IPA driver I have several phases of initialization that
occur:
- *_init() is done to initialize anything (like allocating memory
   and looking up DT information) that does not require any access
   to hardware.  Its inverse is *_exit().
- *_config() is done once "primitive" (register-based) access to
   the hardware is needed, where the hardware must be clocked.  Its
   inverse is *_deconfig().
- *_setup() is done after the above, at a point where a higher-level
   command-based (submit/await completion) interface is available.
   That is used for the last steps of setting up the hardware.  Its
   inverse is *_teardown().

You're right, that in this case all this init function does is
validate things.  But at an abstract level, this is the place
in the "IPA command" module where *any* early-stage initialization
takes place.  The caller doesn't "know" that at the moment this
happens to only be validation.  (I don't recall, but this might
previously have done some other things.)

So that's the reasoning behind the name.  Changing it to
ipa_cmd_validate() makes sense too, but wouldn't fit the
pattern used elsewhere.  I'm open to it though; it's just a
design choice.  But unless you're convinced such a change
would really improve the code, I plan to leave it as-is.

> Not blocking the series, I'm applying it.

Thank you very much.

					-Alex


> Thanks,
> 
> Paolo
>