Ben Skeggs
2015-Sep-03 07:08 UTC
[Nouveau] [PATCH 2/2] gr/gf100: do not assume a PMU is present
On 3 September 2015 at 16:32, Alexandre Courbot <acourbot at nvidia.com> wrote:> Some devices may not have a PMU. Avoid a NULL pointer dereference in > such cases. > > Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> > --- > drm/nouveau/nvkm/engine/gr/gf100.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c b/drm/nouveau/nvkm/engine/gr/gf100.c > index f1358a564e3e..f252fa2d7cf9 100644 > --- a/drm/nouveau/nvkm/engine/gr/gf100.c > +++ b/drm/nouveau/nvkm/engine/gr/gf100.c > @@ -1499,7 +1499,8 @@ gf100_gr_oneinit(struct nvkm_gr *base) > struct nvkm_device *device = gr->base.engine.subdev.device; > int ret, i, j; > > - nvkm_pmu_pgob(device->pmu, false); > + if (device->pmu) > + nvkm_pmu_pgob(device->pmu, false);I'd probably just change the condition in nvkm_pmu_pgob() to (pmu && pmu->func->pgob) ?> > ret = nvkm_memory_new(device, NVKM_MEM_TARGET_INST, 0x1000, 256, false, > &gr->unk4188b4); > -- > 2.5.1 > > _______________________________________________ > Nouveau mailing list > Nouveau at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/nouveau
Alexandre Courbot
2015-Sep-03 07:11 UTC
[Nouveau] [PATCH 2/2] gr/gf100: do not assume a PMU is present
On Thu, Sep 3, 2015 at 4:08 PM, Ben Skeggs <skeggsb at gmail.com> wrote:> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot at nvidia.com> wrote: >> Some devices may not have a PMU. Avoid a NULL pointer dereference in >> such cases. >> >> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >> --- >> drm/nouveau/nvkm/engine/gr/gf100.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c b/drm/nouveau/nvkm/engine/gr/gf100.c >> index f1358a564e3e..f252fa2d7cf9 100644 >> --- a/drm/nouveau/nvkm/engine/gr/gf100.c >> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c >> @@ -1499,7 +1499,8 @@ gf100_gr_oneinit(struct nvkm_gr *base) >> struct nvkm_device *device = gr->base.engine.subdev.device; >> int ret, i, j; >> >> - nvkm_pmu_pgob(device->pmu, false); >> + if (device->pmu) >> + nvkm_pmu_pgob(device->pmu, false); > I'd probably just change the condition in nvkm_pmu_pgob() to (pmu && > pmu->func->pgob) ?It seems logical to me that the caller should check that the object it tries to call a method on is valid (just like in C++ you don't expect methods to check whether this == NULL), but your call.
Ben Skeggs
2015-Sep-03 07:16 UTC
[Nouveau] [PATCH 2/2] gr/gf100: do not assume a PMU is present
On 3 September 2015 at 17:11, Alexandre Courbot <gnurou at gmail.com> wrote:> On Thu, Sep 3, 2015 at 4:08 PM, Ben Skeggs <skeggsb at gmail.com> wrote: >> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot at nvidia.com> wrote: >>> Some devices may not have a PMU. Avoid a NULL pointer dereference in >>> such cases. >>> >>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >>> --- >>> drm/nouveau/nvkm/engine/gr/gf100.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c b/drm/nouveau/nvkm/engine/gr/gf100.c >>> index f1358a564e3e..f252fa2d7cf9 100644 >>> --- a/drm/nouveau/nvkm/engine/gr/gf100.c >>> +++ b/drm/nouveau/nvkm/engine/gr/gf100.c >>> @@ -1499,7 +1499,8 @@ gf100_gr_oneinit(struct nvkm_gr *base) >>> struct nvkm_device *device = gr->base.engine.subdev.device; >>> int ret, i, j; >>> >>> - nvkm_pmu_pgob(device->pmu, false); >>> + if (device->pmu) >>> + nvkm_pmu_pgob(device->pmu, false); >> I'd probably just change the condition in nvkm_pmu_pgob() to (pmu && >> pmu->func->pgob) ? > > It seems logical to me that the caller should check that the object it > tries to call a method on is valid (just like in C++ you don't expect > methods to check whether this == NULL), but your call.Yeah, I had similar thoughts when writing some of these accessor functions, and decided on treating them as "helper" functions that act as a stub when the object doesn't exist or doesn't support a particular function. Mainly to avoid having duplicated checks in multiple places that can get out of sync.