Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 05 Sep 2022 14:56:39 +0200
Message-ID: <87k06ij7y0.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Thomas Zimmermann <tzimmermann@suse.de>
Cc:     Takashi Iwai <tiwai@suse.de>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
In-Reply-To: <5730ea32-caea-03db-c37c-658484c2f8f0@suse.de>
References: <20220816153655.27526-1-tiwai@suse.de>
        <20220816153655.27526-11-tiwai@suse.de>
        <5730ea32-caea-03db-c37c-658484c2f8f0@suse.de>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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 <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);
> 
> 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(&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()' ?

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 check
there just to be sure...


thanks,

Takashi