media: atomisp: Fixed error handling path

PATCH In review Souptick Joarder 2 participants 6 messages Nov 4, 2020 – Dec 29, 2020 2 messages not in the archive

Subsystems (4)

Media (V4L/DVB) Staging Intel MID (Mobile Internet Device) platform Staging - atomisp driver

lore

Share & tools

lore

Souptick Joarder

author

2020-11-04 02:02 UTC #0 lore raw

PATCH v2 1/1 media: atomisp: Fixed error handling path

Inside alloc_user_pages() based on flag value either pin_user_pages() or get_user_pages_fast() will be called. However, these API might fail.

But free_user_pages() called in error handling path doesn't bother about return value and will try to unpin bo->pgnr pages, which is incorrect.

Fix this by passing the page_nr to free_user_pages(). If page_nr > 0 pages will be unpinned based on bo->mem_type. This will also take care of non error handling path.

Fixes 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory

allocation")

Signed-off-by Souptick Joarder <[email protected]>

Reviewed-by Dan Carpenter <[email protected]>

Cc John Hubbard <[email protected]>

Cc Ira Weiny <[email protected]>

Cc Dan Carpenter <[email protected]>

---
v2:
Added review tag.

 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/staging/media/atomisp/pci/hmm/hmm_bo.c +8 -5

index f13af23..0168f98 100644
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
 	kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+			    unsigned int page_nr)
 {
 	int i;
 
 	hmm_mem_stat.usr_size -= bo->pgnr;
 
 	if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-		unpin_user_pages(bo->pages, bo->pgnr);
+		unpin_user_pages(bo->pages, page_nr);
 	} else {
-		for (i = 0; i < bo->pgnr; i++)
+		for (i = 0; i < page_nr; i++)
 			put_page(bo->pages[i]);
 	}
 	kfree(bo->pages);

@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 		dev_err(atomisp_dev,
 			"get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
 			bo->pgnr, page_nr);
+		if (page_nr < 0)
+			page_nr = 0;
 		goto out_of_mem;
 	}

@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-	free_user_pages(bo);
+	free_user_pages(bo, page_nr);
 
 	return -ENOMEM;
 }

@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
 	if (bo->type == HMM_BO_PRIVATE)
 		free_private_pages(bo, &dynamic_pool, &reserved_pool);
 	else if (bo->type == HMM_BO_USER)
-		free_user_pages(bo);
+		free_user_pages(bo, bo->pgnr);
 	else
 		dev_err(atomisp_dev, "invalid buffer type.\n");
 	mutex_unlock(&bo->mutex);

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

Dan Carpenter

2020-11-04 08:31 UTC #2 lore raw

Re: [PATCH v2] media: atomisp: Fix error handling path

Quoted 4 lines · Markus Elfring

Quoted 2 lines
Fixes 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Please delete a line break for this tag.

Markus, the thing is that we all saw the line break and we just thought it didn't matter at all...

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): [email protected]

Dan Carpenter

2020-11-04 11:40 UTC #4 lore raw

Re: [v2] media: atomisp: Fix error handling path

Quoted 10 lines · Markus Elfring

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Fixes 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Please delete a line break for this tag.
Markus, the thing is that we all saw the line break and we just thought it didn't matter at all...
Do you disagree to the known documentation then?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=4ef8451b332662d004df269d4cdeb7d9f31419b5#n123

The documentation is correct but no one wants you to constantly be nagging developers about minor stuff...

One thing that I do is I start to write an email and then if I realize it's not worth complaining about and I save it to my postponed messages folder. Then I never send it and I forget about it completely. I have currently have 740 messages in my postponed messages folder. :P

That's a lot of whining and complaining which I never sent and the world is the more beautiful for it.

regards,
dan carpenter

Souptick Joarder

author

2020-11-18 19:36 UTC #5 lore raw

In reply to Souptick Joarder

Quoted 14 lines · Souptick Joarder

Inside alloc_user_pages() based on flag value either pin_user_pages() or get_user_pages_fast() will be called. However, these API might fail.
But free_user_pages() called in error handling path doesn't bother about return value and will try to unpin bo->pgnr pages, which is incorrect.
Fix this by passing the page_nr to free_user_pages(). If page_nr > 0 pages will be unpinned based on bo->mem_type. This will also take care of non error handling path.
Fixes 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")

Quoted 8 lines

Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Dan Carpenter <[email protected]>
Cc John Hubbard <[email protected]>
Cc Ira Weiny <[email protected]>
Cc Dan Carpenter <[email protected]>
---
v2:
Added review tag.

Any further comment ? If no, can we get this patch in queue for 5.11 ?

Quoted 59 lines

 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/staging/media/atomisp/pci/hmm/hmm_bo.c +8 -6

index f13af23..0168f98 100644
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
        kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+                           unsigned int page_nr)
 {
        int i;
 
        hmm_mem_stat.usr_size -= bo->pgnr;
 
        if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-               unpin_user_pages(bo->pages, bo->pgnr);
+               unpin_user_pages(bo->pages, page_nr);
        } else {
-               for (i = 0; i < bo->pgnr; i++)
+               for (i = 0; i < page_nr; i++)
                        put_page(bo->pages[i]);
        }
        kfree(bo->pages);

@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
                dev_err(atomisp_dev,
                        "get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
                        bo->pgnr, page_nr);
+               if (page_nr < 0)
+                       page_nr = 0;
                goto out_of_mem;
        }

@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-       free_user_pages(bo);
+       free_user_pages(bo, page_nr);
 
        return -ENOMEM;
 }

@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
        if (bo->type == HMM_BO_PRIVATE)
                free_private_pages(bo, &dynamic_pool, &reserved_pool);
        else if (bo->type == HMM_BO_USER)
