Nouveau - Sep 2015 - [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

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.

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.