| Message ID | 20220906073951.2085-10-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/udl: More fixes | expand |
Hi Am 06.09.22 um 09:39 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 | 44 +++++++++++++++++++++++----------- > 2 files changed, 31 insertions(+), 21 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 de28eeff3155..df7ebe1fdc90 100644 > --- a/drivers/gpu/drm/udl/udl_main.c > +++ b/drivers/gpu/drm/udl/udl_main.c > @@ -23,6 +23,8 @@ > #define WRITES_IN_FLIGHT (20) > #define MAX_VENDOR_DESCRIPTOR_SIZE 256 > > +static struct urb *udl_get_urb_locked(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); > @@ -140,21 +142,23 @@ void udl_urb_completion(struct urb *urb) > udl->urbs.available++; > spin_unlock_irqrestore(&udl->urbs.lock, flags); > > - wake_up(&udl->urbs.sleep); > + wake_up_all(&udl->urbs.sleep); > } > > 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_locked(udl, MAX_SCHEDULE_TIMEOUT); > + udl->urbs.count--; > + spin_unlock_irq(&udl->urbs.lock); > if (WARN_ON(!urb)) > break; > unode = urb->context; > @@ -164,7 +168,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); I meant that we should maybe do a wake_up_all() here and not in the completion handler udl_urb_completion(). In any case Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > } > > static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size) > @@ -228,33 +233,44 @@ 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_locked(struct udl_device *udl, long timeout) > { > - struct udl_device *udl = to_udl(dev); > - struct urb_node *unode = NULL; > + struct urb_node *unode; > > - if (!udl->urbs.count) > - return NULL; > + assert_spin_locked(&udl->urbs.lock); > > /* 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, > + !udl->urbs.count || > !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; > } > > + if (!udl->urbs.count) > + 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_locked(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);
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 de28eeff3155..df7ebe1fdc90 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -23,6 +23,8 @@ #define WRITES_IN_FLIGHT (20) #define MAX_VENDOR_DESCRIPTOR_SIZE 256 +static struct urb *udl_get_urb_locked(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); @@ -140,21 +142,23 @@ void udl_urb_completion(struct urb *urb) udl->urbs.available++; spin_unlock_irqrestore(&udl->urbs.lock, flags); - wake_up(&udl->urbs.sleep); + wake_up_all(&udl->urbs.sleep); } 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_locked(udl, MAX_SCHEDULE_TIMEOUT); + udl->urbs.count--; + spin_unlock_irq(&udl->urbs.lock); if (WARN_ON(!urb)) break; unode = urb->context; @@ -164,7 +168,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) @@ -228,33 +233,44 @@ 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_locked(struct udl_device *udl, long timeout) { - struct udl_device *udl = to_udl(dev); - struct urb_node *unode = NULL; + struct urb_node *unode; - if (!udl->urbs.count) - return NULL; + assert_spin_locked(&udl->urbs.lock); /* 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, + !udl->urbs.count || !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; } + if (!udl->urbs.count) + 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_locked(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);
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 | 44 +++++++++++++++++++++++----------- 2 files changed, 31 insertions(+), 21 deletions(-)