| Message ID | 20201028194148.6659-1-elder@linaro.org |
|---|---|
| Headers | show |
| Series | net: ipa: minor bug fixes | expand |
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".
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
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!
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 >