Message ID | 1385745164-10534-1-git-send-email-mpn@google.com |
---|---|
State | Not Applicable, archived |
Delegated to: | Stephen Warren |
Headers | show |
On Fri, Nov 29, 2013 at 06:12:44PM +0100, Michal Nazarewicz wrote: > From: Michal Nazarewicz <mina86@mina86.com> > > u8 type, which is unsigned, is promoted to int, which is singned, when > doing a binary shift. Then, on 64-bit machines, it is further promoted > to unsigned long which may lead to more significant half of the value > to be all ones. To avoid this, explicitly promote to an unsigned type. So this would only become an problem on 64-bit Tegra SoCs, right? I suspect that there are quite a few places where something similar is done and which suffer from the same issue. But I don't think there's a real issue here. Even on 64-bit platforms, writel() will only write 32-bit registers, and therefore discard the upper 32 bits. I think in general perhaps a more proper fix would be to use only u32 instead of unsigned long for these cases, since then the problem goes away. u32 is also the type of the value used by writel() so it makes perfect sense to use it. Thierry
> On Fri, Nov 29, 2013 at 06:12:44PM +0100, Michal Nazarewicz wrote: >> u8 type, which is unsigned, is promoted to int, which is singned, when >> doing a binary shift. Then, on 64-bit machines, it is further promoted >> to unsigned long which may lead to more significant half of the value >> to be all ones. To avoid this, explicitly promote to an unsigned type. On Fri, Nov 29 2013, Thierry Reding wrote: > But I don't think there's a real issue here. Even on 64-bit platforms, > writel() will only write 32-bit registers, and therefore discard the > upper 32 bits. > > I think in general perhaps a more proper fix would be to use only u32 > instead of unsigned long for these cases, since then the problem goes > away. u32 is also the type of the value used by writel() so it makes > perfect sense to use it. Yes, you're absolutely right. I dunno why I missed the fact it's used in writel only. I'll prepare a new patch soon(ish).
This series has only been compile-tested, but it does not introduce any behaviour changes. Michal Nazarewicz (3): spi: tegra114: use u32 for 32-bit register values spi: tegra20-slink: use u32 for 32-bit register values spi: tegra20-sflash: use u32 for 32-bit register values drivers/spi/spi-tegra114.c | 98 ++++++++++++++-------------------------- drivers/spi/spi-tegra20-sflash.c | 22 ++++----- drivers/spi/spi-tegra20-slink.c | 97 ++++++++++++++++----------------------- 3 files changed, 84 insertions(+), 133 deletions(-)
On 12/08/2013 08:35 AM, Michal Nazarewicz wrote: > This series has only been compile-tested, but it does not introduce any > behaviour changes. The series, Tested-by: Stephen Warren <swarren@nvidia.com> Acked-by: Stephen Warren <swarren@nvidia.com> ... although I will observe that the patch subjects, and even patch descriptions, don't describe all the other unrelated code transformations that are rolled into these patches... -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Dec 08, 2013 at 04:35:08PM +0100, Michal Nazarewicz wrote: > This series has only been compile-tested, but it does not introduce any > behaviour changes. Applied, thanks. Like Stephen says please do make sure your commits do as they're described...
diff --git a/drivers/spi/spi-tegra114.c b/drivers/spi/spi-tegra114.c index aaecfb3..2dadc3f 100644 --- a/drivers/spi/spi-tegra114.c +++ b/drivers/spi/spi-tegra114.c @@ -315,7 +315,7 @@ static unsigned tegra_spi_fill_tx_fifo_from_client_txbuf( for (count = 0; count < max_n_32bit; count++) { x = 0; for (i = 0; (i < 4) && nbytes; i++, nbytes--) - x |= (*tx_buf++) << (i*8); + x |= (unsigned)(*tx_buf++) << (i*8); tegra_spi_writel(tspi, x, SPI_TX_FIFO); } } else { @@ -326,7 +326,7 @@ static unsigned tegra_spi_fill_tx_fifo_from_client_txbuf( x = 0; for (i = 0; nbytes && (i < tspi->bytes_per_word); i++, nbytes--) - x |= ((*tx_buf++) << i*8); + x |= (unsigned)(*tx_buf++) << (i*8); tegra_spi_writel(tspi, x, SPI_TX_FIFO); } } diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c index e66715b..0fc4f7d 100644 --- a/drivers/spi/spi-tegra20-slink.c +++ b/drivers/spi/spi-tegra20-slink.c @@ -331,7 +331,7 @@ static unsigned tegra_slink_fill_tx_fifo_from_client_txbuf( for (count = 0; count < max_n_32bit; count++) { x = 0; for (i = 0; (i < 4) && nbytes; i++, nbytes--) - x |= (*tx_buf++) << (i*8); + x |= (unsigned)(*tx_buf++) << (i*8); tegra_slink_writel(tspi, x, SLINK_TX_FIFO); } } else { @@ -342,7 +342,7 @@ static unsigned tegra_slink_fill_tx_fifo_from_client_txbuf( x = 0; for (i = 0; nbytes && (i < tspi->bytes_per_word); i++, nbytes--) - x |= ((*tx_buf++) << i*8); + x |= (unsigned)(*tx_buf++) << (i*8); tegra_slink_writel(tspi, x, SLINK_TX_FIFO); } }