[PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information.

When pin_user_pages() returns numbers of partially mapped pages,
those pages were not unpinned as part of error handling. Fixed
it as part of this patch.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
        https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
---
Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.

 drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8975346..29bab13 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	u64 card_addr;
 	u64 dma_addr;
 	u64 user_ctl;
+	int nr_pages = 0;
 
 	ldev = priv->ldev;
 
@@ -76,13 +77,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 
 	// Lock the user buffer pages in memory, and hold on to the page pointers (for the sglist)
 	mmap_read_lock(current->mm);      /*  get memory map semaphore */
-	rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE | FOLL_GET, acd->user_pages, NULL);
+	rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, acd->user_pages, NULL);
 	mmap_read_unlock(current->mm);        /*  release the semaphore */
 	if (rv != acd->page_count) {
-		dev_err(&priv->ldev->pldev->dev, "Couldn't get_user_pages (%ld)\n", rv);
+		dev_err(&priv->ldev->pldev->dev, "Couldn't pin_user_pages (%ld)\n", rv);
+		nr_pages = rv;
 		goto err_get_user_pages;
 	}
 
+	nr_pages = acd->page_count;
 	// Allocate and setup the sg_table (scatterlist entries)
 	rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
 	if (rv) {
@@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	sg_free_table(&acd->sgt);
  err_dma_map_sg:
  err_alloc_sg_table:
-	for (i = 0 ; i < acd->page_count ; i++)
-		put_page(acd->user_pages[i]);
-
  err_get_user_pages:
+	if (nr_pages > 0)
+		unpin_user_pages(acd->user_pages, nr_pages);
 	kfree(acd->user_pages);
  err_alloc_userpages:
 	kfree(acd);
@@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
 
 	dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
 
-	for (i = 0 ; i < acd->page_count ; i++)
-		put_page(acd->user_pages[i]);
+	unpin_user_pages(acd->user_pages, acd->page_count);
 
 	sg_free_table(&acd->sgt);
 
-- 
1.9.1

[PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information.

When pin_user_pages() returns numbers of partially mapped pages,
those pages were not unpinned as part of error handling. Fixed
it as part of this patch.

[1] Documentation/core-api/pin_user_pages.rst

[2] "Explicit pinning of user-space pages":
        https://lwn.net/Articles/807108/

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Cc: John Hubbard <jhubbard@nvidia.com>
---
Hi,

I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.

 drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
index 8975346..29bab13 100644
--- a/drivers/staging/kpc2000/kpc_dma/fileops.c
+++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
@@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	u64 card_addr;
 	u64 dma_addr;
 	u64 user_ctl;
+	int nr_pages = 0;
 
 	ldev = priv->ldev;
 
@@ -76,13 +77,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 
 	// Lock the user buffer pages in memory, and hold on to the page pointers (for the sglist)
 	mmap_read_lock(current->mm);      /*  get memory map semaphore */
-	rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE | FOLL_GET, acd->user_pages, NULL);
+	rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, acd->user_pages, NULL);
 	mmap_read_unlock(current->mm);        /*  release the semaphore */
 	if (rv != acd->page_count) {
-		dev_err(&priv->ldev->pldev->dev, "Couldn't get_user_pages (%ld)\n", rv);
+		dev_err(&priv->ldev->pldev->dev, "Couldn't pin_user_pages (%ld)\n", rv);
+		nr_pages = rv;
 		goto err_get_user_pages;
 	}
 
+	nr_pages = acd->page_count;
 	// Allocate and setup the sg_table (scatterlist entries)
 	rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
 	if (rv) {
@@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
 	sg_free_table(&acd->sgt);
  err_dma_map_sg:
  err_alloc_sg_table:
-	for (i = 0 ; i < acd->page_count ; i++)
-		put_page(acd->user_pages[i]);
-
  err_get_user_pages:
+	if (nr_pages > 0)
+		unpin_user_pages(acd->user_pages, nr_pages);
 	kfree(acd->user_pages);
  err_alloc_userpages:
 	kfree(acd);
@@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
 
 	dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
 
-	for (i = 0 ; i < acd->page_count ; i++)
-		put_page(acd->user_pages[i]);
+	unpin_user_pages(acd->user_pages, acd->page_count);
 
 	sg_free_table(&acd->sgt);
 
-- 
1.9.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-05-31 10:51, Souptick Joarder wrote:
> In 2019, we introduced pin_user_pages*() and now we are converting
> get_user_pages*() to the new API as appropriate. [1] & [2] could
> be referred for more information.
> 
> When pin_user_pages() returns numbers of partially mapped pages,
> those pages were not unpinned as part of error handling. Fixed
> it as part of this patch.
> 

Hi Souptick,

btw, Bharath (+cc) attempted to do the "put" side of this, last year.
That got as far as a v4 patch [1], and then I asked him to let me put
it into my tree. But then it didn't directly apply anymore after the
whole design moved to pin+unpin, and so here we are now.


If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).

