Under some circumstances it might be required to return NULL from the
create_address_space callback, meaning that the driver should use global
address space. Use IS_ERR_OR_NULL() to guard aspace->pid assignment
instead of just IS_ERR(). This plays well with the IS_ERR_OR_NULL()
check few lines below.

Fixes: 25faf2f2e065 ("drm/msm: Show process names in gem_describe")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_gpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index a274b8466423..47803f410dbb 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -833,7 +833,7 @@ msm_gpu_create_private_address_space(struct msm_gpu *gpu, struct task_struct *ta
 	 */
 	if (gpu->funcs->create_private_address_space) {
 		aspace = gpu->funcs->create_private_address_space(gpu);
-		if (!IS_ERR(aspace))
+		if (!IS_ERR_OR_NULL(aspace))
 			aspace->pid = get_pid(task_pid(task));
 	}
 

---
base-commit: 9aaeb87ce1e966169a57f53a02ba05b30880ffb8
change-id: 20240908-adreno-fix-cpas-48f3b992439d

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote:
> Under some circumstance

Under what circumstances?

This branch is only taken if there's a .create_private_address_space
callback and it only seems to be there on a[67]xx.

a6xx_create_address_space returns:

- an ERR_PTR if msm_iommu_pagetable_create() fails
- retval of msm_gem_address_space_create() otherwise
   - retval of msm_iommu_pagetable_create() is nullchecked here
     again because we apparently we want to be double sure
   - err_ptr(-enomem) is returned if allocating aspace fails
   - otherwise aspace is allocated somewhere

Konrad

On Mon, 9 Sept 2024 at 13:34, Konrad Dybcio <konradybcio@kernel.org> wrote:
>
> On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote:
> > Under some circumstance
>
> Under what circumstances?
>
> This branch is only taken if there's a .create_private_address_space
> callback and it only seems to be there on a[67]xx.

Existing code doesn't. I stumbled upon it while debugging private
address space translation. And that's why I wrote 'it might be
required' rather than 'the function returns'.
So yes, there is no issue with the current code. And at the same time
not having this in place makes debugging more difficult.


> a6xx_create_address_space returns:
>
> - an ERR_PTR if msm_iommu_pagetable_create() fails
> - retval of msm_gem_address_space_create() otherwise
>    - retval of msm_iommu_pagetable_create() is nullchecked here
>      again because we apparently we want to be double sure
>    - err_ptr(-enomem) is returned if allocating aspace fails
>    - otherwise aspace is allocated somewhere
>
> Konrad



-- 
With best wishes
Dmitry

On 9.09.2024 1:25 PM, Dmitry Baryshkov wrote:
> On Mon, 9 Sept 2024 at 13:34, Konrad Dybcio <konradybcio@kernel.org> wrote:
>>
>> On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote:
>>> Under some circumstance
>>
>> Under what circumstances?
>>
>> This branch is only taken if there's a .create_private_address_space
>> callback and it only seems to be there on a[67]xx.
> 
> Existing code doesn't. I stumbled upon it while debugging private
> address space translation. And that's why I wrote 'it might be
> required' rather than 'the function returns'.
> So yes, there is no issue with the current code. And at the same time
> not having this in place makes debugging more difficult.

Feel free to submit your debugging code in a way that won't mess
with non-debug paths then.. This is trying to solve a non-existent
issue.

Konrad

On Mon, 9 Sept 2024 at 14:29, Konrad Dybcio <konradybcio@kernel.org> wrote:
>
> On 9.09.2024 1:25 PM, Dmitry Baryshkov wrote:
> > On Mon, 9 Sept 2024 at 13:34, Konrad Dybcio <konradybcio@kernel.org> wrote:
> >>
> >> On 8.09.2024 7:59 PM, Dmitry Baryshkov wrote:
> >>> Under some circumstance
> >>
> >> Under what circumstances?
> >>
> >> This branch is only taken if there's a .create_private_address_space
> >> callback and it only seems to be there on a[67]xx.
> >
> > Existing code doesn't. I stumbled upon it while debugging private
> > address space translation. And that's why I wrote 'it might be
> > required' rather than 'the function returns'.
> > So yes, there is no issue with the current code. And at the same time
> > not having this in place makes debugging more difficult.
>
> Feel free to submit your debugging code in a way that won't mess
> with non-debug paths then.. This is trying to solve a non-existent
> issue.

Ack


-- 
With best wishes
Dmitry

> Under some circumstances it might be required to return NULL from the
> create_address_space callback, meaning that the driver should use global
> address space. …

Please avoid a typo in the summary phrase.

Regards,
Markus