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

2015-10-28 Thread Jammy Zhou 
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 
---
 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, );
+   err = drmSetInterfaceVersion(pAMDGPUEnt->fd, );
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)
-- 
1.9.1

___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

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

2015-10-28 Thread Michel Dänzer 
On 28.10.2015 22:00, William Lewis wrote:
> Would it not make more sense to set pAMDGPUEnt->fd to -1 beneath the 
> error_amdgpu tag?

No:


> On 10/28/15 08:24, Jammy Zhou wrote:
>> +pAMDGPUEnt = pPriv->ptr;

[...]

>> @@ -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;

pAMDGPUEnt == pPriv->ptr, so the memory pointed to by pAMDGPUEnt is
freed immediately after the removed assignment.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

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

2015-10-28 Thread 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 
> ---
>   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, );
> + err = drmSetInterfaceVersion(pAMDGPUEnt->fd, );
>   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)

___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati

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

2015-10-28 Thread Michel Dänzer 
On 28.10.2015 22: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 
> ---
>  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)

Sorry I wasn't explicit about this before, but please don't add an
unused entity_num parameter.


> - 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;
> - }

You could rebase on top of the attached patch, since this is a logically
separate change from the crash fix.


> -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;
[...]
> @@ -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)
> 

These changes make sense, but are again logically separate from the
crash fix.

If you want, I can send out a new series with the logical changes split
up into separate patches.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
From eb618a4008821dcbf558066058a02b1b74ad6be4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michel=20D=C3=A4nzer?= 
Date: Wed, 28 Oct 2015 17:56:13 +0900
Subject: [PATCH xf86-video-amdgpu] Remove dead code from
 amdgpu_open_drm_master
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

amdgpu_get_scrninfo allocates the memory pointed to by pAMDGPUEnt just
before it calls amdgpu_open_drm_master, so pAMDGPUEnt->fd is always 0
here.

Signed-off-by: Michel Dänzer 
---
 src/amdgpu_probe.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 481271b..3fedfab 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -154,15 +154,6 @@ static Bool amdgpu_open_drm_master(ScrnInfoPtr 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)
 		return FALSE;
-- 
2.6.1

___
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
http://lists.x.org/mailman/listinfo/xorg-driver-ati