| Message ID | 20210201232609.3524451-4-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: ipa: a few bug fixes | expand |
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 > > 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. > 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? Best regards, Amy Parker (she/her/hers)
On Mon, Feb 1, 2021 at 4:02 PM Amy Parker <enbyamy@gmail.com> wrote: > > Fix this by using u8_get_bits() to get the destination endpoint ID. > > Isn't > Apologies about this - premature email sending. This was simply going to be "Isn't endpoint_id u32?", which was addressed later anyways. Best regards, Amy Parker (she/her/hers)
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. 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. >> 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 > Best regards, > Amy Parker > (she/her/hers) >
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)
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; 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;
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. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa_endpoint.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.27.0