| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <829f05dc918a9a8a1848ecb68d175c8d7f892584.camel@collabora.com>
Date: Thu, 10 Apr 2025 14:26:39 -0400
From: Nicolas Dufresne <nicolas.dufresne@...labora.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, Mauro Carvalho Chehab
<mchehab@...nel.org>, Hans Verkuil <hverkuil@...all.nl>, Tiffany Lin
<tiffany.lin@...iatek.com>, Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
Yunfei Dong <yunfei.dong@...iatek.com>, Matthias Brugger
<matthias.bgg@...il.com>, AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
kernel@...labora.com, linux-media@...r.kernel.org, Sebastian Fricke
<sebastian.fricke@...labora.com>
Subject: Re: [PATCH v2 1/5] media: mc: add manual request completion
Hi Hans,
another question for you below.
Le jeudi 10 avril 2025 à 20:50 +0300, Laurent Pinchart a écrit :
> Hi Nicolas,
>
> Thank you for the patch.
>
> On Thu, Apr 10, 2025 at 11:39:56AM -0400, Nicolas Dufresne wrote:
> > From: Hans Verkuil <hverkuil@...all.nl>
> >
> > By default when the last request object is completed, the whole
> > request completes as well.
> >
> > But sometimes you want to manually complete a request in a driver,
> > so add a manual complete mode for this.
>
> I didn't immediately understand this was about delaying completion of
> the request. It would be nice to make that more explicit in the
> commit
> message and in the documentation of
> media_request_mark_manual_completion(). A sample use case would also
> help.
>
> > In req_queue the driver marks the request for manual completion by
> > calling media_request_mark_manual_completion, and when the driver
> > wants to manually complete the request it calls
> > media_request_manual_complete().
> >
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@...all.nl>
The CI is not happy that the Sob did not match the author, shall we
ignore the CI or edit one of the two ?
Nicolas
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.com>
> > ---
> > drivers/media/mc/mc-request.c | 38
> > ++++++++++++++++++++++++++++++++++++--
> > include/media/media-request.h | 36
> > +++++++++++++++++++++++++++++++++++-
> > 2 files changed, 71 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-
> > request.c
> > index
> > 5edfc2791ce7c7485def5db675bbf53ee223d837..398d0806d1d274eb8c454fc5c
> > 37b77476abe1e74 100644
> > --- a/drivers/media/mc/mc-request.c
> > +++ b/drivers/media/mc/mc-request.c
> > @@ -54,6 +54,7 @@ static void media_request_clean(struct
> > media_request *req)
> > req->access_count = 0;
> > WARN_ON(req->num_incomplete_objects);
> > req->num_incomplete_objects = 0;
> > + req->manual_completion = false;
> > wake_up_interruptible_all(&req->poll_wait);
> > }
> >
> > @@ -313,6 +314,7 @@ int media_request_alloc(struct media_device
> > *mdev, int *alloc_fd)
> > req->mdev = mdev;
> > req->state = MEDIA_REQUEST_STATE_IDLE;
> > req->num_incomplete_objects = 0;
> > + req->manual_completion = false;
> > kref_init(&req->kref);
> > INIT_LIST_HEAD(&req->objects);
> > spin_lock_init(&req->lock);
> > @@ -459,7 +461,7 @@ void media_request_object_unbind(struct
> > media_request_object *obj)
> >
> > req->num_incomplete_objects--;
> > if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
> > - !req->num_incomplete_objects) {
> > + !req->num_incomplete_objects && !req-
> > >manual_completion) {
> > req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > completed = true;
> > wake_up_interruptible_all(&req->poll_wait);
> > @@ -488,7 +490,7 @@ void media_request_object_complete(struct
> > media_request_object *obj)
> > WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> > goto unlock;
> >
> > - if (!--req->num_incomplete_objects) {
> > + if (!--req->num_incomplete_objects && !req-
> > >manual_completion) {
> > req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > wake_up_interruptible_all(&req->poll_wait);
> > completed = true;
> > @@ -499,3 +501,35 @@ void media_request_object_complete(struct
> > media_request_object *obj)
> > media_request_put(req);
> > }
> > EXPORT_SYMBOL_GPL(media_request_object_complete);
> > +
> > +void media_request_manual_complete(struct media_request *req)
> > +{
> > + unsigned long flags;
> > + bool completed = false;
> > +
> > + if (WARN_ON(!req))
> > + return;
> > + if (WARN_ON(!req->manual_completion))
> > + return;
> > +
> > + spin_lock_irqsave(&req->lock, flags);
> > + if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> > + goto unlock;
> > +
> > + req->manual_completion = false;
> > + /*
> > + * It is expected that all other objects in this request
> > are
> > + * completed when this function is called. WARN if that is
> > + * not the case.
> > + */
> > + if (!WARN_ON(req->num_incomplete_objects)) {
> > + req->state = MEDIA_REQUEST_STATE_COMPLETE;
> > + wake_up_interruptible_all(&req->poll_wait);
> > + completed = true;
> > + }
> > +unlock:
> > + spin_unlock_irqrestore(&req->lock, flags);
> > + if (completed)
> > + media_request_put(req);
> > +}
> > +EXPORT_SYMBOL_GPL(media_request_manual_complete);
> > diff --git a/include/media/media-request.h b/include/media/media-
> > request.h
> > index
> > d4ac557678a78372222704400c8c96cf3150b9d9..645d18907be7148ca50dcc924
> > 8ff06bd8ccdf953 100644
> > --- a/include/media/media-request.h
> > +++ b/include/media/media-request.h
> > @@ -56,6 +56,10 @@ struct media_request_object;
> > * @access_count: count the number of request accesses that are in
> > progress
> > * @objects: List of @struct media_request_object request objects
> > * @num_incomplete_objects: The number of incomplete objects in
> > the request
> > + * @manual_completion: if true, then the request won't be marked
> > as completed
> > + * when @num_incomplete_objects reaches 0. Call
> > media_request_manual_complete()
> > + * to set this field to false and complete the request
>
> I'd drop "set this field to false and " here.
>
> > + * if @num_incomplete_objects == 0.
>
> * after @num_incomplete_objects reaches 0.
>
> > * @poll_wait: Wait queue for poll
> > * @lock: Serializes access to this struct
> > */
> > @@ -68,6 +72,7 @@ struct media_request {
> > unsigned int access_count;
> > struct list_head objects;
> > unsigned int num_incomplete_objects;
> > + bool manual_completion;
> > wait_queue_head_t poll_wait;
> > spinlock_t lock;
> > };
> > @@ -218,6 +223,35 @@ media_request_get_by_fd(struct media_device
> > *mdev, int request_fd);
> > int media_request_alloc(struct media_device *mdev,
> > int *alloc_fd);
> >
> > +/**
> > + * media_request_mark_manual_completion - Set manual_completion to
> > true
> > + *
> > + * @req: The request
> > + *
> > + * Mark that the request has to be manually completed by calling
> > + * media_request_manual_complete().
> > + *
> > + * This function should be called in the req_queue callback.
>
> s/should/shall/ unless it's not a hard requirement. Any way to catch
> incorrect call patterns ?
>
> > + */
> > +static inline void
> > +media_request_mark_manual_completion(struct media_request *req)
> > +{
> > + req->manual_completion = true;
> > +}
> > +
> > +/**
> > + * media_request_manual_complete - Set manual_completion to false
>
> The main purpose of the function is to complete the request, not
> setting
> manual_completion to false.
>
> > + *
> > + * @req: The request
> > + *
> > + * Set @manual_completion to false, and if @num_incomplete_objects
> > + * is 0, then mark the request as completed.
> > + *
> > + * If there are still incomplete objects in the request, then
> > + * WARN for that since that suggests a driver error.
>
> If that's an error then I'd document it more explicitly, as the first
> sentence makes it sound that both cases are valid. Maybe
>
> * This function completes a request that was marked for manual
> completion by an
> * earlier call to media_request_mark_manual_completion(). The
> request's
> * @manual_completion flag is reset to false.
> *
> * All objects contained in the request must have been completed
> previously. It
> * is an error to call this function otherwise. The request will not
> be
> * completed in that case, and the function will WARN.
>
> > + */
> > +void media_request_manual_complete(struct media_request *req);
> > +
> > #else
> >
> > static inline void media_request_get(struct media_request *req)
> > @@ -336,7 +370,7 @@ void media_request_object_init(struct
> > media_request_object *obj);
> > * @req: The media request
> > * @ops: The object ops for this object
> > * @priv: A driver-specific priv pointer associated with this
> > object
> > - * @is_buffer: Set to true if the object a buffer object.
> > + * @is_buffer: Set to true if the object is a buffer object.
> > * @obj: The object
> > *
> > * Bind this object to the request and set the ops and priv values
> > of
--
Nicolas Dufresne
Principal Engineer at Collabora
Powered by blists - more mailing lists