-               free_user_pages(bo);
+               free_user_pages(bo, bo->pgnr);
        else
                dev_err(atomisp_dev, "invalid buffer type.\n");
        mutex_unlock(&bo->mutex);
--

1.9.1

Souptick Joarder

author

2020-12-08 20:17 UTC #6 lore raw

Quoted 25 lines · Souptick Joarder

Quoted 14 lines · Souptick Joarder
Inside alloc_user_pages() based on flag value either pin_user_pages() or get_user_pages_fast() will be called. However, these API might fail.
But free_user_pages() called in error handling path doesn't bother about return value and will try to unpin bo->pgnr pages, which is incorrect.
Fix this by passing the page_nr to free_user_pages(). If page_nr > 0 pages will be unpinned based on bo->mem_type. This will also take care of non error handling path.
Fixes 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Quoted 8 lines
Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Dan Carpenter <[email protected]>
Cc John Hubbard <[email protected]>
Cc Ira Weiny <[email protected]>
Cc Dan Carpenter <[email protected]>
---
v2:
Added review tag.

Quoted 1 line

Any further comment ? If no, can we get this patch in queue for 5.11 ?

Can we get this patch in the queue for 5.11 ?

Quoted 59 lines

 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/staging/media/atomisp/pci/hmm/hmm_bo.c +8 -6

index f13af23..0168f98 100644
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
        kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+                           unsigned int page_nr)
 {
        int i;
 
        hmm_mem_stat.usr_size -= bo->pgnr;
 
        if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-               unpin_user_pages(bo->pages, bo->pgnr);
+               unpin_user_pages(bo->pages, page_nr);
        } else {
-               for (i = 0; i < bo->pgnr; i++)
+               for (i = 0; i < page_nr; i++)
                        put_page(bo->pages[i]);
        }
        kfree(bo->pages);

@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
                dev_err(atomisp_dev,
                        "get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
                        bo->pgnr, page_nr);
+               if (page_nr < 0)
+                       page_nr = 0;
                goto out_of_mem;
        }

@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-       free_user_pages(bo);
+       free_user_pages(bo, page_nr);
 
        return -ENOMEM;
 }

@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
        if (bo->type == HMM_BO_PRIVATE)
                free_private_pages(bo, &dynamic_pool, &reserved_pool);
        else if (bo->type == HMM_BO_USER)
-               free_user_pages(bo);
+               free_user_pages(bo, bo->pgnr);
        else
                dev_err(atomisp_dev, "invalid buffer type.\n");
        mutex_unlock(&bo->mutex);
--

1.9.1

Souptick Joarder

author

2020-12-29 07:50 UTC #7 lore raw

Quoted 27 lines · Souptick Joarder

Quoted 25 lines · Souptick Joarder
Quoted 22 lines · Souptick Joarder
Inside alloc_user_pages() based on flag value either pin_user_pages() or get_user_pages_fast() will be called. However, these API might fail.
But free_user_pages() called in error handling path doesn't bother about return value and will try to unpin bo->pgnr pages, which is incorrect.
Fix this by passing the page_nr to free_user_pages(). If page_nr > 0 pages will be unpinned based on bo->mem_type. This will also take care of non error handling path.
Fixes 14a638ab96c5 ("media: atomisp: use pin_user_pages() for memory
allocation")
Signed-off-by Souptick Joarder <[email protected]>
Reviewed-by Dan Carpenter <[email protected]>
Cc John Hubbard <[email protected]>
Cc Ira Weiny <[email protected]>
Cc Dan Carpenter <[email protected]>
---
v2:
Added review tag.

Quoted 3 lines

Quoted 1 line
Any further comment ? If no, can we get this patch in queue for 5.11 ?
Can we get this patch in the queue for 5.11 ?

Any further comment on this patch ?

Quoted 60 lines


 drivers/staging/media/atomisp/pci/hmm/hmm_bo.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

drivers/staging/media/atomisp/pci/hmm/hmm_bo.c +8 -6

index f13af23..0168f98 100644
@@ -857,16 +857,17 @@ static void free_private_pages(struct hmm_buffer_object *bo,
        kfree(bo->page_obj);
 }
 
-static void free_user_pages(struct hmm_buffer_object *bo)
+static void free_user_pages(struct hmm_buffer_object *bo,
+                           unsigned int page_nr)
 {
        int i;
 
        hmm_mem_stat.usr_size -= bo->pgnr;
 
        if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-               unpin_user_pages(bo->pages, bo->pgnr);
+               unpin_user_pages(bo->pages, page_nr);
        } else {
-               for (i = 0; i < bo->pgnr; i++)
+               for (i = 0; i < page_nr; i++)
                        put_page(bo->pages[i]);
        }
        kfree(bo->pages);

@@ -942,6 +943,8 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
                dev_err(atomisp_dev,
                        "get_user_pages err: bo->pgnr = %d, pgnr actually pinned = %d.\n",
                        bo->pgnr, page_nr);
+               if (page_nr < 0)
+                       page_nr = 0;
                goto out_of_mem;
        }

@@ -954,7 +957,7 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 
 out_of_mem:
 
-       free_user_pages(bo);
+       free_user_pages(bo, page_nr);
 
        return -ENOMEM;
 }

@@ -1037,7 +1040,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
        if (bo->type == HMM_BO_PRIVATE)
                free_private_pages(bo, &dynamic_pool, &reserved_pool);
        else if (bo->type == HMM_BO_USER)
-               free_user_pages(bo);
+               free_user_pages(bo, bo->pgnr);
        else
                dev_err(atomisp_dev, "invalid buffer type.\n");
        mutex_unlock(&bo->mutex);
--

1.9.1