Anyway, I'd recommend splitting the bug fix(es) into it at least one
separate patch. That's a "best practice", and I don't see any reason
not to do it here, even though the bugs are not huge.

Also I think there may be more than one bug to fix, because I just
noticed that the pre-existing code is doing set_page_dirty(), when
it should be doing set_page_dirty_lock(). See below.


> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
>          https://lwn.net/Articles/807108/
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> ---
> Hi,
> 
> I'm compile tested this, but unable to run-time test, so any testing
> help is much appriciated.
> 
>   drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> index 8975346..29bab13 100644
> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> @@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>   	u64 card_addr;
>   	u64 dma_addr;
>   	u64 user_ctl;
> +	int nr_pages = 0;

Probably best to correct the "rv" type as well: it should be an int, rather
than a long.

>   
>   	ldev = priv->ldev;
>   
> @@ -76,13 +77,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>   
>   	// Lock the user buffer pages in memory, and hold on to the page pointers (for the sglist)
>   	mmap_read_lock(current->mm);      /*  get memory map semaphore */
> -	rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE | FOLL_GET, acd->user_pages, NULL);
> +	rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, acd->user_pages, NULL);
>   	mmap_read_unlock(current->mm);        /*  release the semaphore */
>   	if (rv != acd->page_count) {
> -		dev_err(&priv->ldev->pldev->dev, "Couldn't get_user_pages (%ld)\n", rv);
> +		dev_err(&priv->ldev->pldev->dev, "Couldn't pin_user_pages (%ld)\n", rv);
> +		nr_pages = rv;
>   		goto err_get_user_pages;
>   	}
>   
> +	nr_pages = acd->page_count;
>   	// Allocate and setup the sg_table (scatterlist entries)
>   	rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
>   	if (rv) {
> @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>   	sg_free_table(&acd->sgt);
>    err_dma_map_sg:
>    err_alloc_sg_table:

So now we end up with two unnecessary labels. Probably best to delete two of these
three and name the remaining one appropriately:

  err_dma_map_sg:
  err_alloc_sg_table:
  err_get_user_pages:

> -	for (i = 0 ; i < acd->page_count ; i++)
> -		put_page(acd->user_pages[i]);
> -
>    err_get_user_pages:
> +	if (nr_pages > 0)
> +		unpin_user_pages(acd->user_pages, nr_pages);
>   	kfree(acd->user_pages);
>    err_alloc_userpages:
>   	kfree(acd);
> @@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
>   

There is code up here (not shown in this diff), that does a set_page_dirty().
First of all, that should be set_page_dirty_lock(), and second, maybe (or maybe not)
it can all be done after the dma_unmap_sg(), at the same time as the unpin, via
unpin_user_pages_dirty_lock(). In fact, it's misleading at best to leave those
pages mapped, because there is an interval in there after set_page_dirty() and
before put_page(), in which the device could be running and setting pages dirty.
(Remember that writeback attempts can be happening concurrently with all of this,
and writeback is deeply involved with page dirtiness.)

I remember Bharath wrestled with this in an earlier conversion attempt (back when
we were only converting the "put_page" side of things), let me see if I can dig up
that email thread for some guidance...OK, in [1] it appears that everyone
finally settled on keeping the PageReserved check, but OK to move everything below
the dma_unmap_sg() call.

[1] https://lore.kernel.org/r/20190720173214.GA4250@bharath12345-Inspiron-5559


>   	dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
>   
> -	for (i = 0 ; i < acd->page_count ; i++)
> -		put_page(acd->user_pages[i]);
> +	unpin_user_pages(acd->user_pages, acd->page_count);
>   
>   	sg_free_table(&acd->sgt);
>   
> 

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-05-31 10:51, Souptick Joarder wrote:
> In 2019, we introduced pin_user_pages*() and now we are converting
> get_user_pages*() to the new API as appropriate. [1] & [2] could
> be referred for more information.
> 
> When pin_user_pages() returns numbers of partially mapped pages,
> those pages were not unpinned as part of error handling. Fixed
> it as part of this patch.
> 

Hi Souptick,

btw, Bharath (+cc) attempted to do the "put" side of this, last year.
That got as far as a v4 patch [1], and then I asked him to let me put
it into my tree. But then it didn't directly apply anymore after the
whole design moved to pin+unpin, and so here we are now.


If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).

Anyway, I'd recommend splitting the bug fix(es) into it at least one
separate patch. That's a "best practice", and I don't see any reason
not to do it here, even though the bugs are not huge.

Also I think there may be more than one bug to fix, because I just
noticed that the pre-existing code is doing set_page_dirty(), when
it should be doing set_page_dirty_lock(). See below.


