[PATCH xf86-video-amdgpu 1/2] Fix crash in PCI probe path (v3)

[William Lewis]

Would it not make more sense to set pAMDGPUEnt->fd to -1 beneath the 
error_amdgpu tag?

On 10/28/15 08:24, Jammy Zhou wrote:
> The crash is caused by the NULL value returned by AMDGPUPTR(pScrn),
> because the driverPrivate is not allocated yet in PciProbe phase,
> and it is usually done in the PreInit phase.
>
> Use pAMDGPUEnt->fd instead of info->dri2.drm_fd to avoid AMDGPUInfoPtr
> related code in amdgpu_open_drm_master, so that the crash can be fixed.
>
> v3: some more cleanup
> v2: switch to pAMDGPUEnt->fd, and update the commit message
>
> Signed-off-by: Jammy Zhou <Jammy.Zhou at amd.com>
> ---
>   src/amdgpu_probe.c | 37 +++++++++++++------------------------
>   1 file changed, 13 insertions(+), 24 deletions(-)
>
> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 481271b..f6e675c 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -147,24 +147,14 @@ static int amdgpu_kernel_open_fd(ScrnInfoPtr pScrn, struct pci_device *dev,
>   	return fd;
>   }
>   
> -static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn)
> +static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn, AMDGPUEntPtr pAMDGPUEnt,
> +				   struct pci_device *pci_dev, int entity_num)
>   {
> -	AMDGPUInfoPtr  info   = AMDGPUPTR(pScrn);
> -	AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(pScrn);
>   	drmSetVersion sv;
>   	int err;
>   
> -	if (pAMDGPUEnt->fd) {
> -		xf86DrvMsg(pScrn->scrnIndex, X_INFO,
> -			   " reusing fd for second head\n");
> -
> -		info->drmmode.fd = info->dri2.drm_fd = pAMDGPUEnt->fd;
> -		pAMDGPUEnt->fd_ref++;
> -		return TRUE;
> -	}
> -
> -	info->dri2.drm_fd = amdgpu_kernel_open_fd(pScrn, info->PciInfo, NULL);
> -	if (info->dri2.drm_fd == -1)
> +	pAMDGPUEnt->fd = amdgpu_kernel_open_fd(pScrn, pci_dev, NULL);
> +	if (pAMDGPUEnt->fd == -1)
>   		return FALSE;
>   
>   	/* Check that what we opened was a master or a master-capable FD,
> @@ -175,18 +165,18 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr pScrn)
>   	sv.drm_di_minor = 1;
>   	sv.drm_dd_major = -1;
>   	sv.drm_dd_minor = -1;
> -	err = drmSetInterfaceVersion(info->dri2.drm_fd, &sv);
> +	err = drmSetInterfaceVersion(pAMDGPUEnt->fd, &sv);
>   	if (err != 0) {
>   		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
>   			   "[drm] failed to set drm interface version.\n");
> -		drmClose(info->dri2.drm_fd);
> +		drmClose(pAMDGPUEnt->fd);
>   		return FALSE;
>   	}
>   
>   	return TRUE;
>   }
>   
> -static Bool amdgpu_get_scrninfo(int entity_num, void *pci_dev)
> +static Bool amdgpu_get_scrninfo(int entity_num, struct pci_device *pci_dev)
>   {
>   	ScrnInfoPtr pScrn = NULL;
>   	EntityInfoPtr pEnt;
> @@ -236,11 +226,13 @@ static Bool amdgpu_get_scrninfo(int entity_num, void *pci_dev)
>   		uint32_t minor_version;
>   
>   		pPriv->ptr = xnfcalloc(sizeof(AMDGPUEntRec), 1);
> -		pAMDGPUEnt = pPriv->ptr;
> +		if (!pPriv->ptr)
> +			return FALSE;
>   
> -		if (amdgpu_open_drm_master(pScrn)) {
> +		pAMDGPUEnt = pPriv->ptr;
> +		if (!amdgpu_open_drm_master(pScrn, pAMDGPUEnt,
> +					    pci_dev, entity_num))
>   			goto error_fd;
> -		}
>   
>   		pAMDGPUEnt->fd_ref = 1;
>   
> @@ -252,8 +244,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, void *pci_dev)
>   				   "amdgpu_device_initialize failed\n");
>   			goto error_amdgpu;
>   		}
> -	} else {
> -		pAMDGPUEnt = pPriv->ptr;
>   	}
>   
>   	xf86SetEntityInstanceForScreen(pScrn, pEnt->index,
> @@ -266,7 +256,6 @@ static Bool amdgpu_get_scrninfo(int entity_num, void *pci_dev)
>   
>   error_amdgpu:
>   	drmClose(pAMDGPUEnt->fd);
> -	pAMDGPUEnt->fd = 0;
>   error_fd:
>   	free(pPriv->ptr);
>   	return FALSE;
> @@ -276,7 +265,7 @@ static Bool
>   amdgpu_pci_probe(DriverPtr pDriver,
>   		 int entity_num, struct pci_device *device, intptr_t match_data)
>   {
> -	return amdgpu_get_scrninfo(entity_num, (void *)device);
> +	return amdgpu_get_scrninfo(entity_num, device);
>   }
>   
>   static Bool AMDGPUDriverFunc(ScrnInfoPtr scrn, xorgDriverFuncOp op, void *data)