* [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-05-31 17:51 ` Souptick Joarder 0 siblings, 0 replies; 18+ messages in thread From: Souptick Joarder @ 2020-05-31 17:51 UTC (permalink / raw) To: gregkh, jane.pnx9, simon, harshjain32, linux.bhar, festevam, jeremy Cc: devel, linux-kernel, Souptick Joarder, John Hubbard 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-05-31 17:51 ` Souptick Joarder 0 siblings, 0 replies; 18+ messages in thread From: Souptick Joarder @ 2020-05-31 17:51 UTC (permalink / raw) To: gregkh, jane.pnx9, simon, harshjain32, linux.bhar, festevam, jeremy Cc: devel, John Hubbard, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-05-31 17:51 ` Souptick Joarder @ 2020-06-01 1:44 ` John Hubbard -1 siblings, 0 replies; 18+ messages in thread From: John Hubbard @ 2020-06-01 1:44 UTC (permalink / raw) To: Souptick Joarder, gregkh, jane.pnx9, simon, harshjain32, linux.bhar, festevam, jeremy Cc: devel, linux-kernel, Bharath Vedartham 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-01 1:44 ` John Hubbard 0 siblings, 0 replies; 18+ messages in thread From: John Hubbard @ 2020-06-01 1:44 UTC (permalink / raw) To: Souptick Joarder, gregkh, jane.pnx9, simon, harshjain32, linux.bhar, festevam, jeremy Cc: devel, Bharath Vedartham, linux-kernel 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-01 1:44 ` John Hubbard @ 2020-06-08 19:01 ` Souptick Joarder -1 siblings, 0 replies; 18+ messages in thread From: Souptick Joarder @ 2020-06-08 19:01 UTC (permalink / raw) To: John Hubbard Cc: Greg KH, jane.pnx9, Simon Sandström, harshjain32, festevam, jeremy, open list:ANDROID DRIVERS, linux-kernel, Bharath Vedartham 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:01 ` Souptick Joarder 0 siblings, 0 replies; 18+ messages in thread From: Souptick Joarder @ 2020-06-08 19:01 UTC (permalink / raw) To: John Hubbard Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-08 19:01 ` Souptick Joarder @ 2020-06-08 19:05 ` John Hubbard -1 siblings, 0 replies; 18+ messages in thread From: John Hubbard @ 2020-06-08 19:05 UTC (permalink / raw) To: Souptick Joarder Cc: Greg KH, jane.pnx9, Simon Sandström, harshjain32, festevam, jeremy, open list:ANDROID DRIVERS, linux-kernel, Bharath Vedartham 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:05 ` John Hubbard 0 siblings, 0 replies; 18+ messages in thread From: John Hubbard @ 2020-06-08 19:05 UTC (permalink / raw) To: Souptick Joarder Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-08 19:05 ` John Hubbard @ 2020-06-08 19:16 ` Dan Carpenter -1 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2020-06-08 19:16 UTC (permalink / raw) To: John Hubbard Cc: Souptick Joarder, open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:16 ` Dan Carpenter 0 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2020-06-08 19:16 UTC (permalink / raw) To: John Hubbard Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Souptick Joarder, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-08 19:16 ` Dan Carpenter @ 2020-06-08 19:22 ` John Hubbard -1 siblings, 0 replies; 18+ messages in thread From: John Hubbard @ 2020-06-08 19:22 UTC (permalink / raw) To: Dan Carpenter Cc: Souptick Joarder, open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:22 ` John Hubbard 0 siblings, 0 replies; 18+ messages in thread From: John Hubbard @ 2020-06-08 19:22 UTC (permalink / raw) To: Dan Carpenter Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Souptick Joarder, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-08 19:01 ` Souptick Joarder @ 2020-06-08 19:14 ` Dan Carpenter -1 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2020-06-08 19:14 UTC (permalink / raw) To: Souptick Joarder Cc: John Hubbard, open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:14 ` Dan Carpenter 0 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2020-06-08 19:14 UTC (permalink / raw) To: Souptick Joarder Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, John Hubbard, Simon Sandström, linux-kernel, Greg KH, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-08 19:14 ` Dan Carpenter @ 2020-06-08 19:33 ` Souptick Joarder -1 siblings, 0 replies; 18+ messages in thread From: Souptick Joarder @ 2020-06-08 19:33 UTC (permalink / raw) To: Dan Carpenter Cc: John Hubbard, open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:33 ` Souptick Joarder 0 siblings, 0 replies; 18+ messages in thread From: Souptick Joarder @ 2020-06-08 19:33 UTC (permalink / raw) To: Dan Carpenter Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, John Hubbard, Simon Sandström, linux-kernel, Greg KH, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() 2020-06-08 19:33 ` Souptick Joarder @ 2020-06-08 19:55 ` Dan Carpenter -1 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2020-06-08 19:55 UTC (permalink / raw) To: Souptick Joarder Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, John Hubbard, Simon Sandström, linux-kernel, Greg KH, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() @ 2020-06-08 19:55 ` Dan Carpenter 0 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2020-06-08 19:55 UTC (permalink / raw) To: Souptick Joarder Cc: open list:ANDROID DRIVERS, Bharath Vedartham, harshjain32, Greg KH, John Hubbard, linux-kernel, Simon Sandström, jane.pnx9 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-06-08 19:57 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-31 17:51 [PATCH] staging: kpc2000: kpc_dma: Convert get_user_pages() --> pin_user_pages() Souptick Joarder 2020-05-31 17:51 ` Souptick Joarder 2020-06-01 1:44 ` John Hubbard 2020-06-01 1:44 ` John Hubbard 2020-06-08 19:01 ` Souptick Joarder 2020-06-08 19:01 ` Souptick Joarder 2020-06-08 19:05 ` John Hubbard 2020-06-08 19:05 ` John Hubbard 2020-06-08 19:16 ` Dan Carpenter 2020-06-08 19:16 ` Dan Carpenter 2020-06-08 19:22 ` John Hubbard 2020-06-08 19:22 ` John Hubbard 2020-06-08 19:14 ` Dan Carpenter 2020-06-08 19:14 ` Dan Carpenter 2020-06-08 19:33 ` Souptick Joarder 2020-06-08 19:33 ` Souptick Joarder 2020-06-08 19:55 ` Dan Carpenter 2020-06-08 19:55 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.