> [1] Documentation/core-api/pin_user_pages.rst
> 
> [2] "Explicit pinning of user-space pages":
>          https://lwn.net/Articles/807108/
> 
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> ---
> Hi,
> 
> I'm compile tested this, but unable to run-time test, so any testing
> help is much appriciated.
> 
>   drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> index 8975346..29bab13 100644
> --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> @@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>   	u64 card_addr;
>   	u64 dma_addr;
>   	u64 user_ctl;
> +	int nr_pages = 0;

Probably best to correct the "rv" type as well: it should be an int, rather
than a long.

>   
>   	ldev = priv->ldev;
>   
> @@ -76,13 +77,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>   
>   	// Lock the user buffer pages in memory, and hold on to the page pointers (for the sglist)
>   	mmap_read_lock(current->mm);      /*  get memory map semaphore */
> -	rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE | FOLL_GET, acd->user_pages, NULL);
> +	rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, acd->user_pages, NULL);
>   	mmap_read_unlock(current->mm);        /*  release the semaphore */
>   	if (rv != acd->page_count) {
> -		dev_err(&priv->ldev->pldev->dev, "Couldn't get_user_pages (%ld)\n", rv);
> +		dev_err(&priv->ldev->pldev->dev, "Couldn't pin_user_pages (%ld)\n", rv);
> +		nr_pages = rv;
>   		goto err_get_user_pages;
>   	}
>   
> +	nr_pages = acd->page_count;
>   	// Allocate and setup the sg_table (scatterlist entries)
>   	rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
>   	if (rv) {
> @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
>   	sg_free_table(&acd->sgt);
>    err_dma_map_sg:
>    err_alloc_sg_table:

So now we end up with two unnecessary labels. Probably best to delete two of these
three and name the remaining one appropriately:

  err_dma_map_sg:
  err_alloc_sg_table:
  err_get_user_pages:

> -	for (i = 0 ; i < acd->page_count ; i++)
> -		put_page(acd->user_pages[i]);
> -
>    err_get_user_pages:
> +	if (nr_pages > 0)
> +		unpin_user_pages(acd->user_pages, nr_pages);
>   	kfree(acd->user_pages);
>    err_alloc_userpages:
>   	kfree(acd);
> @@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
>   

There is code up here (not shown in this diff), that does a set_page_dirty().
First of all, that should be set_page_dirty_lock(), and second, maybe (or maybe not)
it can all be done after the dma_unmap_sg(), at the same time as the unpin, via
unpin_user_pages_dirty_lock(). In fact, it's misleading at best to leave those
pages mapped, because there is an interval in there after set_page_dirty() and
before put_page(), in which the device could be running and setting pages dirty.
(Remember that writeback attempts can be happening concurrently with all of this,
and writeback is deeply involved with page dirtiness.)

I remember Bharath wrestled with this in an earlier conversion attempt (back when
we were only converting the "put_page" side of things), let me see if I can dig up
that email thread for some guidance...OK, in [1] it appears that everyone
finally settled on keeping the PageReserved check, but OK to move everything below
the dma_unmap_sg() call.

[1] https://lore.kernel.org/r/20190720173214.GA4250@bharath12345-Inspiron-5559


>   	dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
>   
> -	for (i = 0 ; i < acd->page_count ; i++)
> -		put_page(acd->user_pages[i]);
> +	unpin_user_pages(acd->user_pages, acd->page_count);
>   
>   	sg_free_table(&acd->sgt);
>   
> 

thanks,
-- 
John Hubbard
NVIDIA
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-05-31 10:51, Souptick Joarder wrote:
> > In 2019, we introduced pin_user_pages*() and now we are converting
> > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > be referred for more information.
> >
> > When pin_user_pages() returns numbers of partially mapped pages,
> > those pages were not unpinned as part of error handling. Fixed
> > it as part of this patch.
> >
>
> Hi Souptick,
>
> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
> That got as far as a v4 patch [1], and then I asked him to let me put
> it into my tree. But then it didn't directly apply anymore after the
> whole design moved to pin+unpin, and so here we are now.
>
>
> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
> tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).

Sure, will add him as *Co-Developed-by*
>
> Anyway, I'd recommend splitting the bug fix(es) into it at least one
> separate patch. That's a "best practice", and I don't see any reason
> not to do it here, even though the bugs are not huge.
>
> Also I think there may be more than one bug to fix, because I just
> noticed that the pre-existing code is doing set_page_dirty(), when
> it should be doing set_page_dirty_lock(). See below.
>
>
> > [1] Documentation/core-api/pin_user_pages.rst
> >
> > [2] "Explicit pinning of user-space pages":
> >          https://lwn.net/Articles/807108/
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > ---
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> >
> >   drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ++++++++-------
> >   1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > index 8975346..29bab13 100644
> > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > @@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >       u64 card_addr;
> >       u64 dma_addr;
> >       u64 user_ctl;
> > +     int nr_pages = 0;
>
> Probably best to correct the "rv" type as well: it should be an int, rather
> than a long.

