[PATCH 0/2] two trivial PMU fixes

[PATCH 0/2] two trivial PMU fixes

[Alexandre Courbot]

Two trivial PMU-related fixes for Tegra:
- Add a dummy func member to GK20A, since nvkm_pmmu_pgob() unconditionally
  dereferences it
- Check whether a PMU actually exists before calling nvkm_pmmu_pgob(). During
  early bringup we are likely to not have a PMU

Alexandre Courbot (2):
  pmu/gk20a: add dummy func
  gr/gf100: do not assume a PMU is present

 drm/nouveau/nvkm/engine/gr/gf100.c  | 3 ++-
 drm/nouveau/nvkm/subdev/pmu/gk20a.c | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.5.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 1/2] pmu/gk20a: add dummy func

[Alexandre Courbot]

Some parts of the driver expect the PMU to have a func member, so set
one for GK20A to avoid NULL pointer derefences.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drm/nouveau/nvkm/subdev/pmu/gk20a.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
index 6689d0290a7e..f46e3310e21a 100644
--- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
+++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
@@ -202,6 +202,10 @@ gk20a_dvfs_data= {
 	.p_smooth = 1,
 };
 
+static const struct nvkm_pmu_func
+gk20a_pmu_func = {
+};
+
 static const struct nvkm_subdev_func
 gk20a_pmu = {
 	.init = gk20a_pmu_init,
@@ -221,6 +225,7 @@ gk20a_pmu_new(struct nvkm_device *device, int index, struct nvkm_pmu **ppmu)
 	*ppmu = &pmu->base;
 
 	nvkm_subdev_ctor(&gk20a_pmu, device, index, 0, &pmu->base.subdev);
+	pmu->base.func = &gk20a_pmu_func;
 	pmu->data = &gk20a_dvfs_data;
 	nvkm_alarm_init(&pmu->alarm, gk20a_pmu_dvfs_work);
 	return 0;
-- 
2.5.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 2/2] gr/gf100: do not assume a PMU is present

[Alexandre Courbot]

Some devices may not have a PMU. Avoid a NULL pointer dereference in
such cases.

Signed-off-by: Alexandre Courbot <acourbot@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);
 
 	ret = nvkm_memory_new(device, NVKM_MEM_TARGET_INST, 0x1000, 256, false,
 			      &gr->unk4188b4);
-- 
2.5.1

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 0/2] two trivial PMU fixes

[Alexandre Courbot]

On 09/03/2015 03:32 PM, Alexandre Courbot wrote:
> Two trivial PMU-related fixes for Tegra:
> - Add a dummy func member to GK20A, since nvkm_pmmu_pgob() unconditionally
>    dereferences it
> - Check whether a PMU actually exists before calling nvkm_pmmu_pgob(). During
>    early bringup we are likely to not have a PMU

Forgot to mention: it would be nice to have these merged in the 4.3 
cycle, since GK20A needs at least the first patch to probe successfully.

Thanks!

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] pmu/gk20a: add dummy func

[Ben Skeggs]

On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Some parts of the driver expect the PMU to have a func member, so set
> one for GK20A to avoid NULL pointer derefences.
This part should already be done in the current code.

>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drm/nouveau/nvkm/subdev/pmu/gk20a.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drm/nouveau/nvkm/subdev/pmu/gk20a.c b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> index 6689d0290a7e..f46e3310e21a 100644
> --- a/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> +++ b/drm/nouveau/nvkm/subdev/pmu/gk20a.c
> @@ -202,6 +202,10 @@ gk20a_dvfs_data= {
>         .p_smooth = 1,
>  };
>
> +static const struct nvkm_pmu_func
> +gk20a_pmu_func = {
> +};
> +
>  static const struct nvkm_subdev_func
>  gk20a_pmu = {
>         .init = gk20a_pmu_init,
> @@ -221,6 +225,7 @@ gk20a_pmu_new(struct nvkm_device *device, int index, struct nvkm_pmu **ppmu)
>         *ppmu = &pmu->base;
>
>         nvkm_subdev_ctor(&gk20a_pmu, device, index, 0, &pmu->base.subdev);
> +       pmu->base.func = &gk20a_pmu_func;
>         pmu->data = &gk20a_dvfs_data;
>         nvkm_alarm_init(&pmu->alarm, gk20a_pmu_dvfs_work);
>         return 0;
> --
> 2.5.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] gr/gf100: do not assume a PMU is present

[Ben Skeggs]

On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Some devices may not have a PMU. Avoid a NULL pointer dereference in
> such cases.
>
> Signed-off-by: Alexandre Courbot <acourbot@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] pmu/gk20a: add dummy func

[Alexandre Courbot]

On Thu, Sep 3, 2015 at 4:07 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> Some parts of the driver expect the PMU to have a func member, so set
>> one for GK20A to avoid NULL pointer derefences.
> This part should already be done in the current code.

Ah indeed, it was right before my eyes:

    static const struct nvkm_pmu_func func = {};
...
    pmu->base.func = &func;

Dropping this patch, sorry about the noise.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] gr/gf100: do not assume a PMU is present

[Alexandre Courbot]

On Thu, Sep 3, 2015 at 4:08 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> Some devices may not have a PMU. Avoid a NULL pointer dereference in
>> such cases.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@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.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] gr/gf100: do not assume a PMU is present

[Ben Skeggs]

On 3 September 2015 at 17:11, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Thu, Sep 3, 2015 at 4:08 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
>> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>> Some devices may not have a PMU. Avoid a NULL pointer dereference in
>>> such cases.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@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.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] gr/gf100: do not assume a PMU is present

[Alexandre Courbot]

On Thu, Sep 3, 2015 at 4:16 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
> On 3 September 2015 at 17:11, Alexandre Courbot <gnurou@gmail.com> wrote:
>> On Thu, Sep 3, 2015 at 4:08 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
>>> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>>> Some devices may not have a PMU. Avoid a NULL pointer dereference in
>>>> such cases.
>>>>
>>>> Signed-off-by: Alexandre Courbot <acourbot@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.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] gr/gf100: do not assume a PMU is present

[Ben Skeggs]

On 3 September 2015 at 17:18, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Thu, Sep 3, 2015 at 4:16 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
>> On 3 September 2015 at 17:11, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> On Thu, Sep 3, 2015 at 4:08 PM, Ben Skeggs <skeggsb@gmail.com> wrote:
>>>> On 3 September 2015 at 16:32, Alexandre Courbot <acourbot@nvidia.com> wrote:
>>>>> Some devices may not have a PMU. Avoid a NULL pointer dereference in
>>>>> such cases.
>>>>>
>>>>> Signed-off-by: Alexandre Courbot <acourbot@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 :)
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau