Re: drm/nouveau/bios/ramcfg, setting of RON pull value

[Ilia Mirkin]

On Sat, Feb 16, 2019 at 10:02 AM Colin Ian King
<colin.king@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> Static Analysis with CoverityScan as detected an issue with the setting
> of the RON pull value in function nvkm_gddr3_calc in
> drm/nouveau/bios/ramcfg.c
>
> This was introduced by commit: c25bf7b6155cb ("drm/nouveau/bios/ramcfg:
> Separate out RON pull value")
>
> CoverityScan reports the issue as follows:
>
>  84        case 0x20:
>  85                CWL = (ram->next->bios.timing[1] & 0x00000f80) >> 7;
>  86                CL  = (ram->next->bios.timing[1] & 0x0000001f) >> 0;
>  87                WR  = (ram->next->bios.timing[2] & 0x007f0000) >> 16;
>  88                /* XXX: Get these values from the VBIOS instead */
>  89                DLL = !(ram->mr[1] & 0x1);
>
>    CID 1324005 (#1 of 1): Operands don't affect result
> (CONSTANT_EXPRESSION_RESULT)
>
> result_independent_of_operands: !(ram->mr[1] & 768) >> 8 is 0 regardless
> of the values of its operands. This occurs as the operand of assignment.
>
>  90                RON = !(ram->mr[1] & 0x300) >> 8;
>  91                break;
>
> Looking at this, I believe perhaps the correct setting could be:
>
> RON = !((ram->mr[1] & 0x300) >> 8);
>
> ..however I don't have the datasheet available for the H/W so I'm not
> sure if this a correct fix.

Actually looking at the code a bit, I suspect it should just be

RON = (ram->mr[1] & 0x300) >> 8;

since later on, when we recompose the MR (memory register) value, we do:

        ram->mr[1] |= (RON & 0x03) << 8;

(And the whole point here is that we don't know how to get the proper
RON value for that timing table version, so we just copy whatever used
to be there in that case.)

  -ilia