Noted.

>
> >
> >       ldev = priv->ldev;
> >
> > @@ -76,13 +77,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >
> >       // Lock the user buffer pages in memory, and hold on to the page pointers (for the sglist)
> >       mmap_read_lock(current->mm);      /*  get memory map semaphore */
> > -     rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE | FOLL_GET, acd->user_pages, NULL);
> > +     rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, acd->user_pages, NULL);
> >       mmap_read_unlock(current->mm);        /*  release the semaphore */
> >       if (rv != acd->page_count) {
> > -             dev_err(&priv->ldev->pldev->dev, "Couldn't get_user_pages (%ld)\n", rv);
> > +             dev_err(&priv->ldev->pldev->dev, "Couldn't pin_user_pages (%ld)\n", rv);
> > +             nr_pages = rv;
> >               goto err_get_user_pages;
> >       }
> >
> > +     nr_pages = acd->page_count;
> >       // Allocate and setup the sg_table (scatterlist entries)
> >       rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
> >       if (rv) {
> > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >       sg_free_table(&acd->sgt);
> >    err_dma_map_sg:
> >    err_alloc_sg_table:
>
> So now we end up with two unnecessary labels. Probably best to delete two of these
> three and name the remaining one appropriately:

Hmm, I thought about it. But later decided to wait for review comments
on the same in v1.
I will remove it now.

>
>   err_dma_map_sg:
>   err_alloc_sg_table:
>   err_get_user_pages:
>
> > -     for (i = 0 ; i < acd->page_count ; i++)
> > -             put_page(acd->user_pages[i]);
> > -
> >    err_get_user_pages:
> > +     if (nr_pages > 0)
> > +             unpin_user_pages(acd->user_pages, nr_pages);
> >       kfree(acd->user_pages);
> >    err_alloc_userpages:
> >       kfree(acd);
> > @@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> >
>
> There is code up here (not shown in this diff), that does a set_page_dirty().
> First of all, that should be set_page_dirty_lock(), and second, maybe (or maybe not)
> it can all be done after the dma_unmap_sg(), at the same time as the unpin, via
> unpin_user_pages_dirty_lock(). In fact, it's misleading at best to leave those
> pages mapped, because there is an interval in there after set_page_dirty() and
> before put_page(), in which the device could be running and setting pages dirty.
> (Remember that writeback attempts can be happening concurrently with all of this,
> and writeback is deeply involved with page dirtiness.)
>
> I remember Bharath wrestled with this in an earlier conversion attempt (back when
> we were only converting the "put_page" side of things), let me see if I can dig up
> that email thread for some guidance...OK, in [1] it appears that everyone
> finally settled on keeping the PageReserved check, but OK to move everything below
> the dma_unmap_sg() call.
>
> [1] https://lore.kernel.org/r/20190720173214.GA4250@bharath12345-Inspiron-5559

Well, I need to rework on this based on the above feedback and
suggestions. Will post the
new series.

>
>
> >       dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
> >
> > -     for (i = 0 ; i < acd->page_count ; i++)
> > -             put_page(acd->user_pages[i]);
> > +     unpin_user_pages(acd->user_pages, acd->page_count);
> >
> >       sg_free_table(&acd->sgt);
> >
> >
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2020-05-31 10:51, Souptick Joarder wrote:
> > In 2019, we introduced pin_user_pages*() and now we are converting
> > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > be referred for more information.
> >
> > When pin_user_pages() returns numbers of partially mapped pages,
> > those pages were not unpinned as part of error handling. Fixed
> > it as part of this patch.
> >
>
> Hi Souptick,
>
> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
> That got as far as a v4 patch [1], and then I asked him to let me put
> it into my tree. But then it didn't directly apply anymore after the
> whole design moved to pin+unpin, and so here we are now.
>
>
> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
> tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).

Sure, will add him as *Co-Developed-by*
>
> Anyway, I'd recommend splitting the bug fix(es) into it at least one
> separate patch. That's a "best practice", and I don't see any reason
> not to do it here, even though the bugs are not huge.
>
> Also I think there may be more than one bug to fix, because I just
> noticed that the pre-existing code is doing set_page_dirty(), when
> it should be doing set_page_dirty_lock(). See below.
>
>
> > [1] Documentation/core-api/pin_user_pages.rst
> >
> > [2] "Explicit pinning of user-space pages":
> >          https://lwn.net/Articles/807108/
> >
> > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > ---
> > Hi,
> >
> > I'm compile tested this, but unable to run-time test, so any testing
> > help is much appriciated.
> >
> >   drivers/staging/kpc2000/kpc_dma/fileops.c | 15 ++++++++-------
> >   1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > index 8975346..29bab13 100644
> > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c
> > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c
> > @@ -48,6 +48,7 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >       u64 card_addr;
> >       u64 dma_addr;
> >       u64 user_ctl;
> > +     int nr_pages = 0;
>
> Probably best to correct the "rv" type as well: it should be an int, rather
> than a long.

