Messages in this thread | | | Subject | Re: [PATCH net-next 0/6] net: ipa: GSI interrupt updates | From | Alex Elder <> | Date | Fri, 15 Jan 2021 04:36:54 -0600 |
| |
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@nvidia.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(-) >> >
| |