From: Takashi Iwai <tiwai@suse.de> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list() Date: Tue, 16 Aug 2022 17:36:53 +0200 [thread overview] Message-ID: <20220816153655.27526-11-tiwai@suse.de> (raw) In-Reply-To: <20220816153655.27526-1-tiwai@suse.de> 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); } 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); -- 2.35.3
WARNING: multiple messages have this Message-ID (diff)
From: Takashi Iwai <tiwai@suse.de> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list() Date: Tue, 16 Aug 2022 17:36:53 +0200 [thread overview] Message-ID: <20220816153655.27526-11-tiwai@suse.de> (raw) In-Reply-To: <20220816153655.27526-1-tiwai@suse.de> 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); } 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); -- 2.35.3
next prev parent reply other threads:[~2022-08-16 15:38 UTC|newest] Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-16 15:36 [PATCH 00/12] drm/udl: More fixes Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-08-16 15:36 ` [PATCH 01/12] drm/udl: Restore display mode on resume Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-06 20:06 ` Daniel Vetter 2022-09-06 20:06 ` Daniel Vetter 2022-09-07 5:51 ` Takashi Iwai 2022-09-07 5:51 ` Takashi Iwai 2022-09-07 17:01 ` Daniel Vetter 2022-09-07 17:01 ` Daniel Vetter 2022-08-16 15:36 ` [PATCH 02/12] drm/udl: Add reset_resume Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-08-16 15:36 ` [PATCH 03/12] drm/udl: Enable damage clipping Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-08-16 15:36 ` [PATCH 04/12] Revert "drm/udl: Kill pending URBs at suspend and disconnect" Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:07 ` Thomas Zimmermann 2022-08-16 15:36 ` [PATCH 05/12] drm/udl: Suppress error print for -EPROTO at URB completion Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-08-16 15:36 ` [PATCH 06/12] drm/udl: Increase the default URB list size to 20 Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:08 ` Thomas Zimmermann 2022-09-05 8:08 ` Thomas Zimmermann 2022-08-16 15:36 ` [PATCH 07/12] drm/udl: Add parameter to set number of URBs Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:09 ` Thomas Zimmermann 2022-09-05 8:09 ` Thomas Zimmermann 2022-08-16 15:36 ` [PATCH 08/12] drm/udl: Drop unneeded alignment Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:40 ` Thomas Zimmermann 2022-09-05 8:40 ` Thomas Zimmermann 2022-09-05 12:50 ` Takashi Iwai 2022-09-05 12:50 ` Takashi Iwai 2022-08-16 15:36 ` [PATCH 09/12] drm/udl: Fix potential URB leaks Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:16 ` Thomas Zimmermann 2022-08-16 15:36 ` Takashi Iwai [this message] 2022-08-16 15:36 ` [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list() Takashi Iwai 2022-09-05 8:32 ` Thomas Zimmermann 2022-09-05 8:32 ` Thomas Zimmermann 2022-09-05 12:56 ` Takashi Iwai 2022-09-05 12:56 ` Takashi Iwai 2022-08-16 15:36 ` [PATCH 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:42 ` Thomas Zimmermann 2022-09-05 8:42 ` Thomas Zimmermann 2022-08-16 15:36 ` [PATCH 12/12] drm/udl: Sync pending URBs at the end of suspend Takashi Iwai 2022-08-16 15:36 ` Takashi Iwai 2022-09-05 8:44 ` Thomas Zimmermann 2022-09-05 13:00 ` Takashi Iwai 2022-09-05 13:00 ` Takashi Iwai
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220816153655.27526-11-tiwai@suse.de \ --to=tiwai@suse.de \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.