Noted.

>
> >
> >       ldev = priv->ldev;
> >
> > @@ -76,13 +77,15 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >
> >       // Lock the user buffer pages in memory, and hold on to the page pointers (for the sglist)
> >       mmap_read_lock(current->mm);      /*  get memory map semaphore */
> > -     rv = get_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE | FOLL_GET, acd->user_pages, NULL);
> > +     rv = pin_user_pages(iov_base, acd->page_count, FOLL_TOUCH | FOLL_WRITE, acd->user_pages, NULL);
> >       mmap_read_unlock(current->mm);        /*  release the semaphore */
> >       if (rv != acd->page_count) {
> > -             dev_err(&priv->ldev->pldev->dev, "Couldn't get_user_pages (%ld)\n", rv);
> > +             dev_err(&priv->ldev->pldev->dev, "Couldn't pin_user_pages (%ld)\n", rv);
> > +             nr_pages = rv;
> >               goto err_get_user_pages;
> >       }
> >
> > +     nr_pages = acd->page_count;
> >       // Allocate and setup the sg_table (scatterlist entries)
> >       rv = sg_alloc_table_from_pages(&acd->sgt, acd->user_pages, acd->page_count, iov_base & (PAGE_SIZE - 1), iov_len, GFP_KERNEL);
> >       if (rv) {
> > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> >       sg_free_table(&acd->sgt);
> >    err_dma_map_sg:
> >    err_alloc_sg_table:
>
> So now we end up with two unnecessary labels. Probably best to delete two of these
> three and name the remaining one appropriately:

Hmm, I thought about it. But later decided to wait for review comments
on the same in v1.
I will remove it now.

>
>   err_dma_map_sg:
>   err_alloc_sg_table:
>   err_get_user_pages:
>
> > -     for (i = 0 ; i < acd->page_count ; i++)
> > -             put_page(acd->user_pages[i]);
> > -
> >    err_get_user_pages:
> > +     if (nr_pages > 0)
> > +             unpin_user_pages(acd->user_pages, nr_pages);
> >       kfree(acd->user_pages);
> >    err_alloc_userpages:
> >       kfree(acd);
> > @@ -217,8 +219,7 @@ void  transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags)
> >
>
> There is code up here (not shown in this diff), that does a set_page_dirty().
> First of all, that should be set_page_dirty_lock(), and second, maybe (or maybe not)
> it can all be done after the dma_unmap_sg(), at the same time as the unpin, via
> unpin_user_pages_dirty_lock(). In fact, it's misleading at best to leave those
> pages mapped, because there is an interval in there after set_page_dirty() and
> before put_page(), in which the device could be running and setting pages dirty.
> (Remember that writeback attempts can be happening concurrently with all of this,
> and writeback is deeply involved with page dirtiness.)
>
> I remember Bharath wrestled with this in an earlier conversion attempt (back when
> we were only converting the "put_page" side of things), let me see if I can dig up
> that email thread for some guidance...OK, in [1] it appears that everyone
> finally settled on keeping the PageReserved check, but OK to move everything below
> the dma_unmap_sg() call.
>
> [1] https://lore.kernel.org/r/20190720173214.GA4250@bharath12345-Inspiron-5559

Well, I need to rework on this based on the above feedback and
suggestions. Will post the
new series.

>
>
> >       dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir);
> >
> > -     for (i = 0 ; i < acd->page_count ; i++)
> > -             put_page(acd->user_pages[i]);
> > +     unpin_user_pages(acd->user_pages, acd->page_count);
> >
> >       sg_free_table(&acd->sgt);
> >
> >
>
> thanks,
> --
> John Hubbard
> NVIDIA
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-06-08 12:01, Souptick Joarder wrote:
> On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 2020-05-31 10:51, Souptick Joarder wrote:
>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>> be referred for more information.
>>>
>>> When pin_user_pages() returns numbers of partially mapped pages,
>>> those pages were not unpinned as part of error handling. Fixed
>>> it as part of this patch.
>>>
>>
>> Hi Souptick,
>>
>> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
>> That got as far as a v4 patch [1], and then I asked him to let me put
>> it into my tree. But then it didn't directly apply anymore after the
>> whole design moved to pin+unpin, and so here we are now.
>>
>>
>> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
>> tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).
> 
> Sure, will add him as *Co-Developed-by*
>>


Yes, but it's best to wait and see if he responds, before adding that tag, because
it also required a Signed-off-by from him.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-06-08 12:01, Souptick Joarder wrote:
> On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>
>> On 2020-05-31 10:51, Souptick Joarder wrote:
>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>> be referred for more information.
>>>
>>> When pin_user_pages() returns numbers of partially mapped pages,
>>> those pages were not unpinned as part of error handling. Fixed
>>> it as part of this patch.
>>>
>>
>> Hi Souptick,
>>
>> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
>> That got as far as a v4 patch [1], and then I asked him to let me put
>> it into my tree. But then it didn't directly apply anymore after the
>> whole design moved to pin+unpin, and so here we are now.
>>
>>
>> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
>> tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).
> 
> Sure, will add him as *Co-Developed-by*
>>


