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  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, 15 Jan 2021 04:36:54 -0600
From:   Alex Elder <elder@...aro.org>
To:     Saeed Mahameed <saeed@...nel.org>, davem@...emloft.net,
        kuba@...nel.org
Cc:     evgreen@...omium.org, bjorn.andersson@...aro.org,
        cpratapa@...eaurora.org, subashab@...eaurora.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

On 1/14/21 5:22 PM, Saeed Mahameed wrote:
> On Wed, 2021-01-13 at 11:15 -0600, Alex Elder wrote:
>> This series implements some updates for the GSI interrupt code,
>> buliding on some bug fixes implemented last month.
>>
>> The first two are simple changes made to improve readability and
>> consistency.  The third replaces all msleep() calls with comparable
>> usleep_range() calls.
>>
>> The remainder make some more substantive changes to make the code
>> align with recommendations from Qualcomm.  The fourth implements a
>> much shorter timeout for completion GSI commands, and the fifth
>> implements a longer delay between retries of the STOP channel
>> command.  Finally, the last implements retries for stopping TX
>> channels (in addition to RX channels).
>>
>> 					-Alex
>>
> 
> A minor thing that bothers me about this series is that it looks like
> it is based on magic numbers and some redefined constant values
> according to some mysterious sources ;-) .. It would be nice to have
> some wording in the commit messages explaining reasoning and maybe
> "semi-official" sources behind the changes.

I understand this, and agree with the sentiment.

This code is ultimately derived from code published on
codeaurora.org (CAF), but it is now quite different from
what you'll find there.

While investigating some issues recently I discovered that
the details on the retry logic and timeouts, etc. no longer
matched what I saw on CAF.  I inquired and got some updated
information, and implemented in this series what I learned.

To be honest I don't know precisely where these details
are defined.  Even if I did, it wouldn't help much,
because it would be found in an internal hardware
specification of some kind, and I have no ability to
make that public.  Still, I agree, it would be nice
to have a reference.

I would absolutely have mentioned where these magic
values came from if I could (or if I knew). As you
you noticed, the commit messages were intentionally
vague on it.

Thank you very much for the review.

					-Alex

> LGMT code style wise :)
> 
> Reviewed-by: Saeed Mahameed <saeedm@...dia.com>
> 
> 
>> Alex Elder (6):
>>    net: ipa: a few simple renames
>>    net: ipa: introduce some interrupt helpers
>>    net: ipa: use usleep_range()
>>    net: ipa: change GSI command timeout
>>    net: ipa: change stop channel retry delay
>>    net: ipa: retry TX channel stop commands
>>
>>   drivers/net/ipa/gsi.c          | 140 +++++++++++++++++++----------
>> ----
>>   drivers/net/ipa/ipa_endpoint.c |   4 +-
>>   2 files changed, 83 insertions(+), 61 deletions(-)
>>
> 

Powered by blists - more mailing lists