drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
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(). Acked-by: Thomas Zimmermann <tzimmermann@suse.de> Signed-off-by: Takashi Iwai <tiwai@suse.de> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Link: https://patchwork.freedesktop.org/patch/msgid/20220908095115.23396-11-tiwai@suse.de
This commit is contained in:
parent
046f4f0af7
commit
c5c354a3a4
@ -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);
|
||||
|
@ -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);
|
||||
@ -146,15 +148,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_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_all(&udl->urbs.sleep);
|
||||
}
|
||||
|
||||
static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
|
||||
@ -228,33 +233,44 @@ retry:
|
||||
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);
|
||||
|
Loading…
x
Reference in New Issue
Block a user