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
