Messages in this thread | | | From | Amy Parker <> | Date | Mon, 1 Feb 2021 16:49:33 -0800 | Subject | Re: [PATCH net 3/4] net: ipa: use the right accessor in ipa_endpoint_status_skip() |
| |
On Mon, Feb 1, 2021 at 4:15 PM Alex Elder <elder@linaro.org> wrote: > > On 2/1/21 6:02 PM, Amy Parker wrote: > > On Mon, Feb 1, 2021 at 3:32 PM Alex Elder <elder@linaro.org> wrote: > >> > >> When extracting the destination endpoint ID from the status in > >> ipa_endpoint_status_skip(), u32_get_bits() is used. This happens to > >> work, but it's wrong: the structure field is only 8 bits wide > >> instead of 32. > >> > >> Fix this by using u8_get_bits() to get the destination endpoint ID. > > > > Isn't > > (I saw your second message.) > > >> Signed-off-by: Alex Elder <elder@linaro.org> > >> --- > >> drivers/net/ipa/ipa_endpoint.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c > >> index 448d89da1e456..612afece303f3 100644 > >> --- a/drivers/net/ipa/ipa_endpoint.c > >> +++ b/drivers/net/ipa/ipa_endpoint.c > >> @@ -1164,8 +1164,8 @@ static bool ipa_endpoint_status_skip(struct ipa_endpoint *endpoint, > >> return true; > > > > A few lines above this, endpoint_id is initialized as u32. If we're > > going for "correctness", endpoint_id should be a u8. But of course, > > this would contrast with ipa_endpoint having it as a u32. > > You are correct, endpoint_id is *defined* as type u32. > > But the issue here is that the field status->endp_dst_idx > has type u8. u32_get_bits() assumes the field it is > passed has type u32; while u8_get_bits() takes a u8.
Ah, missed that bit. Thanks for clarifying.
> > The return value of u8_get_bits() is u8, as you might > suspect. The C standard guarantees that the u8 value > will be promoted to the u32 target type here.
Yes, it does, and so it wouldn't be a theoretical issue - just an issue of developer confusion at first when working with it. But the above outlined point of the types taken is more important.
> > >> if (!status->pkt_len) > >> return true; > >> - endpoint_id = u32_get_bits(status->endp_dst_idx, > >> - IPA_STATUS_DST_IDX_FMASK); > >> + endpoint_id = u8_get_bits(status->endp_dst_idx, > >> + IPA_STATUS_DST_IDX_FMASK); > >> if (endpoint_id != endpoint->endpoint_id) > >> return true; > >> > >> -- > >> 2.27.0 > >> > > > > As far as I see it, using u32_get_bits instead of u8_get_bits simply > > eliminates confusion about the type of endpoint_id. Perhaps instead of > > this patch, send a patch with a comment that while u32_get_bits is > > used, the field is only 8 bits? > > No. We really want to extract a sub-field from the u8 > value passed to u8_get_bits() (not u32_get_bits()). > > Does that make sense? > > -Alex
Yes, it does. Thank you.
Best regards, Amy Parker (she/her/hers)
| |