media: atomisp: change copy_from_compatible to iov_iter

[RFC] media: atomisp: change copy_from_compatible to iov_iter

Posted by Joshua Crofts 2 months, 1 week ago

Since the atomisp driver concerns transferring video
data, it would probably be better to use an iov_iter
than copy_to_user calls (scatter-gather i/o and better
pointer safety). Per yesterday's emails, I'm sending an
an example of a function in atomisp_cmd.c where I've
implemented iov_iter usage.

Note, this isn't a patch, more of a question whether this
style of changing the copy_to_user calls is valid (or if
I'm writing garbage code). I'd rather get your opinion
than submitting a patch first.

Thanks for your response!

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/staging/media/atomisp/pci/atomisp_cmd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index b3e971be0e..3b987e8e8c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -15,6 +15,7 @@
 #include <linux/kfifo.h>
 #include <linux/pm_runtime.h>
 #include <linux/timer.h>
+#include <linux/uio.h>
 
 #include <asm/iosf_mbi.h>
 
@@ -1676,7 +1677,8 @@ int atomisp_3a_stat(struct atomisp_sub_device *asd, int flag,
 {
 	struct atomisp_device *isp = asd->isp;
 	struct atomisp_s3a_buf *s3a_buf;
-	unsigned long ret;
+	struct iov_iter iter;
+	int ret;
 
 	if (flag != 0)
 		return -EINVAL;
@@ -1709,13 +1711,12 @@ int atomisp_3a_stat(struct atomisp_sub_device *asd, int flag,
 	config->exp_id = s3a_buf->s3a_data->exp_id;
 	config->isp_config_id = s3a_buf->s3a_data->isp_config_id;
 
-	ret = copy_to_user(config->data, asd->params.s3a_user_stat->data,
-			   asd->params.s3a_output_bytes);
-	if (ret) {
-		dev_err(isp->dev, "copy to user failed: copied %lu bytes\n",
-			ret);
+	ret = import_ubuf(ITER_DEST, (void __user *)config->data, asd->params.s3a_output_bytes, &iter);
+	if (ret)
+		return ret;
+
+	if (copy_to_iter(asd->params.s3a_user_stat->data, asd->params.s3a_output_bytes, &iter) != asd->params.s3a_output_bytes)
 		return -EFAULT;
-	}
 
 	/* Move to free buffer list */
 	list_del_init(&s3a_buf->list);
-- 
2.47.3

Re: [RFC] media: atomisp: change copy_from_compatible to iov_iter

Posted by Andy Shevchenko 2 months, 1 week ago

On Wed, Apr 08, 2026 at 01:46:11PM +0000, Joshua Crofts wrote:
> Since the atomisp driver concerns transferring video
> data, it would probably be better to use an iov_iter
> than copy_to_user calls (scatter-gather i/o and better
> pointer safety). Per yesterday's emails, I'm sending an
> an example of a function in atomisp_cmd.c where I've
> implemented iov_iter usage.
> 
> Note, this isn't a patch, more of a question whether this
> style of changing the copy_to_user calls is valid (or if
> I'm writing garbage code). I'd rather get your opinion
> than submitting a patch first.

...

> -	ret = copy_to_user(config->data, asd->params.s3a_user_stat->data,
> -			   asd->params.s3a_output_bytes);
> -	if (ret) {
> -		dev_err(isp->dev, "copy to user failed: copied %lu bytes\n",
> -			ret);
> +	ret = import_ubuf(ITER_DEST, (void __user *)config->data, asd->params.s3a_output_bytes, &iter);
> +	if (ret)
> +		return ret;
> +
> +	if (copy_to_iter(asd->params.s3a_user_stat->data, asd->params.s3a_output_bytes, &iter) != asd->params.s3a_output_bytes)
>  		return -EFAULT;
> -	}

It doesn't change any architectural decision here. Basically you need to have
s3a_output_bytes to be already iov_iter at this point. So, import_ubuf() has to
happen somewhere else.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] media: atomisp: change copy_from_compatible to iov_iter

Posted by Andy Shevchenko 2 months, 1 week ago

On Wed, Apr 08, 2026 at 05:25:39PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 08, 2026 at 01:46:11PM +0000, Joshua Crofts wrote:
> > Since the atomisp driver concerns transferring video
> > data, it would probably be better to use an iov_iter
> > than copy_to_user calls (scatter-gather i/o and better
> > pointer safety). Per yesterday's emails, I'm sending an
> > an example of a function in atomisp_cmd.c where I've
> > implemented iov_iter usage.
> > 
> > Note, this isn't a patch, more of a question whether this
> > style of changing the copy_to_user calls is valid (or if
> > I'm writing garbage code). I'd rather get your opinion
> > than submitting a patch first.

Sure, see below.

...

> > -	ret = copy_to_user(config->data, asd->params.s3a_user_stat->data,
> > -			   asd->params.s3a_output_bytes);
> > -	if (ret) {
> > -		dev_err(isp->dev, "copy to user failed: copied %lu bytes\n",
> > -			ret);
> > +	ret = import_ubuf(ITER_DEST, (void __user *)config->data, asd->params.s3a_output_bytes, &iter);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (copy_to_iter(asd->params.s3a_user_stat->data, asd->params.s3a_output_bytes, &iter) != asd->params.s3a_output_bytes)
> >  		return -EFAULT;
> > -	}
> 
> It doesn't change any architectural decision here. Basically you need to have
> s3a_output_bytes to be already iov_iter at this point. So, import_ubuf() has to
> happen somewhere else.

Brief grepping shows this

        /*
         * Intermediate buffers used to communicate data between
         * CSS and user space.
         */
        struct ia_css_3a_statistics *s3a_user_stat;

which suggests that iov_iter should be somewhere here, in the struct
atomisp_subdev_params. But you need to read much more code and get familiar
with this. I wouldn't expect any meaningful change by a few days, maybe by
a few weeks as this driver is quite complicated.

In any case, thanks into trying with this, will be very appreciated
when the task is logically finished.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC] media: atomisp: change copy_from_compatible to iov_iter

Posted by Joshua Crofts 2 months, 1 week ago

On Wed, 8 Apr 2026 at 16:29, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> > It doesn't change any architectural decision here. Basically you need to have
> > s3a_output_bytes to be already iov_iter at this point. So, import_ubuf() has to
> > happen somewhere else.

Yes, I know it's not an architectural change, more of a temporary
band-aid for better pointer
management, but fair, pointless for now.

> which suggests that iov_iter should be somewhere here, in the struct
> atomisp_subdev_params. But you need to read much more code and get familiar
> with this. I wouldn't expect any meaningful change by a few days, maybe by
> a few weeks as this driver is quite complicated.

I agree, the codebase is complicated, I definitely won't send any
changes at the moment
(unless they would be checkpatch.pl changes, which are low priority
ofc). However, the
iov_iter problem is intriguing and I'll surely spend some time working on it.

> In any case, thanks into trying with this, will be very appreciated
> when the task is logically finished.

Not promising anything. Thanks for your response!

-- 
Kind regards

CJD

Re: [RFC] media: atomisp: change copy_from_compatible to iov_iter

Posted by Joshua Crofts 2 months, 1 week ago

On Wed, 8 Apr 2026 at 15:46, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> Since the atomisp driver concerns transferring video
> data, it would probably be better to use an iov_iter
> than copy_to_user calls (scatter-gather i/o and better
> pointer safety). Per yesterday's emails, I'm sending an
> an example of a function in atomisp_cmd.c where I've
> implemented iov_iter usage.
>
> Note, this isn't a patch, more of a question whether this
> style of changing the copy_to_user calls is valid (or if
> I'm writing garbage code). I'd rather get your opinion
> than submitting a patch first.
>
Before I forget, this change was compile-tested.

-- 
Kind regards

CJD