| Message ID | 20170711162911.39114-1-chris.brandt@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Chris, On Tue, Jul 11, 2017 at 6:29 PM, Chris Brandt <chris.brandt@renesas.com> wrote: > The existing code gives an incorrect value. > The buffer pointer 'buf' was of type unsigned short *, and 'count' was a > number in bytes, so the pointer should have been cast before doing any > pointer arithmetic. > > Since we know the code before it is doing as many 4-byte transfers as > possible, we just need a pointer to where it left off in the buffer, hence > zeroing out the bottom 2 bits of count for out math. s/out/our/ > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port") > Signed-off-by: Chris Brandt <chris.brandt@renesas.com> > --- > drivers/mmc/host/tmio_mmc_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c > index 77e7b56a9099..5dfc556ccedf 100644 > --- a/drivers/mmc/host/tmio_mmc_core.c > +++ b/drivers/mmc/host/tmio_mmc_core.c > @@ -428,7 +428,7 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host, > if (!(count & 0x3)) > return; > > - buf8 = (u8 *)(buf + (count >> 2)); > + buf8 = (u8 *)buf + (count & ~3); > count %= 4; While correct, this is IMHO still difficult to understand for the casual reader. Given the code before casts to "u32 *", and uses "count >>2", and the code after also casts to "u32 *", what about getting rid of all casts like: u32 data = 0; u32 *buf32 = buf; if (is_read) sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32, count >> 2); else sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32, count >> 2); /* if count was multiple of 4 */ if (!(count & 0x3)) return; buf32 += count >> 2; count %= 4; if (is_read) { sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1); memcpy(buf32, &data, count); } else { memcpy(&data, buf32, count); sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1); } return; } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Tuesday, July 11, 2017, Geert Uytterhoeven wrote: > > zeroing out the bottom 2 bits of count for out math. > > s/out/our/ Thank you! > > - buf8 = (u8 *)(buf + (count >> 2)); > > + buf8 = (u8 *)buf + (count & ~3); > > count %= 4; > > While correct, this is IMHO still difficult to understand for the casual > reader. > > Given the code before casts to "u32 *", and uses "count >>2", and the code > after also casts to "u32 *", what about getting rid of all casts like: > > u32 data = 0; > u32 *buf32 = buf; > > if (is_read) > sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32, > count >> 2); > else > sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32, > count >> 2); > > /* if count was multiple of 4 */ > if (!(count & 0x3)) > return; > > buf32 += count >> 2; > count %= 4; > > if (is_read) { > sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1); > memcpy(buf32, &data, count); > } else { > memcpy(&data, buf32, count); > sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1); > } > > return; > } > Good idea. I just tried it and it seems to work. I'll resend a patch. > u32 *buf32 = buf; GCC didn't like this line without casting buf to a u32 *. It threw an error, not just a warning. Go figure. Question: > u32 data = 0; Any special reason why you are initializing this to 0???? Thank you, Chris
Hi Chris, On Tue, Jul 11, 2017 at 9:37 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tuesday, July 11, 2017, Geert Uytterhoeven wrote: >> > zeroing out the bottom 2 bits of count for out math. >> >> s/out/our/ > > Thank you! > >> > - buf8 = (u8 *)(buf + (count >> 2)); >> > + buf8 = (u8 *)buf + (count & ~3); >> > count %= 4; >> >> While correct, this is IMHO still difficult to understand for the casual >> reader. >> >> Given the code before casts to "u32 *", and uses "count >>2", and the code >> after also casts to "u32 *", what about getting rid of all casts like: >> >> u32 data = 0; >> u32 *buf32 = buf; >> >> if (is_read) >> sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32, >> count >> 2); >> else >> sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32, >> count >> 2); >> >> /* if count was multiple of 4 */ >> if (!(count & 0x3)) >> return; >> >> buf32 += count >> 2; >> count %= 4; >> >> if (is_read) { >> sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, &data, 1); >> memcpy(buf32, &data, count); >> } else { >> memcpy(&data, buf32, count); >> sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, &data, 1); >> } >> >> return; >> } >> > > Good idea. I just tried it and it seems to work. I'll resend a patch. > > >> u32 *buf32 = buf; > > GCC didn't like this line without casting buf to a u32 *. It threw an > error, not just a warning. Go figure. Sorry, the cast is indeed missing, as buf is not a void *. > Question: >> u32 data = 0; > > Any special reason why you are initializing this to 0???? I think the original code did that, too. Hmm, I got mislead by the curly braces, there's no "0" in between them. It's also a bit safer to not write uninitialized data to the CTL_SD_DATA_PORT register. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index 77e7b56a9099..5dfc556ccedf 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -428,7 +428,7 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host, if (!(count & 0x3)) return; - buf8 = (u8 *)(buf + (count >> 2)); + buf8 = (u8 *)buf + (count & ~3); count %= 4; if (is_read) {
The existing code gives an incorrect value. The buffer pointer 'buf' was of type unsigned short *, and 'count' was a number in bytes, so the pointer should have been cast before doing any pointer arithmetic. Since we know the code before it is doing as many 4-byte transfers as possible, we just need a pointer to where it left off in the buffer, hence zeroing out the bottom 2 bits of count for out math. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Fixes: 8185e51f358a: ("mmc: tmio-mmc: add support for 32bit data port") Signed-off-by: Chris Brandt <chris.brandt@renesas.com> --- drivers/mmc/host/tmio_mmc_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)