Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd
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
Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd
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.
>
>> +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
>
Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd
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...
> + 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.
> + */
> +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
Re: [PATCHv13 04/28] media-request: add media_request_get_by_fd
On Thu, May 03, 2018 at 04:52:54PM +0200, Hans Verkuil wrote: > 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 Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi
[PATCHv13 04/28] media-request: add media_request_get_by_fd
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);
+ 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.
+ */
+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
/**
--
2.17.0