Yes, but it's best to wait and see if he responds, before adding that tag, because
it also required a Signed-off-by from him.

thanks,
-- 
John Hubbard
NVIDIA

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Dan Carpenter]

On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > >       sg_free_table(&acd->sgt);
> > >    err_dma_map_sg:
> > >    err_alloc_sg_table:
> >
> > So now we end up with two unnecessary labels. Probably best to delete two of these
> > three and name the remaining one appropriately:
> 
> Hmm, I thought about it. But later decided to wait for review comments
> on the same in v1.
> I will remove it now.

These are all unrelated to pin_user_pages().  Please don't do it in the
same patch. Staging code is there because it's ugly...  If you don't
want to do unrelated changes to label names then you don't have to.

Also on a personal note.  The label name should say what the goto does
just like a function name says what the function does.  "goto put_pages;"
Or "goto free_foo;".

Don't do this:

	p = kmalloc();
	if (!p)
		goto kmalloc_failed;

This is a come-from label name and does not provide any information that
is not there on the line above...

If you send a patch which uses your own personal style of label names,
I won't say anything about unless there is a bug.  But just know in your
heart that you are wrong and I have silently reviewed your patch to
drivers/staging.

regards,
dan carpenter

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Dan Carpenter]

On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > >       sg_free_table(&acd->sgt);
> > >    err_dma_map_sg:
> > >    err_alloc_sg_table:
> >
> > So now we end up with two unnecessary labels. Probably best to delete two of these
> > three and name the remaining one appropriately:
> 
> Hmm, I thought about it. But later decided to wait for review comments
> on the same in v1.
> I will remove it now.

These are all unrelated to pin_user_pages().  Please don't do it in the
same patch. Staging code is there because it's ugly...  If you don't
want to do unrelated changes to label names then you don't have to.

Also on a personal note.  The label name should say what the goto does
just like a function name says what the function does.  "goto put_pages;"
Or "goto free_foo;".

Don't do this:

	p = kmalloc();
	if (!p)
		goto kmalloc_failed;

This is a come-from label name and does not provide any information that
is not there on the line above...

If you send a patch which uses your own personal style of label names,
I won't say anything about unless there is a bug.  But just know in your
heart that you are wrong and I have silently reviewed your patch to
drivers/staging.

regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Dan Carpenter]

On Mon, Jun 08, 2020 at 12:05:57PM -0700, John Hubbard wrote:
> On 2020-06-08 12:01, Souptick Joarder wrote:
> > On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
> > > 
> > > On 2020-05-31 10:51, Souptick Joarder wrote:
> > > > In 2019, we introduced pin_user_pages*() and now we are converting
> > > > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > > > be referred for more information.
> > > > 
> > > > When pin_user_pages() returns numbers of partially mapped pages,
> > > > those pages were not unpinned as part of error handling. Fixed
> > > > it as part of this patch.
> > > > 
> > > 
> > > Hi Souptick,
> > > 
> > > btw, Bharath (+cc) attempted to do the "put" side of this, last year.
> > > That got as far as a v4 patch [1], and then I asked him to let me put
> > > it into my tree. But then it didn't directly apply anymore after the
> > > whole design moved to pin+unpin, and so here we are now.
> > > 
> > > 
> > > If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
> > > tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).
> > 
> > Sure, will add him as *Co-Developed-by*
> > > 
> 
> 
> Yes, but it's best to wait and see if he responds, before adding that tag, because
> it also required a Signed-off-by from him.

Souptick is porting patches from your tree?  It's not clear to me how
Bharath actually helped author this patch.

regards,
dan carpenter

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Dan Carpenter]

On Mon, Jun 08, 2020 at 12:05:57PM -0700, John Hubbard wrote:
> On 2020-06-08 12:01, Souptick Joarder wrote:
> > On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
> > > 
> > > On 2020-05-31 10:51, Souptick Joarder wrote:
> > > > In 2019, we introduced pin_user_pages*() and now we are converting
> > > > get_user_pages*() to the new API as appropriate. [1] & [2] could
> > > > be referred for more information.
> > > > 
> > > > When pin_user_pages() returns numbers of partially mapped pages,
> > > > those pages were not unpinned as part of error handling. Fixed
> > > > it as part of this patch.
> > > > 
> > > 
> > > Hi Souptick,
> > > 
> > > btw, Bharath (+cc) attempted to do the "put" side of this, last year.
> > > That got as far as a v4 patch [1], and then I asked him to let me put
> > > it into my tree. But then it didn't directly apply anymore after the
> > > whole design moved to pin+unpin, and so here we are now.
> > > 
> > > 
> > > If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
> > > tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).
> > 
> > Sure, will add him as *Co-Developed-by*
> > > 
> 
> 
> Yes, but it's best to wait and see if he responds, before adding that tag, because
> it also required a Signed-off-by from him.

