On Fri, Oct 17, 2014 at 02:31:23PM +0300, Dan Carpenter wrote:
> Hello Thierry Reding,
>
> The patch e687651bc1ed: "drm/tegra: Add hardware cursor support" from
> Dec 20, 2013, leads to the following static checker warning:
>
> drivers/gpu/drm/tegra/dc.c:597 tegra_dc_cursor_set2()
> warn: mask and shift to zero
>
> drivers/gpu/drm/tegra/dc.c
> 594 if (bo) {
> 595 unsigned long addr = (bo->paddr & 0xfffffc00) >> 10;
> 596 #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> 597 unsigned long high = (bo->paddr & 0xfffffffc) >> 32;
How timely. I'm in fact revising this code at this very moment. =)
The masking here is indeed somewhat fishy. While it does the right
thing, it's also needless to some degree. A better version would be:
addr = (bo->paddr >> 10) & 0x3fffff;
Since this is written to a register where the upper 10 bits (actually
only 8 because bits 22 and 23 are reserved) are used for additional
control parameters (cursor size and clipping region). The above will
properly write bits 31:10 into bits 21:0 of addr.
For the upper 32 bits the mask is completely redundant. According to the
register documentation, bits 1:0 should correspond to bits 33:32 of the
physical address, so something like:
high = (bo->paddr >> 32) & 0x3;
would be more correct.
Thierry
Attachment:
pgpNgWzNMITeM.pgp
Description: PGP signature
