linux-kernel - Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates

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: <012a939e-2198-439a-6339-c69adbaaadd6@linaro.org>
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(-)
>>
>