Souptick is porting patches from your tree?  It's not clear to me how
Bharath actually helped author this patch.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-06-08 12:16, Dan Carpenter wrote:
> On Mon, Jun 08, 2020 at 12:05:57PM -0700, John Hubbard wrote:
>> On 2020-06-08 12:01, Souptick Joarder wrote:
>>> On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>>>
>>>> On 2020-05-31 10:51, Souptick Joarder wrote:
>>>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>>>> be referred for more information.
>>>>>
>>>>> When pin_user_pages() returns numbers of partially mapped pages,
>>>>> those pages were not unpinned as part of error handling. Fixed
>>>>> it as part of this patch.
>>>>>
>>>>
>>>> Hi Souptick,
>>>>
>>>> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
>>>> That got as far as a v4 patch [1], and then I asked him to let me put
>>>> it into my tree. But then it didn't directly apply anymore after the
>>>> whole design moved to pin+unpin, and so here we are now.
>>>>
>>>>
>>>> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
>>>> tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).
>>>
>>> Sure, will add him as *Co-Developed-by*
>>>>
>>
>>
>> Yes, but it's best to wait and see if he responds, before adding that tag, because
>> it also required a Signed-off-by from him.
> 
> Souptick is porting patches from your tree?  It's not clear to me how
> Bharath actually helped author this patch.


What happened is that Bharath wrote patches very similar to these, last year.
And we spent some time on review and figuring out pre-existing issues in the code.

Anyway, I suspect that he's not actually involved anymore, so I probably shouldn't
have done any more than to just put him on Cc. Sorry for any confusion I created
here.

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[John Hubbard]

On 2020-06-08 12:16, Dan Carpenter wrote:
> On Mon, Jun 08, 2020 at 12:05:57PM -0700, John Hubbard wrote:
>> On 2020-06-08 12:01, Souptick Joarder wrote:
>>> On Mon, Jun 1, 2020 at 7:15 AM John Hubbard <jhubbard@nvidia.com> wrote:
>>>>
>>>> On 2020-05-31 10:51, Souptick Joarder wrote:
>>>>> In 2019, we introduced pin_user_pages*() and now we are converting
>>>>> get_user_pages*() to the new API as appropriate. [1] & [2] could
>>>>> be referred for more information.
>>>>>
>>>>> When pin_user_pages() returns numbers of partially mapped pages,
>>>>> those pages were not unpinned as part of error handling. Fixed
>>>>> it as part of this patch.
>>>>>
>>>>
>>>> Hi Souptick,
>>>>
>>>> btw, Bharath (+cc) attempted to do the "put" side of this, last year.
>>>> That got as far as a v4 patch [1], and then I asked him to let me put
>>>> it into my tree. But then it didn't directly apply anymore after the
>>>> whole design moved to pin+unpin, and so here we are now.
>>>>
>>>>
>>>> If Bharath is still doing kernel work, you might offer him a Co-Developed-by:
>>>> tag (see https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html).
>>>
>>> Sure, will add him as *Co-Developed-by*
>>>>
>>
>>
>> Yes, but it's best to wait and see if he responds, before adding that tag, because
>> it also required a Signed-off-by from him.
> 
> Souptick is porting patches from your tree?  It's not clear to me how
> Bharath actually helped author this patch.


What happened is that Bharath wrote patches very similar to these, last year.
And we spent some time on review and figuring out pre-existing issues in the code.

Anyway, I suspect that he's not actually involved anymore, so I probably shouldn't
have done any more than to just put him on Cc. Sorry for any confusion I created
here.

thanks,
-- 
John Hubbard
NVIDIA
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

On Tue, Jun 9, 2020 at 12:47 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > > >       sg_free_table(&acd->sgt);
> > > >    err_dma_map_sg:
> > > >    err_alloc_sg_table:
> > >
> > > So now we end up with two unnecessary labels. Probably best to delete two of these
> > > three and name the remaining one appropriately:
> >
> > Hmm, I thought about it. But later decided to wait for review comments
> > on the same in v1.
> > I will remove it now.
>
> These are all unrelated to pin_user_pages().  Please don't do it in the
> same patch. Staging code is there because it's ugly...  If you don't
> want to do unrelated changes to label names then you don't have to.

What I am planning is to put this changes in a series. One patch will take care
of pin_user_pages() related changes, 2nd patch will take care of minor bug
fix in error path + level correction and 3rd patch
will take care of set_page_dirty() -> set_page_dirty_lock().

Does it make sense ?

