| Message ID | 20210113171532.19248-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: ipa: GSI interrupt updates | expand |
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. 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(-) >
On Thu, 14 Jan 2021 15:22:54 -0800 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). > > 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. > > LGMT code style wise :) > > Reviewed-by: Saeed Mahameed <saeedm@nvidia.com> Dropped the fixes tags (since its not a series of fixes) and applied. Thanks!
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(-) >> >
On 1/14/21 8:08 PM, Jakub Kicinski wrote:
> Dropped the fixes tags (since its not a series of fixes) and applied.
Thanks for applying these.
Do you only want "Fixes" tags on patches posted for the net
branch (and not net-next)?
I think I might have debated sending these as bug fixes but
decided they weren't serious enough to warrant that. Anyway
if I know it's important to *not* use that tag I can avoid
including it.
Thanks.
-Alex
On Fri, 15 Jan 2021 04:42:14 -0600 Alex Elder wrote: > On 1/14/21 8:08 PM, Jakub Kicinski wrote: > > Dropped the fixes tags (since its not a series of fixes) and applied. > > Thanks for applying these. > > Do you only want "Fixes" tags on patches posted for the net > branch (and not net-next)? > > I think I might have debated sending these as bug fixes but > decided they weren't serious enough to warrant that. Anyway > if I know it's important to *not* use that tag I can avoid > including it. The point of the tag is to facilitate backporting. If something is important enough to back port is should also be important enough to go to net, no?
On 1/15/21 12:59 PM, Jakub Kicinski wrote: > On Fri, 15 Jan 2021 04:42:14 -0600 Alex Elder wrote: >> On 1/14/21 8:08 PM, Jakub Kicinski wrote: >>> Dropped the fixes tags (since its not a series of fixes) and applied. >> >> Thanks for applying these. >> >> Do you only want "Fixes" tags on patches posted for the net >> branch (and not net-next)? >> >> I think I might have debated sending these as bug fixes but >> decided they weren't serious enough to warrant that. Anyway >> if I know it's important to *not* use that tag I can avoid >> including it. > > The point of the tag is to facilitate backporting. If something is > important enough to back port is should also be important enough > to go to net, no? Yes, agreed and understood. -Alex