| Message ID | 20211112071025.150431-1-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] sbefifo: fix sbefifo_register_get with multiple registers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-CI | success | Successfully ran 1 jobs. |
| snowpatch_ozlabs/github-build_and_test | fail | build failed at step Test pdbg. |
On Fri, 12 Nov 2021 at 07:10, Nicholas Piggin <npiggin@gmail.com> wrote: > > sbefifo_register_get_pull is not indexing the result buffer correctly > for > 1 returned register. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > libsbefifo/cmd_register.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libsbefifo/cmd_register.c b/libsbefifo/cmd_register.c > index 6c3f336..1e74b6e 100644 > --- a/libsbefifo/cmd_register.c > +++ b/libsbefifo/cmd_register.c > @@ -57,6 +57,7 @@ static int sbefifo_register_get_push(uint8_t core_id, uint8_t thread_id, uint8_t > static int sbefifo_register_get_pull(uint8_t *buf, uint32_t buflen, uint8_t reg_count, uint64_t **value) > { > uint32_t i; > + uint32_t *b = (uint32_t *)buf; > > if (buflen != reg_count * 8) > return EPROTO; > @@ -68,8 +69,8 @@ static int sbefifo_register_get_pull(uint8_t *buf, uint32_t buflen, uint8_t reg_ > for (i=0; i<reg_count; i++) { > uint32_t val1, val2; > > - val1 = be32toh(*(uint32_t *) &buf[i*4]); > - val2 = be32toh(*(uint32_t *) &buf[i*4+4]); > + val1 = be32toh(b[i*2]); > + val2 = be32toh(b[i*2+1]); Just to clarify, this is what the code was doing before: val1 = be32toh(b[i]); val2 = be32toh(b[i + 1]); Which would read out from the buffer: value = 0 | 1 value = 1 | 2 value = 2 | 3 And your fix is to add i*2: value = 0 | 1 value = 2 | 3 value = 4 | 5 Looks good to me! Reviewed-by: Joel Stanley <joel@jms.id.au> > > (*value)[i] = ((uint64_t)val1 << 32) | (uint64_t)val2; > } > -- > 2.23.0 > > -- > Pdbg mailing list > Pdbg@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/pdbg
On Tue, 2021-11-16 at 01:47 +0000, Joel Stanley wrote: > On Fri, 12 Nov 2021 at 07:10, Nicholas Piggin <npiggin@gmail.com> > wrote: > > > > sbefifo_register_get_pull is not indexing the result buffer > > correctly > > for > 1 returned register. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > libsbefifo/cmd_register.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/libsbefifo/cmd_register.c b/libsbefifo/cmd_register.c > > index 6c3f336..1e74b6e 100644 > > --- a/libsbefifo/cmd_register.c > > +++ b/libsbefifo/cmd_register.c > > @@ -57,6 +57,7 @@ static int sbefifo_register_get_push(uint8_t > > core_id, uint8_t thread_id, uint8_t > > static int sbefifo_register_get_pull(uint8_t *buf, uint32_t > > buflen, uint8_t reg_count, uint64_t **value) > > { > > uint32_t i; > > + uint32_t *b = (uint32_t *)buf; > > > > if (buflen != reg_count * 8) > > return EPROTO; > > @@ -68,8 +69,8 @@ static int sbefifo_register_get_pull(uint8_t > > *buf, uint32_t buflen, uint8_t reg_ > > for (i=0; i<reg_count; i++) { > > uint32_t val1, val2; > > > > - val1 = be32toh(*(uint32_t *) &buf[i*4]); > > - val2 = be32toh(*(uint32_t *) &buf[i*4+4]); > > + val1 = be32toh(b[i*2]); > > + val2 = be32toh(b[i*2+1]); > > Just to clarify, this is what the code was doing before: > > val1 = be32toh(b[i]); > val2 = be32toh(b[i + 1]); > > Which would read out from the buffer: > > value = 0 | 1 > value = 1 | 2 > value = 2 | 3 > > And your fix is to add i*2: > > value = 0 | 1 > value = 2 | 3 > value = 4 | 5 > > Looks good to me! > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > > > > > (*value)[i] = ((uint64_t)val1 << 32) | > > (uint64_t)val2; > > } > > -- > > 2.23.0 > > > > -- > > Pdbg mailing list > > Pdbg@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/pdbg Good catch. Pushed upstream. Amitay.
diff --git a/libsbefifo/cmd_register.c b/libsbefifo/cmd_register.c index 6c3f336..1e74b6e 100644 --- a/libsbefifo/cmd_register.c +++ b/libsbefifo/cmd_register.c @@ -57,6 +57,7 @@ static int sbefifo_register_get_push(uint8_t core_id, uint8_t thread_id, uint8_t static int sbefifo_register_get_pull(uint8_t *buf, uint32_t buflen, uint8_t reg_count, uint64_t **value) { uint32_t i; + uint32_t *b = (uint32_t *)buf; if (buflen != reg_count * 8) return EPROTO; @@ -68,8 +69,8 @@ static int sbefifo_register_get_pull(uint8_t *buf, uint32_t buflen, uint8_t reg_ for (i=0; i<reg_count; i++) { uint32_t val1, val2; - val1 = be32toh(*(uint32_t *) &buf[i*4]); - val2 = be32toh(*(uint32_t *) &buf[i*4+4]); + val1 = be32toh(b[i*2]); + val2 = be32toh(b[i*2+1]); (*value)[i] = ((uint64_t)val1 << 32) | (uint64_t)val2; }
sbefifo_register_get_pull is not indexing the result buffer correctly for > 1 returned register. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- libsbefifo/cmd_register.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)