>
> Also on a personal note.  The label name should say what the goto does
> just like a function name says what the function does.  "goto put_pages;"
> Or "goto free_foo;".
>
> Don't do this:
>
>         p = kmalloc();
>         if (!p)
>                 goto kmalloc_failed;
>
> This is a come-from label name and does not provide any information that
> is not there on the line above...
>
> If you send a patch which uses your own personal style of label names,
> I won't say anything about unless there is a bug.  But just know in your
> heart that you are wrong and I have silently reviewed your patch to
> drivers/staging.
>
> regards,
> dan carpenter

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Souptick Joarder]

On Tue, Jun 9, 2020 at 12:47 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > > >       sg_free_table(&acd->sgt);
> > > >    err_dma_map_sg:
> > > >    err_alloc_sg_table:
> > >
> > > So now we end up with two unnecessary labels. Probably best to delete two of these
> > > three and name the remaining one appropriately:
> >
> > Hmm, I thought about it. But later decided to wait for review comments
> > on the same in v1.
> > I will remove it now.
>
> These are all unrelated to pin_user_pages().  Please don't do it in the
> same patch. Staging code is there because it's ugly...  If you don't
> want to do unrelated changes to label names then you don't have to.

What I am planning is to put this changes in a series. One patch will take care
of pin_user_pages() related changes, 2nd patch will take care of minor bug
fix in error path + level correction and 3rd patch
will take care of set_page_dirty() -> set_page_dirty_lock().

Does it make sense ?

>
> Also on a personal note.  The label name should say what the goto does
> just like a function name says what the function does.  "goto put_pages;"
> Or "goto free_foo;".
>
> Don't do this:
>
>         p = kmalloc();
>         if (!p)
>                 goto kmalloc_failed;
>
> This is a come-from label name and does not provide any information that
> is not there on the line above...
>
> If you send a patch which uses your own personal style of label names,
> I won't say anything about unless there is a bug.  But just know in your
> heart that you are wrong and I have silently reviewed your patch to
> drivers/staging.
>
> regards,
> dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Dan Carpenter]

On Tue, Jun 09, 2020 at 01:03:51AM +0530, Souptick Joarder wrote:
> On Tue, Jun 9, 2020 at 12:47 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > > > >       sg_free_table(&acd->sgt);
> > > > >    err_dma_map_sg:
> > > > >    err_alloc_sg_table:
> > > >
> > > > So now we end up with two unnecessary labels. Probably best to delete two of these
> > > > three and name the remaining one appropriately:
> > >
> > > Hmm, I thought about it. But later decided to wait for review comments
> > > on the same in v1.
> > > I will remove it now.
> >
> > These are all unrelated to pin_user_pages().  Please don't do it in the
> > same patch. Staging code is there because it's ugly...  If you don't
> > want to do unrelated changes to label names then you don't have to.
> 
> What I am planning is to put this changes in a series. One patch will take care
> of pin_user_pages() related changes, 2nd patch will take care of minor bug
> fix in error path + level correction and 3rd patch
> will take care of set_page_dirty() -> set_page_dirty_lock().


Always do bug fixes first.  Always do the easiest least controversial
after first.

Do the error handling bug first.  Change "rv" to int.  That's closely
related to the error handling.  Then set_page_dirty_lock().  Then the
conversion to pin_user_pages().

Then if you want you can do any unrelated clean ups and error label
renames as patch 4.

regards,
dan carpenter

Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages()

[Dan Carpenter]

On Tue, Jun 09, 2020 at 01:03:51AM +0530, Souptick Joarder wrote:
> On Tue, Jun 9, 2020 at 12:47 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Jun 09, 2020 at 12:31:42AM +0530, Souptick Joarder wrote:
> > > > > @@ -189,10 +192,9 @@ static int kpc_dma_transfer(struct dev_private_data *priv,
> > > > >       sg_free_table(&acd->sgt);
> > > > >    err_dma_map_sg:
> > > > >    err_alloc_sg_table:
> > > >
> > > > So now we end up with two unnecessary labels. Probably best to delete two of these
> > > > three and name the remaining one appropriately:
> > >
> > > Hmm, I thought about it. But later decided to wait for review comments
> > > on the same in v1.
> > > I will remove it now.
> >
> > These are all unrelated to pin_user_pages().  Please don't do it in the
> > same patch. Staging code is there because it's ugly...  If you don't
> > want to do unrelated changes to label names then you don't have to.
> 
> What I am planning is to put this changes in a series. One patch will take care
> of pin_user_pages() related changes, 2nd patch will take care of minor bug
> fix in error path + level correction and 3rd patch
> will take care of set_page_dirty() -> set_page_dirty_lock().


Always do bug fixes first.  Always do the easiest least controversial
after first.

Do the error handling bug first.  Change "rv" to int.  That's closely
related to the error handling.  Then set_page_dirty_lock().  Then the
conversion to pin_user_pages().

Then if you want you can do any unrelated clean ups and error label
renames as patch 4.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel