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.
Alexandre Courbot
2015-Sep-03 07:18 UTC
[Nouveau] [PATCH 2/2] gr/gf100: do not assume a PMU is present
On Thu, Sep 3, 2015 at 4:16 PM, Ben Skeggs <skeggsb at gmail.com> wrote:> 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.Ok then. I will wait for your feedback on patch 3/3 before resending.
Ben Skeggs
2015-Sep-03 07:21 UTC
[Nouveau] [PATCH 2/2] gr/gf100: do not assume a PMU is present
On 3 September 2015 at 17:18, Alexandre Courbot <gnurou at gmail.com> wrote:> On Thu, Sep 3, 2015 at 4:16 PM, Ben Skeggs <skeggsb at gmail.com> wrote: >> 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. > > Ok then. I will wait for your feedback on patch 3/3 before resending.3/3 looks fine to me from a quick glance, I'll take your word that it works, and it doesn't effect dGPUs at all anyway :)