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:

quoted

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@nvidia.com>

quoted

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(-)