mbox series

[v2,net,0/5] net: ipa: minor bug fixes

Message ID 20201028194148.6659-1-elder@linaro.org
Headers show
Series net: ipa: minor bug fixes | expand

Message

Alex Elder Oct. 28, 2020, 7:41 p.m. UTC
This series fixes several bugs.  They are minor, in that the code
currently works on supported platforms even without these patches
applied, but they're bugs nevertheless and should be fixed.

Version 2 improves the commit message for the fourth patch.  It also
fixes a bug in two spots in the last patch.  Both of these changes
were suggested by Willem de Bruijn.

					-Alex

Alex Elder (5):
  net: ipa: assign proper packet context base
  net: ipa: fix resource group field mask definition
  net: ipa: assign endpoint to a resource group
  net: ipa: distinguish between resource group types
  net: ipa: avoid going past end of resource group array

 drivers/net/ipa/ipa_data-sc7180.c |   4 +
 drivers/net/ipa/ipa_data-sdm845.c |   4 +
 drivers/net/ipa/ipa_data.h        |  12 +--
 drivers/net/ipa/ipa_endpoint.c    |  11 +++
 drivers/net/ipa/ipa_main.c        | 121 +++++++++++++++---------------
 drivers/net/ipa/ipa_mem.c         |   2 +-
 drivers/net/ipa/ipa_reg.h         |  48 +++++++++++-
 7 files changed, 136 insertions(+), 66 deletions(-)

Comments

Jakub Kicinski Oct. 29, 2020, 4:11 p.m. UTC | #1
On Wed, 28 Oct 2020 14:41:43 -0500 Alex Elder wrote:
> This series fixes several bugs.  They are minor, in that the code
> currently works on supported platforms even without these patches
> applied, but they're bugs nevertheless and should be fixed.

By which you mean "it seems to work just fine most of the time" or "the
current code does not exercise this paths/functionally these bugs don't
matter for current platforms".
Alex Elder Oct. 29, 2020, 4:50 p.m. UTC | #2
On 10/29/20 11:11 AM, Jakub Kicinski wrote:
> On Wed, 28 Oct 2020 14:41:43 -0500 Alex Elder wrote:
>> This series fixes several bugs.  They are minor, in that the code
>> currently works on supported platforms even without these patches
>> applied, but they're bugs nevertheless and should be fixed.
> 
> By which you mean "it seems to work just fine most of the time" or "the
> current code does not exercise this paths/functionally these bugs don't
> matter for current platforms".

The latter, although for patch 3 I'm not 100% sure.

Case by case:
Patch 1:
   It works.  I inquired what the consequence of passing this
   wrong buffer pointer was, and for the way we are using IPA
   it seems it's fine--the memory pointer we were assigning is
   not used, so it's OK.  But we're assigning the wrong pointer.
Patch 2:
   It works.  Even though the bit field is 1 bit wide (not two)
   we never actually write a value greater than 1, so we don't
   cause a problem.  But the definition is incorrect.
Patch 3:
   It works, but on the SDM845 we should be assigning the endpoints
   to use resource group 1 (they are 0 by default).  The way we
   currently use this upstream we don't have other endpoints
   competing for resources, so I think this is fine.  SC7180 we
   will assign endpoints to resource group 0, which is the default.
Patch 4:
   It works.  This is like patch 2; we define the number of these
   things incorrectly, but the way we currently use them we never
   exceed the limit in a broken way.
Patch 5:
   It works.  The maximum number of supported groups is even,
   and if a (smaller) odd number are used the remainder are
   programmed with 0, which is appropriate for undefined
   fields.

If you have any concerns about back-porting these fixes I
think I'm comfortable posting them for net-next instead.
I debated that before sending them out.  Please request that
if it's what you think would be best.

					-Alex
Jakub Kicinski Oct. 31, 2020, 12:23 a.m. UTC | #3
On Thu, 29 Oct 2020 11:50:52 -0500 Alex Elder wrote:
> On 10/29/20 11:11 AM, Jakub Kicinski wrote:

> > On Wed, 28 Oct 2020 14:41:43 -0500 Alex Elder wrote:  

