drm/radeon: Handle workqueue allocation failure

Submitted by Will Deacon on Oct. 25, 2019, 11:04 a.m.

Details

Message ID 20191025110450.10474-1-will@kernel.org
State New
Headers show
Series "drm/radeon: Handle workqueue allocation failure" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Will Deacon Oct. 25, 2019, 11:04 a.m.
In the highly unlikely event that we fail to allocate the "radeon-crtc"
workqueue, we should bail cleanly rather than blindly march on with a
NULL pointer installed for the 'flip_queue' field of the 'radeon_crtc'
structure.

This was reported previously by Nicolas, but I don't think his fix was
correct:

Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Reported-by: Nicolas Waisman <nico@semmle.com>
Link: https://lore.kernel.org/lkml/CADJ_3a8WFrs5NouXNqS5WYe7rebFP+_A5CheeqAyD_p7DFJJcg@mail.gmail.com/
Signed-off-by: Will Deacon <will@kernel.org>
---

Compile-tested only.

 drivers/gpu/drm/radeon/radeon_display.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index e81b01f8db90..3e4ef1380fca 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -672,17 +672,25 @@  static void radeon_crtc_init(struct drm_device *dev, int index)
 {
 	struct radeon_device *rdev = dev->dev_private;
 	struct radeon_crtc *radeon_crtc;
+	struct workqueue_struct *wq;
 	int i;
 
 	radeon_crtc = kzalloc(sizeof(struct radeon_crtc) + (RADEONFB_CONN_LIMIT * sizeof(struct drm_connector *)), GFP_KERNEL);
 	if (radeon_crtc == NULL)
 		return;
 
+	wq = alloc_workqueue("radeon-crtc", WQ_HIGHPRI, 0);
+	if (unlikely(!wq)) {
+		kfree(radeon_crtc);
+		return;
+	}
+
 	drm_crtc_init(dev, &radeon_crtc->base, &radeon_crtc_funcs);
 
 	drm_mode_crtc_set_gamma_size(&radeon_crtc->base, 256);
 	radeon_crtc->crtc_id = index;
-	radeon_crtc->flip_queue = alloc_workqueue("radeon-crtc", WQ_HIGHPRI, 0);
+	radeon_crtc->flip_queue = wq;
+
 	rdev->mode_info.crtcs[index] = radeon_crtc;
 
 	if (rdev->family >= CHIP_BONAIRE) {

Comments

On 2019-10-25 1:04 p.m., Will Deacon wrote:
> In the highly unlikely event that we fail to allocate the "radeon-crtc"
> workqueue, we should bail cleanly rather than blindly march on with a
> NULL pointer installed for the 'flip_queue' field of the 'radeon_crtc'
> structure.
> 
> This was reported previously by Nicolas, but I don't think his fix was
> correct:

Neither is this one I'm afraid. See my feedback on
https://patchwork.freedesktop.org/patch/331534/ .
On Fri, Oct 25, 2019 at 06:06:26PM +0200, Michel Dänzer wrote:
> On 2019-10-25 1:04 p.m., Will Deacon wrote:
> > In the highly unlikely event that we fail to allocate the "radeon-crtc"
> > workqueue, we should bail cleanly rather than blindly march on with a
> > NULL pointer installed for the 'flip_queue' field of the 'radeon_crtc'
> > structure.
> > 
> > This was reported previously by Nicolas, but I don't think his fix was
> > correct:
> 
> Neither is this one I'm afraid. See my feedback on
> https://patchwork.freedesktop.org/patch/331534/ .

Thanks. Although I agree with you wrt the original patch, I don't think
the workqueue allocation failure is distinguishable from the CRTC allocation
failure with my patch. Are you saying that the error path there is broken
too?

Will
On 2019-10-25 6:18 p.m., Will Deacon wrote:
> On Fri, Oct 25, 2019 at 06:06:26PM +0200, Michel Dänzer wrote:
>> On 2019-10-25 1:04 p.m., Will Deacon wrote:
>>> In the highly unlikely event that we fail to allocate the "radeon-crtc"
>>> workqueue, we should bail cleanly rather than blindly march on with a
>>> NULL pointer installed for the 'flip_queue' field of the 'radeon_crtc'
>>> structure.
>>>
>>> This was reported previously by Nicolas, but I don't think his fix was
>>> correct:
>>
>> Neither is this one I'm afraid. See my feedback on
>> https://patchwork.freedesktop.org/patch/331534/ .
> 
> Thanks. Although I agree with you wrt the original patch, I don't think
> the workqueue allocation failure is distinguishable from the CRTC allocation
> failure with my patch. Are you saying that the error path there is broken
> too?

The driver won't actually work if radeon_crtc_init bails silently, it'll
just fall over at some later point.