| Message ID | 87wq9eohuc.wl%kuninori.morimoto.gx@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Morimoto-san, On Mon, Sep 8, 2014 at 6:29 AM, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -376,6 +376,43 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command > return 0; > } > > +static void tmio_mmc_transfer_data(struct tmio_mmc_host *host, > + unsigned short *buf, > + unsigned int count) > +{ > + int is_read = host->data->flags & MMC_DATA_READ; > + u16 extra; > + u8 *extra8; > + u8 *buf8; > + > + /* > + * Transfer the data > + */ > + if (is_read) > + sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); > + else > + sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); > + > + /* count was even number */ > + if (!(count & 0x1)) > + return; > + > + /* count was odd number */ > + > + extra8 = (u8 *)&extra; I would drop the extra8 variable, if possible... (see below) > + buf8 = (u8 *)(buf + ((count - 1) >> 1)); The "-1" is not really needed. > + if (is_read) { > + extra = sd_ctrl_read16(host, CTL_SD_DATA_PORT); > + > + buf8[1] = extra8[1]; > + } else { > + extra = buf8[1] << 8; > + > + sd_ctrl_write16(host, CTL_SD_DATA_PORT, extra); > + } > +} Shouldn't this use buf8[0] instead of buf8[1]? Originally, this is a byte buffer, right? Or is it a byte-swapped 16-bit buffer? Does this code need to care about endianness? Due to using byte array accesses on read, and shifts on write, the result differs depending on endianness. On little endian, you have Read: extra = [ buf8[1] | buf8[0] ] Write: extra = [ buf8[1] | 0 ] On big endian, you have Read: extra = [ buf8[0] | buf8[1] ] Write: extra = [ buf8[1] | 0 ] I think it's better to use shifts for both read and write, and add an le16_to_cpu()/ cpu_to_le16() if needed. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert > + buf8 = (u8 *)(buf + ((count - 1) >> 1)); > > The "-1" is not really needed. This -1 is needed. We want to have... count - offset 1 0 2 0 3 1 4 1 ... > I think it's better to use shifts for both read and write, and add an > le16_to_cpu()/ > cpu_to_le16() if needed. I see. will fix in v2 Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Morimoto-san, On Mon, Sep 8, 2014 at 9:36 AM, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: > > + buf8 = (u8 *)(buf + ((count - 1) >> 1)); >> >> The "-1" is not really needed. > > This -1 is needed. > > We want to have... > count - offset > 1 0 > 2 0 > 3 1 > 4 1 > ... If count is even, we never get that far, so the even numbers should not be present in your table above? If count is odd, "(count - 1) >> 1 == count >> 1". 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert > If count is even, we never get that far, so the even numbers should not > be present in your table above? > > If count is odd, "(count - 1) >> 1 == count >> 1". Ahh, yes, indeed. About endianness / byte access, it requires byte access especially "read" case. Because if count was odd number, using shift with u16 will might be buffer overflow I need to check cpu_to_le16() / le16_to_cpu() Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Morimoto-san, On Mon, Sep 8, 2014 at 10:06 AM, Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote: > About endianness / byte access, it requires byte access > especially "read" case. > Because if count was odd number, > using shift with u16 will might be buffer overflow Of course the buffer must be accessed using byte accesses. But sd_ctrl_read16() returns u16, and sd_ctrl_write16() takes u16. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 9/8/2014 8:29 AM, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > Current tmio is using sd_ctrl_read16/write16_rep() > for data transfer. > It works if transfer size was even number, > but, last 1 byte will be ignored if > transfer size was odd number. > This patch adds new tmio_mmc_transfer_data() > and solve this issue. > Tested-by: Shinobu Uehara <shinobu.uehara.xc@renesas.com> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > drivers/mmc/host/tmio_mmc_pio.c | 42 +++++++++++++++++++++++++++++++++++---- > 1 file changed, 38 insertions(+), 4 deletions(-) > diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c > index ba45413..817ec7d 100644 > --- a/drivers/mmc/host/tmio_mmc_pio.c > +++ b/drivers/mmc/host/tmio_mmc_pio.c > @@ -376,6 +376,43 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command > return 0; > } > > +static void tmio_mmc_transfer_data(struct tmio_mmc_host *host, > + unsigned short *buf, > + unsigned int count) > +{ > + int is_read = host->data->flags & MMC_DATA_READ; > + u16 extra; > + u8 *extra8; > + u8 *buf8; > + > + /* > + * Transfer the data > + */ > + if (is_read) > + sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); > + else > + sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); > + > + /* count was even number */ > + if (!(count & 0x1)) > + return; > + > + /* count was odd number */ > + > + extra8 = (u8 *)&extra; > + buf8 = (u8 *)(buf + ((count - 1) >> 1)); You don't need to subtract 1. > + > + if (is_read) { > + extra = sd_ctrl_read16(host, CTL_SD_DATA_PORT); > + > + buf8[1] = extra8[1]; > + } else { > + extra = buf8[1] << 8; Hm, why [1] here and above? Shouldn't it be [0] instead? > + > + sd_ctrl_write16(host, CTL_SD_DATA_PORT, extra); > + } > +} > + > /* > * This chip always returns (at least?) as much data as you ask for. > * I'm unsure what happens if you ask for less than a block. This should be WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c index ba45413..817ec7d 100644 --- a/drivers/mmc/host/tmio_mmc_pio.c +++ b/drivers/mmc/host/tmio_mmc_pio.c @@ -376,6 +376,43 @@ static int tmio_mmc_start_command(struct tmio_mmc_host *host, struct mmc_command return 0; } +static void tmio_mmc_transfer_data(struct tmio_mmc_host *host, + unsigned short *buf, + unsigned int count) +{ + int is_read = host->data->flags & MMC_DATA_READ; + u16 extra; + u8 *extra8; + u8 *buf8; + + /* + * Transfer the data + */ + if (is_read) + sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); + else + sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); + + /* count was even number */ + if (!(count & 0x1)) + return; + + /* count was odd number */ + + extra8 = (u8 *)&extra; + buf8 = (u8 *)(buf + ((count - 1) >> 1)); + + if (is_read) { + extra = sd_ctrl_read16(host, CTL_SD_DATA_PORT); + + buf8[1] = extra8[1]; + } else { + extra = buf8[1] << 8; + + sd_ctrl_write16(host, CTL_SD_DATA_PORT, extra); + } +} + /* * This chip always returns (at least?) as much data as you ask for. * I'm unsure what happens if you ask for less than a block. This should be @@ -408,10 +445,7 @@ static void tmio_mmc_pio_irq(struct tmio_mmc_host *host) count, host->sg_off, data->flags); /* Transfer the data */ - if (data->flags & MMC_DATA_READ) - sd_ctrl_read16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); - else - sd_ctrl_write16_rep(host, CTL_SD_DATA_PORT, buf, count >> 1); + tmio_mmc_transfer_data(host, buf, count); host->sg_off += count;