From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bombadil.infradead.org ([198.137.202.133]:55504 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754476AbeEHKiX (ORCPT ); Tue, 8 May 2018 06:38:23 -0400 Date: Tue, 8 May 2018 07:38:13 -0300 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: linux-media@vger.kernel.org, Hans Verkuil Subject: Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd Message-ID: <20180508073813.2b121eba@vento.lan> In-Reply-To: References: <20180503145318.128315-1-hverkuil@xs4all.nl> <20180503145318.128315-5-hverkuil@xs4all.nl> <20180507140120.4f04b9bb@vento.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Tue, 8 May 2018 09:34:11 +0200 Hans Verkuil escreveu: > On 05/07/2018 07:01 PM, Mauro Carvalho Chehab wrote: > > Em Thu, 3 May 2018 16:52:54 +0200 > > Hans Verkuil escreveu: > > > >> From: Hans Verkuil > >> > >> Add media_request_get_by_fd() to find a request based on the file > >> descriptor. > >> > >> The caller has to call media_request_put() for the returned > >> request since this function increments the refcount. > >> > >> Signed-off-by: Hans Verkuil > >> --- > >> drivers/media/media-request.c | 32 ++++++++++++++++++++++++++++++++ > >> include/media/media-request.h | 24 ++++++++++++++++++++++++ > >> 2 files changed, 56 insertions(+) > >> > >> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c > >> index c216c4ab628b..edc1c3af1959 100644 > >> --- a/drivers/media/media-request.c > >> +++ b/drivers/media/media-request.c > >> @@ -218,6 +218,38 @@ static const struct file_operations request_fops = { > >> .release = media_request_close, > >> }; > >> > >> +struct media_request * > >> +media_request_get_by_fd(struct media_device *mdev, int request_fd) > >> +{ > >> + struct file *filp; > >> + struct media_request *req; > >> + > >> + if (!mdev || !mdev->ops || > >> + !mdev->ops->req_validate || !mdev->ops->req_queue) > >> + return ERR_PTR(-EPERM); > >> + > >> + filp = fget(request_fd); > >> + if (!filp) > >> + return ERR_PTR(-ENOENT); > >> + > >> + if (filp->f_op != &request_fops) > >> + goto err_fput; > >> + req = filp->private_data; > >> + if (req->mdev != mdev) > >> + goto err_fput; > >> + > >> + media_request_get(req); > > > > Hmm... this function changes the req struct (by calling kref_get) without > > holding neither the mutex or the spin lock... > > kref_get is atomic, so why would you need to add additional locking? > > Neither can the request be 'put' by another process before media_request_get() > is called due to the fget() above: while the fd has a refcount > 0 the > request refcount is also > 0. I'll add some comments to clarify this. I see. Yeah, please add a comment here. Thanks, Mauro > > > > >> + fput(filp); > >> + > >> + return req; > >> + > >> +err_fput: > >> + fput(filp); > >> + > >> + return ERR_PTR(-ENOENT); > >> +} > >> +EXPORT_SYMBOL_GPL(media_request_get_by_fd); > >> + > >> int media_request_alloc(struct media_device *mdev, > >> struct media_request_alloc *alloc) > >> { > >> diff --git a/include/media/media-request.h b/include/media/media-request.h > >> index e39122dfd717..997e096d7128 100644 > >> --- a/include/media/media-request.h > >> +++ b/include/media/media-request.h > >> @@ -86,6 +86,24 @@ static inline void media_request_get(struct media_request *req) > >> */ > >> void media_request_put(struct media_request *req); > >> > >> +/** > >> + * media_request_get_by_fd - Get a media request by fd > >> + * > >> + * @mdev: Media device this request belongs to > >> + * @request_fd: The file descriptor of the request > >> + * > >> + * Get the request represented by @request_fd that is owned > >> + * by the media device. > >> + * > >> + * Return a -EPERM error pointer if requests are not supported > >> + * by this driver. Return -ENOENT if the request was not found. > >> + * Return the pointer to the request if found: the caller will > >> + * have to call @media_request_put when it finished using the > >> + * request. > > > > ... so, it should be said here how this should be serialized, in order > > to avoid it to be destroyed by a task while some other task might be > > trying to instantiate it. > > This doesn't need any serialization. If the request_fd is found, then > it will return the request with the request refcount increased. > > I also do not understand what you mean with "some other task might be > trying to instantiate it". > > I think there is some misunderstanding here. > > Regards, > > Hans > > > > > >> + */ > >> +struct media_request * > >> +media_request_get_by_fd(struct media_device *mdev, int request_fd); > >> + > >> /** > >> * media_request_alloc - Allocate the media request > >> * > >> @@ -107,6 +125,12 @@ static inline void media_request_put(struct media_request *req) > >> { > >> } > >> > >> +static inline struct media_request * > >> +media_request_get_by_fd(struct media_device *mdev, int request_fd) > >> +{ > >> + return ERR_PTR(-EPERM); > >> +} > >> + > >> #endif > >> > >> /** > > > > > > > > Thanks, > > Mauro > > > Thanks, Mauro