| Message ID | 20220304202406.846485-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/a6xx: Fix missing ARRAY_SIZE() check | expand |
On Fri, 4 Mar 2022 at 23:23, Rob Clark <robdclark@gmail.com> wrote: > > From: Rob Clark <robdclark@chromium.org> > > Fixes: f6d62d091cfd ("drm/msm/a6xx: add support for Adreno 660 GPU") > Signed-off-by: Rob Clark <robdclark@chromium.org> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> However see the comment below. > --- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 02b47977b5c3..6406d8c3411a 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -687,6 +687,7 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > > BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); > BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); > + BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); The magic number 32 and 48 are repeated through this code. I'd suggest to define them and use defined names. It can come up as a separate commit. > if (adreno_is_a650(adreno_gpu)) { > regs = a650_protect; > -- > 2.35.1 >
On Fri, Mar 4, 2022 at 1:47 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Fri, 4 Mar 2022 at 23:23, Rob Clark <robdclark@gmail.com> wrote: > > > > From: Rob Clark <robdclark@chromium.org> > > > > Fixes: f6d62d091cfd ("drm/msm/a6xx: add support for Adreno 660 GPU") > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > However see the comment below. > > > --- > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > index 02b47977b5c3..6406d8c3411a 100644 > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > @@ -687,6 +687,7 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > > > > BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); > > BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); > > + BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); > > The magic number 32 and 48 are repeated through this code. I'd suggest > to define them and use defined names. > It can come up as a separate commit. > Or perhaps instead: ---- diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 6406d8c3411a..58c371930fb4 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -683,20 +683,23 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); const u32 *regs = a6xx_protect; - unsigned i, count = ARRAY_SIZE(a6xx_protect), count_max = 32; - - BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); - BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); - BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); + unsigned i, count, count_max; if (adreno_is_a650(adreno_gpu)) { regs = a650_protect; count = ARRAY_SIZE(a650_protect); count_max = 48; + BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); } else if (adreno_is_a660_family(adreno_gpu)) { regs = a660_protect; count = ARRAY_SIZE(a660_protect); count_max = 48; + BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); + } else { + regs = a6xx_protect; + count = ARRAY_SIZE(a6xx_protect); + count_max = 32; + BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); } /* ---- that moves each of the two uses of constant together.. adding three #defines each used only twice seems a bit silly, IMHO BR, -R
On Sat, 5 Mar 2022 at 00:57, Rob Clark <robdclark@gmail.com> wrote: > > On Fri, Mar 4, 2022 at 1:47 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Fri, 4 Mar 2022 at 23:23, Rob Clark <robdclark@gmail.com> wrote: > > > > > > From: Rob Clark <robdclark@chromium.org> > > > > > > Fixes: f6d62d091cfd ("drm/msm/a6xx: add support for Adreno 660 GPU") > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > However see the comment below. > > > > > --- > > > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > > index 02b47977b5c3..6406d8c3411a 100644 > > > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > > > @@ -687,6 +687,7 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > > > > > > BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); > > > BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); > > > + BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); > > > > The magic number 32 and 48 are repeated through this code. I'd suggest > > to define them and use defined names. > > It can come up as a separate commit. > > > > Or perhaps instead: IMO this is much better. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > ---- > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > index 6406d8c3411a..58c371930fb4 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c > @@ -683,20 +683,23 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) > { > struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); > const u32 *regs = a6xx_protect; > - unsigned i, count = ARRAY_SIZE(a6xx_protect), count_max = 32; > - > - BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); > - BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); > - BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); > + unsigned i, count, count_max; > > if (adreno_is_a650(adreno_gpu)) { > regs = a650_protect; > count = ARRAY_SIZE(a650_protect); > count_max = 48; > + BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); > } else if (adreno_is_a660_family(adreno_gpu)) { > regs = a660_protect; > count = ARRAY_SIZE(a660_protect); > count_max = 48; > + BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); > + } else { > + regs = a6xx_protect; > + count = ARRAY_SIZE(a6xx_protect); > + count_max = 32; > + BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); > } > > /* > ---- > > that moves each of the two uses of constant together.. adding three > #defines each used only twice seems a bit silly, IMHO > > BR, > -R
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 02b47977b5c3..6406d8c3411a 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -687,6 +687,7 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu) BUILD_BUG_ON(ARRAY_SIZE(a6xx_protect) > 32); BUILD_BUG_ON(ARRAY_SIZE(a650_protect) > 48); + BUILD_BUG_ON(ARRAY_SIZE(a660_protect) > 48); if (adreno_is_a650(adreno_gpu)) { regs = a650_protect;