[PATCH 11/23] drm/amdgpu: check function points valid before use. (v2)

[Christian König]

Am 09.03.2017 um 16:57 schrieb Alex Deucher:
> On Thu, Mar 9, 2017 at 10:53 AM, Christian König
> <deathsimple at vodafone.de> wrote:
>> Am 09.03.2017 um 16:25 schrieb Alex Deucher:
>>> From: Rex Zhu <Rex.Zhu at amd.com>
>>>
>>> v2: agd: integrate Christian's comments.
>>>
>>> Signed-off-by: Rex Zhu <Rex.Zhu at amd.com>
>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 26
>>> ++++++++++++--------------
>>>    1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 641c286..1c854c0 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1191,13 +1191,12 @@ int amdgpu_set_clockgating_state(struct
>>> amdgpu_device *adev,
>>>          for (i = 0; i < adev->num_ip_blocks; i++) {
>>>                  if (!adev->ip_blocks[i].status.valid)
>>>                          continue;
>>> -               if (adev->ip_blocks[i].version->type == block_type) {
>>> -                       r =
>>> adev->ip_blocks[i].version->funcs->set_clockgating_state((void *)adev,
>>> -
>>> state);
>>> -                       if (r)
>>> -                               return r;
>>> -                       break;
>>> -               }
>>> +               if (adev->ip_blocks[i].version->type != block_type)
>>> +                       continue;
>>> +               if
>>> (!adev->ip_blocks[i].version->funcs->set_clockgating_state)
>>> +                       continue;
>>> +               r =
>>> adev->ip_blocks[i].version->funcs->set_clockgating_state(
>>> +                       (void *)adev, state);
>>
>> Aren't we missing the "if (r) return r" now? Not that it makes much
>> difference, cause we currently only have one block of each type, don't we?
> yeah, I dropped them on purpose since we could theoretically have
> multiple blocks of the same type.  We don't at the moment.

But in this case we should probably at least print some warning.

Otherwise if we ever get more than one block of a type and the first one 
has a problem we never notice that because the return value is lost 
after the second instance is called.

Christian.

>
> Alex
>
>> Christian.
>>
>>
>>>          }
>>>          return r;
>>>    }
>>> @@ -1211,13 +1210,12 @@ int amdgpu_set_powergating_state(struct
>>> amdgpu_device *adev,
>>>          for (i = 0; i < adev->num_ip_blocks; i++) {
>>>                  if (!adev->ip_blocks[i].status.valid)
>>>                          continue;
>>> -               if (adev->ip_blocks[i].version->type == block_type) {
>>> -                       r =
>>> adev->ip_blocks[i].version->funcs->set_powergating_state((void *)adev,
>>> -
>>> state);
>>> -                       if (r)
>>> -                               return r;
>>> -                       break;
>>> -               }
>>> +               if (adev->ip_blocks[i].version->type != block_type)
>>> +                       continue;
>>> +               if
>>> (!adev->ip_blocks[i].version->funcs->set_powergating_state)
>>> +                       continue;
>>> +               r =
>>> adev->ip_blocks[i].version->funcs->set_powergating_state(
>>> +                       (void *)adev, state);
>>>          }
>>>          return r;
>>>    }
>>
>>