> >> This series fixes several bugs.  They are minor, in that the code

> >> currently works on supported platforms even without these patches

> >> applied, but they're bugs nevertheless and should be fixed.  

> > 

> > By which you mean "it seems to work just fine most of the time" or "the

> > current code does not exercise this paths/functionally these bugs don't

> > matter for current platforms".  

> 

> The latter, although for patch 3 I'm not 100% sure.

> 

> Case by case:

> Patch 1:

>    It works.  I inquired what the consequence of passing this

>    wrong buffer pointer was, and for the way we are using IPA

>    it seems it's fine--the memory pointer we were assigning is

>    not used, so it's OK.  But we're assigning the wrong pointer.

> Patch 2:

>    It works.  Even though the bit field is 1 bit wide (not two)

>    we never actually write a value greater than 1, so we don't

>    cause a problem.  But the definition is incorrect.

> Patch 3:

>    It works, but on the SDM845 we should be assigning the endpoints

>    to use resource group 1 (they are 0 by default).  The way we

>    currently use this upstream we don't have other endpoints

>    competing for resources, so I think this is fine.  SC7180 we

>    will assign endpoints to resource group 0, which is the default.

> Patch 4:

>    It works.  This is like patch 2; we define the number of these

>    things incorrectly, but the way we currently use them we never

>    exceed the limit in a broken way.

> Patch 5:

>    It works.  The maximum number of supported groups is even,

>    and if a (smaller) odd number are used the remainder are

>    programmed with 0, which is appropriate for undefined

>    fields.

> 

> If you have any concerns about back-porting these fixes I

> think I'm comfortable posting them for net-next instead.

> I debated that before sending them out.  Please request that

> if it's what you think would be best.


Looks like these patches apply cleanly to net-next, so I put them there.

Thanks!
Alex Elder Oct. 31, 2020, 12:57 p.m. UTC | #4
On 10/30/20 7:23 PM, Jakub Kicinski wrote:
> On Thu, 29 Oct 2020 11:50:52 -0500 Alex Elder wrote:

>> On 10/29/20 11:11 AM, Jakub Kicinski wrote:

>>> On Wed, 28 Oct 2020 14:41:43 -0500 Alex Elder wrote:  

>>>> This series fixes several bugs.  They are minor, in that the code

>>>> currently works on supported platforms even without these patches

>>>> applied, but they're bugs nevertheless and should be fixed.  

>>>

>>> By which you mean "it seems to work just fine most of the time" or "the

>>> current code does not exercise this paths/functionally these bugs don't

>>> matter for current platforms".  

>>

>> The latter, although for patch 3 I'm not 100% sure.

>>

>> Case by case:

>> Patch 1:

>>    It works.  I inquired what the consequence of passing this

>>    wrong buffer pointer was, and for the way we are using IPA

>>    it seems it's fine--the memory pointer we were assigning is

>>    not used, so it's OK.  But we're assigning the wrong pointer.

>> Patch 2:

>>    It works.  Even though the bit field is 1 bit wide (not two)

>>    we never actually write a value greater than 1, so we don't

>>    cause a problem.  But the definition is incorrect.

>> Patch 3:

>>    It works, but on the SDM845 we should be assigning the endpoints

>>    to use resource group 1 (they are 0 by default).  The way we

>>    currently use this upstream we don't have other endpoints

>>    competing for resources, so I think this is fine.  SC7180 we

>>    will assign endpoints to resource group 0, which is the default.

>> Patch 4:

>>    It works.  This is like patch 2; we define the number of these

>>    things incorrectly, but the way we currently use them we never

>>    exceed the limit in a broken way.

>> Patch 5:

>>    It works.  The maximum number of supported groups is even,

>>    and if a (smaller) odd number are used the remainder are

>>    programmed with 0, which is appropriate for undefined

>>    fields.

>>

>> If you have any concerns about back-porting these fixes I

>> think I'm comfortable posting them for net-next instead.

>> I debated that before sending them out.  Please request that

>> if it's what you think would be best.

> 

> Looks like these patches apply cleanly to net-next, so I put them there.

> 

> Thanks!


Works for me.  Thank you.	-Alex

>