[4/7] drm/amdkfd: Make kfd_lookup_process_by_pasid() well defined

Submitted by Edward O'Callaghan on Sept. 10, 2016, 1:31 a.m.

Details

Message ID 1473471099-16914-5-git-send-email-funfunctor@folklore1984.net
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Edward O'Callaghan Sept. 10, 2016, 1:31 a.m.
Ensure we return a NULL on the fail branch so that the call
site may BUG_ON() it.

Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 4f3849a..8d78052 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -481,13 +481,14 @@  bool kfd_has_process_device_data(struct kfd_process *p)
 /* This returns with process->mutex locked. */
 struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
 {
-	struct kfd_process *p;
+	struct kfd_process *p, *ret_p = NULL;
 	unsigned int temp;
 
 	int idx = srcu_read_lock(&kfd_processes_srcu);
 
 	hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
 		if (p->pasid == pasid) {
+			ret_p = p;
 			mutex_lock(&p->mutex);
 			break;
 		}
@@ -495,5 +496,5 @@  struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
 
 	srcu_read_unlock(&kfd_processes_srcu, idx);
 
-	return p;
+	return ret_p;
 }

Comments

On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
<funfunctor@folklore1984.net> wrote:
> Ensure we return a NULL on the fail branch so that the call
> site may BUG_ON() it.
>
> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index 4f3849a..8d78052 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -481,13 +481,14 @@ bool kfd_has_process_device_data(struct kfd_process *p)
>  /* This returns with process->mutex locked. */
>  struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>  {
> -       struct kfd_process *p;
> +       struct kfd_process *p, *ret_p = NULL;
>         unsigned int temp;
>
>         int idx = srcu_read_lock(&kfd_processes_srcu);
>
>         hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>                 if (p->pasid == pasid) {
> +                       ret_p = p;
>                         mutex_lock(&p->mutex);
>                         break;
>                 }
> @@ -495,5 +496,5 @@ struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>
>         srcu_read_unlock(&kfd_processes_srcu, idx);
>
> -       return p;
> +       return ret_p;
>  }
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

This patch is not needed. p will always be a valid pointer on return or NULL.
Unless we hit the "p->pasid == pasid" condition, in which case p is
valid, p will always be NULL when finishing the hash_for_each_rcu
macro.

        Oded
On 09/11/2016 01:55 AM, Oded Gabbay wrote:
> On Sat, Sep 10, 2016 at 4:31 AM, Edward O'Callaghan
> <funfunctor@folklore1984.net> wrote:
>> Ensure we return a NULL on the fail branch so that the call
>> site may BUG_ON() it.
>>
>> Signed-off-by: Edward O'Callaghan <funfunctor@folklore1984.net>
>> ---
>>  drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> index 4f3849a..8d78052 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>> @@ -481,13 +481,14 @@ bool kfd_has_process_device_data(struct kfd_process *p)
>>  /* This returns with process->mutex locked. */
>>  struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>>  {
>> -       struct kfd_process *p;
>> +       struct kfd_process *p, *ret_p = NULL;
>>         unsigned int temp;
>>
>>         int idx = srcu_read_lock(&kfd_processes_srcu);
>>
>>         hash_for_each_rcu(kfd_processes_table, temp, p, kfd_processes) {
>>                 if (p->pasid == pasid) {
>> +                       ret_p = p;
>>                         mutex_lock(&p->mutex);
>>                         break;
>>                 }
>> @@ -495,5 +496,5 @@ struct kfd_process *kfd_lookup_process_by_pasid(unsigned int pasid)
>>
>>         srcu_read_unlock(&kfd_processes_srcu, idx);
>>
>> -       return p;
>> +       return ret_p;
>>  }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> This patch is not needed. p will always be a valid pointer on return or NULL.
> Unless we hit the "p->pasid == pasid" condition, in which case p is
> valid, p will always be NULL when finishing the hash_for_each_rcu
> macro.

Ah you are correct, I didn't notice it was initialized inside the macro.

> 
>         Oded
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>