Re: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
On Mon, 05 Sep 2022 10:32:55 +0200,
Thomas Zimmermann wrote:
>
> 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
> > ---
> > 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_TIMEOUTHZ
> > -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(>urbs.lock);
> > + urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> > + udl->urbs.count--;
> > + spin_unlock_irq(>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(>urbs.sleep);
>
> There's just one waiter, but it's the shutdown code. Maybe
> wake_up_all() would more clearly communicate the intention.
OK.
> > }
> > 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().
OK.
> > {
> > - struct udl_device *udl = to_udl(dev);
> > - struct urb_node *unode = NULL;
> > + struct urb_node *unode;
> > +
> > + assert_spin_locked(>urbs.lock);
> > if (!udl->urbs.count)
> > return NULL;
> > /* Wait for an in-flight buffer to complete and get re-queued
> > */
> > - spin_lock_irq(>urbs.lock);
> > if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> > !list_empty(>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()' ?
This must not happen as this function itself is the only one who takes
out the element from the list. But it's OK to add the zero
Re: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
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
---
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_TIMEOUTHZ
-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(>urbs.lock);
+ urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(>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(>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(>urbs.lock);
if (!udl->urbs.count)
return NULL;
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(>urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
!list_empty(>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(>urbs.list, struct urb_node, entry);
list_del_init(>entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(>urbs.lock);
return unode ? unode->urb : NULL;
}
[PATCH 10/12] 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().
Signed-off-by: Takashi Iwai
---
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_TIMEOUTHZ
-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(>urbs.lock);
+ urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(>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(>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(>urbs.lock);
if (!udl->urbs.count)
return NULL;
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(>urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
!list_empty(>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(>urbs.list, struct urb_node, entry);
list_del_init(>entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(>urbs.lock);
return unode ? unode->urb : NULL;
}
+#define GET_URB_TIMEOUTHZ
+struct urb *udl_get_urb(struct drm_device *dev)
+{
+ struct udl_device *udl = to_udl(dev);
+ struct urb *urb;
+
+ spin_lock_irq(>urbs.lock);
+ urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
+ spin_unlock_irq(>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
