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

[Thomas Zimmermann]

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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature