diff mbox series

[10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()

Message ID 20220816153655.27526-11-tiwai@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/udl: More fixes | expand

Commit Message

Takashi Iwai Aug. 16, 2022, 3:36 p.m. UTC 
In the current design, udl_get_urb() may be called asynchronously
during the driver freeing its URL list via udl_free_urb_list().
The problem is that the sync is determined by comparing the urbs.count
and urbs.available fields, while we clear urbs.count field only once
after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
the state becomes inconsistent.

For fixing this inconsistency and also for hardening the locking
scheme, this patch does a slight refactoring of the code around
udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
the same spinlock at extracting a URB from the list in
udl_free_url_list().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
 drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 18 deletions(-)

Comments

Thomas Zimmermann Sept. 5, 2022, 8:32 a.m. UTC | #1 
Hi

Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> In the current design, udl_get_urb() may be called asynchronously
> during the driver freeing its URL list via udl_free_urb_list().
> The problem is that the sync is determined by comparing the urbs.count
> and urbs.available fields, while we clear urbs.count field only once
> after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> the state becomes inconsistent.
> 
> For fixing this inconsistency and also for hardening the locking
> scheme, this patch does a slight refactoring of the code around
> udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
> the same spinlock at extracting a URB from the list in
> udl_free_url_list().
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
>   drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
>   2 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 5923d2e02bc8..d943684b5bbb 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -74,13 +74,7 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
>   int udl_modeset_init(struct drm_device *dev);
>   struct drm_connector *udl_connector_init(struct drm_device *dev);
>   
> -struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
> -
> -#define GET_URB_TIMEOUT	HZ
> -static inline struct urb *udl_get_urb(struct drm_device *dev)
> -{
> -	return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
> -}
> +struct urb *udl_get_urb(struct drm_device *dev);
>   
>   int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
>   int udl_sync_pending_urbs(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 8bbb4e2861fb..19dc8317e843 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -28,6 +28,8 @@
>   static uint udl_num_urbs = WRITES_IN_FLIGHT;
>   module_param_named(numurbs, udl_num_urbs, uint, 0600);
>   
> +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
> +
>   static int udl_parse_vendor_descriptor(struct udl_device *udl)
>   {
>   	struct usb_device *udev = udl_to_usb_device(udl);
> @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
>   static void udl_free_urb_list(struct drm_device *dev)
>   {
>   	struct udl_device *udl = to_udl(dev);
> -	int count = udl->urbs.count;
>   	struct urb_node *unode;
>   	struct urb *urb;
>   
>   	DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
>   
>   	/* keep waiting and freeing, until we've got 'em all */
> -	while (count--) {
> -		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> +	while (udl->urbs.count) {
> +		spin_lock_irq(&udl->urbs.lock);
> +		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> +		udl->urbs.count--;
> +		spin_unlock_irq(&udl->urbs.lock);
>   		if (WARN_ON(!urb))
>   			break;
>   		unode = urb->context;
> @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
>   		usb_free_urb(urb);
>   		kfree(unode);
>   	}
> -	udl->urbs.count = 0;
> +
> +	wake_up(&udl->urbs.sleep);

There's just one waiter, but it's the shutdown code. Maybe wake_up_all() 
would more clearly communicate the intention.

>   }
>   
>   static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>   	return udl->urbs.count;
>   }
>   
> -struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
> +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)

I think in DRM, the correct name for this function would be 
udl_get_urb_locked().

>   {
> -	struct udl_device *udl = to_udl(dev);
> -	struct urb_node *unode = NULL;
> +	struct urb_node *unode;
> +
> +	assert_spin_locked(&udl->urbs.lock);
>   
>   	if (!udl->urbs.count)
>   		return NULL;
>   
>   	/* Wait for an in-flight buffer to complete and get re-queued */
> -	spin_lock_irq(&udl->urbs.lock);
>   	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
>   					 !list_empty(&udl->urbs.list),

The urb-free function will wake up this code, but the urb list should be 
empty then. Should we update the condition to something like 
'udl->urbs.count && !list_empty()' ?

Best regards
Thomas

>   					 udl->urbs.lock, timeout)) {
>   		DRM_INFO("wait for urb interrupted: available: %d\n",
>   			 udl->urbs.available);
> -		goto unlock;
> +		return NULL;
>   	}
>   
>   	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
>   	list_del_init(&unode->entry);
>   	udl->urbs.available--;
>   
> -unlock:
> -	spin_unlock_irq(&udl->urbs.lock);
>   	return unode ? unode->urb : NULL;
>   }
>   
> +#define GET_URB_TIMEOUT	HZ
> +struct urb *udl_get_urb(struct drm_device *dev)
> +{
> +	struct udl_device *udl = to_udl(dev);
> +	struct urb *urb;
> +
> +	spin_lock_irq(&udl->urbs.lock);
> +	urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
> +	spin_unlock_irq(&udl->urbs.lock);
> +	return urb;
> +}
> +
>   int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
>   {
>   	struct udl_device *udl = to_udl(dev);
Takashi Iwai Sept. 5, 2022, 12:56 p.m. UTC | #2 
On Mon, 05 Sep 2022 10:32:55 +0200,
Thomas Zimmermann wrote:
> 
> Hi
> 
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > In the current design, udl_get_urb() may be called asynchronously
> > during the driver freeing its URL list via udl_free_urb_list().
> > The problem is that the sync is determined by comparing the urbs.count
> > and urbs.available fields, while we clear urbs.count field only once
> > after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> > the state becomes inconsistent.
> > 
> > For fixing this inconsistency and also for hardening the locking
> > scheme, this patch does a slight refactoring of the code around
> > udl_get_urb() and udl_free_urb_list().  Now urbs.count is updated in
> > the same spinlock at extracting a URB from the list in
> > udl_free_url_list().
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >   drivers/gpu/drm/udl/udl_drv.h  |  8 +-------
> >   drivers/gpu/drm/udl/udl_main.c | 37 ++++++++++++++++++++++++----------
> >   2 files changed, 27 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index 5923d2e02bc8..d943684b5bbb 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -74,13 +74,7 @@ static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
> >   int udl_modeset_init(struct drm_device *dev);
> >   struct drm_connector *udl_connector_init(struct drm_device *dev);
> >   -struct urb *udl_get_urb_timeout(struct drm_device *dev, long
> > timeout);
> > -
> > -#define GET_URB_TIMEOUT	HZ
> > -static inline struct urb *udl_get_urb(struct drm_device *dev)
> > -{
> > -	return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
> > -}
> > +struct urb *udl_get_urb(struct drm_device *dev);
> >     int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > size_t len);
> >   int udl_sync_pending_urbs(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 8bbb4e2861fb..19dc8317e843 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -28,6 +28,8 @@
> >   static uint udl_num_urbs = WRITES_IN_FLIGHT;
> >   module_param_named(numurbs, udl_num_urbs, uint, 0600);
> >   +static struct urb *__udl_get_urb(struct udl_device *udl, long
> > timeout);
> > +
> >   static int udl_parse_vendor_descriptor(struct udl_device *udl)
> >   {
> >   	struct usb_device *udev = udl_to_usb_device(udl);
> > @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
> >   static void udl_free_urb_list(struct drm_device *dev)
> >   {
> >   	struct udl_device *udl = to_udl(dev);
> > -	int count = udl->urbs.count;
> >   	struct urb_node *unode;
> >   	struct urb *urb;
> >     	DRM_DEBUG("Waiting for completes and freeing all render
> > urbs\n");
> >     	/* keep waiting and freeing, until we've got 'em all */
> > -	while (count--) {
> > -		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> > +	while (udl->urbs.count) {
> > +		spin_lock_irq(&udl->urbs.lock);
> > +		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> > +		udl->urbs.count--;
> > +		spin_unlock_irq(&udl->urbs.lock);
> >   		if (WARN_ON(!urb))
> >   			break;
> >   		unode = urb->context;
> > @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
> >   		usb_free_urb(urb);
> >   		kfree(unode);
> >   	}
> > -	udl->urbs.count = 0;
> > +
> > +	wake_up(&udl->urbs.sleep);
> 
> There's just one waiter, but it's the shutdown code. Maybe
> wake_up_all() would more clearly communicate the intention.

OK.

> >   }
> >     static int udl_alloc_urb_list(struct drm_device *dev, int count,
> > size_t size)
> > @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
> >   	return udl->urbs.count;
> >   }
> >   -struct urb *udl_get_urb_timeout(struct drm_device *dev, long
> > timeout)
> > +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
> 
> I think in DRM, the correct name for this function would be
> udl_get_urb_locked().

OK.

> >   {
> > -	struct udl_device *udl = to_udl(dev);
> > -	struct urb_node *unode = NULL;
> > +	struct urb_node *unode;
> > +
> > +	assert_spin_locked(&udl->urbs.lock);
> >     	if (!udl->urbs.count)
> >   		return NULL;
> >     	/* Wait for an in-flight buffer to complete and get re-queued
> > */
> > -	spin_lock_irq(&udl->urbs.lock);
> >   	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >   					 !list_empty(&udl->urbs.list),
> 
> The urb-free function will wake up this code, but the urb list should
> be empty then. Should we update the condition to something like
> 'udl->urbs.count && !list_empty()' ?

This must not happen as this function itself is the only one who takes
out the element from the list.  But it's OK to add the zero check
there just to be sure...


thanks,

Takashi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 5923d2e02bc8..d943684b5bbb 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,13 +74,7 @@  static inline struct usb_device *udl_to_usb_device(struct udl_device *udl)
 int udl_modeset_init(struct drm_device *dev);
 struct drm_connector *udl_connector_init(struct drm_device *dev);
 
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
-
-#define GET_URB_TIMEOUT	HZ
-static inline struct urb *udl_get_urb(struct drm_device *dev)
-{
-	return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
-}
+struct urb *udl_get_urb(struct drm_device *dev);
 
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
 int udl_sync_pending_urbs(struct drm_device *dev);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 8bbb4e2861fb..19dc8317e843 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -28,6 +28,8 @@ 
 static uint udl_num_urbs = WRITES_IN_FLIGHT;
 module_param_named(numurbs, udl_num_urbs, uint, 0600);
 
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
+
 static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
 	struct usb_device *udev = udl_to_usb_device(udl);
@@ -151,15 +153,17 @@  void udl_urb_completion(struct urb *urb)
 static void udl_free_urb_list(struct drm_device *dev)
 {
 	struct udl_device *udl = to_udl(dev);
-	int count = udl->urbs.count;
 	struct urb_node *unode;
 	struct urb *urb;
 
 	DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
 
 	/* keep waiting and freeing, until we've got 'em all */
-	while (count--) {
-		urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+	while (udl->urbs.count) {
+		spin_lock_irq(&udl->urbs.lock);
+		urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+		udl->urbs.count--;
+		spin_unlock_irq(&udl->urbs.lock);
 		if (WARN_ON(!urb))
 			break;
 		unode = urb->context;
@@ -169,7 +173,8 @@  static void udl_free_urb_list(struct drm_device *dev)
 		usb_free_urb(urb);
 		kfree(unode);
 	}
-	udl->urbs.count = 0;
+
+	wake_up(&udl->urbs.sleep);
 }
 
 static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -233,33 +238,43 @@  static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	return udl->urbs.count;
 }
 
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
 {
-	struct udl_device *udl = to_udl(dev);
-	struct urb_node *unode = NULL;
+	struct urb_node *unode;
+
+	assert_spin_locked(&udl->urbs.lock);
 
 	if (!udl->urbs.count)
 		return NULL;
 
 	/* Wait for an in-flight buffer to complete and get re-queued */
-	spin_lock_irq(&udl->urbs.lock);
 	if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
 					 !list_empty(&udl->urbs.list),
 					 udl->urbs.lock, timeout)) {
 		DRM_INFO("wait for urb interrupted: available: %d\n",
 			 udl->urbs.available);
-		goto unlock;
+		return NULL;
 	}
 
 	unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
 	list_del_init(&unode->entry);
 	udl->urbs.available--;
 
-unlock:
-	spin_unlock_irq(&udl->urbs.lock);
 	return unode ? unode->urb : NULL;
 }
 
+#define GET_URB_TIMEOUT	HZ
+struct urb *udl_get_urb(struct drm_device *dev)
+{
+	struct udl_device *udl = to_udl(dev);
+	struct urb *urb;
+
+	spin_lock_irq(&udl->urbs.lock);
+	urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
+	spin_unlock_irq(&udl->urbs.lock);
+	return urb;
+}
+
 int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
 {
 	struct udl_device *udl = to_udl(dev);