| Message ID | 20220413225531.9425-1-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: media: atomisp: Use kmap_local_page() in hmm_store() | expand |
On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco wrote: > The use of kmap() is being deprecated in favor of kmap_local_page() > where it is feasible. The same is true for kmap_atomic(). > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > First of all, in_atomic() shouldn't be used in drivers. This macro > cannot always detect atomic context; in particular, it cannot know > about held spinlocks in non-preemptible kernels. > > Notwithstanding what it is said above, this code doesn't need to care > whether or not it is executing in atomic context. It can simply use > kmap_local_page() / kunmap_local() that can instead do the mapping / > unmapping regardless of the context. > > With kmap_local_page(), the mapping is per thread, CPU local and not > globally visible. Therefore, hmm_store()() is a function where the use > of kmap_local_page() in place of both kmap() and kmap_atomic() is > correctly suited. > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > unnecessary tests which test if the code is in atomic context. > Not specifically about this patch, but more generally about all such conversions - is there a 'proof' that shows this just works or do we need to test each one. If the latter, then how? > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c > index 46ac082cd3f1..54188197c3dc 100644 > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > idx = (virt - bo->start) >> PAGE_SHIFT; > offset = (virt - bo->start) - (idx << PAGE_SHIFT); > > - if (in_atomic()) > - des = (char *)kmap_atomic(bo->page_obj[idx].page); > - else > - des = (char *)kmap(bo->page_obj[idx].page); > + des = (char *)kmap_local_page(bo->page_obj[idx].page); > > if (!des) { > dev_err(atomisp_dev, > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > clflush_cache_range(des, len); > > - if (in_atomic()) > - /* > - * Note: kunmap_atomic requires return addr from > - * kmap_atomic, not the page. See linux/highmem.h > - */ > - kunmap_atomic(des - offset); > - else > - kunmap(bo->page_obj[idx].page); > + kunmap_local(des); > } > > return 0; > -- > 2.34.1 > >
On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote: > On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco wrote: > > The use of kmap() is being deprecated in favor of kmap_local_page() > > where it is feasible. The same is true for kmap_atomic(). > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > > > First of all, in_atomic() shouldn't be used in drivers. This macro > > cannot always detect atomic context; in particular, it cannot know > > about held spinlocks in non-preemptible kernels. > > > > Notwithstanding what it is said above, this code doesn't need to care > > whether or not it is executing in atomic context. It can simply use > > kmap_local_page() / kunmap_local() that can instead do the mapping / > > unmapping regardless of the context. > > > > With kmap_local_page(), the mapping is per thread, CPU local and not > > globally visible. Therefore, hmm_store()() is a function where the use > > of kmap_local_page() in place of both kmap() and kmap_atomic() is > > correctly suited. > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > > unnecessary tests which test if the code is in atomic context. > > > > Not specifically about this patch, but more generally about all > such conversions - is there a 'proof' that shows this just works Just code inspection. Most of them that I have done have been compile tested only. Part of the key is that des is a local variable and is not aliased by anything outside this function. > or do we need to test each one. If the latter, then how? Generally there is no test if we don't have the hardware. Some of the more difficult conversions will probably need to have some testing done but that will need to be discussed with the subsystem maintainers at the time. Ira > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c > > index 46ac082cd3f1..54188197c3dc 100644 > > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c > > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c > > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > idx = (virt - bo->start) >> PAGE_SHIFT; > > offset = (virt - bo->start) - (idx << PAGE_SHIFT); > > > > - if (in_atomic()) > > - des = (char *)kmap_atomic(bo->page_obj[idx].page); > > - else > > - des = (char *)kmap(bo->page_obj[idx].page); > > + des = (char *)kmap_local_page(bo->page_obj[idx].page); > > > > if (!des) { > > dev_err(atomisp_dev, > > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > > > clflush_cache_range(des, len); > > > > - if (in_atomic()) > > - /* > > - * Note: kunmap_atomic requires return addr from > > - * kmap_atomic, not the page. See linux/highmem.h > > - */ > > - kunmap_atomic(des - offset); > > - else > > - kunmap(bo->page_obj[idx].page); > > + kunmap_local(des); > > } > > > > return 0; > > -- > > 2.34.1 > > > >
On Wed, 13 Apr 2022, Ira Weiny wrote: > On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote: > > On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco wrote: > > > The use of kmap() is being deprecated in favor of kmap_local_page() > > > where it is feasible. The same is true for kmap_atomic(). > > > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > > > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > > > > > First of all, in_atomic() shouldn't be used in drivers. This macro > > > cannot always detect atomic context; in particular, it cannot know > > > about held spinlocks in non-preemptible kernels. > > > > > > Notwithstanding what it is said above, this code doesn't need to care > > > whether or not it is executing in atomic context. It can simply use > > > kmap_local_page() / kunmap_local() that can instead do the mapping / > > > unmapping regardless of the context. > > > > > > With kmap_local_page(), the mapping is per thread, CPU local and not > > > globally visible. Therefore, hmm_store()() is a function where the use > > > of kmap_local_page() in place of both kmap() and kmap_atomic() is > > > correctly suited. > > > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > > > unnecessary tests which test if the code is in atomic context. > > > > > > > Not specifically about this patch, but more generally about all > > such conversions - is there a 'proof' that shows this just works > > Just code inspection. Most of them that I have done have been compile tested > only. Part of the key is that des is a local variable and is not aliased by > anything outside this function. Typically, the concern about being in atomic context has to do with whether GFP_KERNEL or GFP_ATOMIC should be used, ie whether allocation can sleep. It doesn't have to do with whether some data can be shared. Is that the concern here? julia > > > or do we need to test each one. If the latter, then how? > > Generally there is no test if we don't have the hardware. Some of the more > difficult conversions will probably need to have some testing done but that > will need to be discussed with the subsystem maintainers at the time. > > Ira > > > > > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > > --- > > > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c > > > index 46ac082cd3f1..54188197c3dc 100644 > > > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c > > > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c > > > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > > idx = (virt - bo->start) >> PAGE_SHIFT; > > > offset = (virt - bo->start) - (idx << PAGE_SHIFT); > > > > > > - if (in_atomic()) > > > - des = (char *)kmap_atomic(bo->page_obj[idx].page); > > > - else > > > - des = (char *)kmap(bo->page_obj[idx].page); > > > + des = (char *)kmap_local_page(bo->page_obj[idx].page); > > > > > > if (!des) { > > > dev_err(atomisp_dev, > > > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > > > > > clflush_cache_range(des, len); > > > > > > - if (in_atomic()) > > > - /* > > > - * Note: kunmap_atomic requires return addr from > > > - * kmap_atomic, not the page. See linux/highmem.h > > > - */ > > > - kunmap_atomic(des - offset); > > > - else > > > - kunmap(bo->page_obj[idx].page); > > > + kunmap_local(des); > > > } > > > > > > return 0; > > > -- > > > 2.34.1 > > > > > > > >
On gioved? 14 aprile 2022 09:03:40 CEST Julia Lawall wrote: > > On Wed, 13 Apr 2022, Ira Weiny wrote: > > > On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote: > > > On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco wrote: > > > > The use of kmap() is being deprecated in favor of kmap_local_page() > > > > where it is feasible. The same is true for kmap_atomic(). > > > > > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > > > > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > > > > > > > First of all, in_atomic() shouldn't be used in drivers. This macro > > > > cannot always detect atomic context; in particular, it cannot know > > > > about held spinlocks in non-preemptible kernels. > > > > > > > > Notwithstanding what it is said above, this code doesn't need to care > > > > whether or not it is executing in atomic context. It can simply use > > > > kmap_local_page() / kunmap_local() that can instead do the mapping / > > > > unmapping regardless of the context. > > > > > > > > With kmap_local_page(), the mapping is per thread, CPU local and not > > > > globally visible. Therefore, hmm_store()() is a function where the use > > > > of kmap_local_page() in place of both kmap() and kmap_atomic() is > > > > correctly suited. > > > > > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > > > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > > > > unnecessary tests which test if the code is in atomic context. > > > > > > > > > > Not specifically about this patch, but more generally about all > > > such conversions - is there a 'proof' that shows this just works > > > > Just code inspection. Most of them that I have done have been compile tested > > only. Part of the key is that des is a local variable and is not aliased by > > anything outside this function. > > Typically, the concern about being in atomic context has to do with > whether GFP_KERNEL or GFP_ATOMIC should be used, ie whether allocation > can sleep. I'd add that the concern about being in atomic context has mainly to do with calling whatever function that may sleep. Some time ago I analyzed a calls chain which, under spinlocks and with IRQ's disabled, led to console_lock() which is annotated with might_sleep(). It took about 8000 ms to recover when executing in a 4 CPU / 8 SMT System. Linus T. suggested to make this work asynchronous (commit 1ee33b1ca2b8 ("tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous")). > It doesn't have to do with whether some data can be shared. Yes, FWIW I agree with you. > Is that the concern here? The concern here is about the locality of the pointer variable to which the struct page has been mapped to. In atomic context we are not allowed to kmap() (this is why in the code we had that in_atomic() test), instead we can kmap_local_page() or kmap_atomic(). The latter is strongly discouraged in favor of the former. Furthermore, Alison was asking if we can prove that these kinds of conversions can actually work when we have not the hardware for testing. As Ira wrote, code inspection is sufficient to prove it. Thanks, Fabio M. De Francesco
On Thu, 14 Apr 2022, Fabio M. De Francesco wrote: > On gioved? 14 aprile 2022 09:03:40 CEST Julia Lawall wrote: > > > > On Wed, 13 Apr 2022, Ira Weiny wrote: > > > > > On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote: > > > > On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco > wrote: > > > > > The use of kmap() is being deprecated in favor of kmap_local_page() > > > > > where it is feasible. The same is true for kmap_atomic(). > > > > > > > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in > atomic > > > > > context and, if so, it calls kmap_atomic(), if not, it calls > kmap(). > > > > > > > > > > First of all, in_atomic() shouldn't be used in drivers. This macro > > > > > cannot always detect atomic context; in particular, it cannot know > > > > > about held spinlocks in non-preemptible kernels. > > > > > > > > > > Notwithstanding what it is said above, this code doesn't need to > care > > > > > whether or not it is executing in atomic context. It can simply use > > > > > kmap_local_page() / kunmap_local() that can instead do the mapping > / > > > > > unmapping regardless of the context. > > > > > > > > > > With kmap_local_page(), the mapping is per thread, CPU local and > not > > > > > globally visible. Therefore, hmm_store()() is a function where the > use > > > > > of kmap_local_page() in place of both kmap() and kmap_atomic() is > > > > > correctly suited. > > > > > > > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > > > > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > > > > > unnecessary tests which test if the code is in atomic context. > > > > > > > > > > > > > Not specifically about this patch, but more generally about all > > > > such conversions - is there a 'proof' that shows this just works > > > > > > Just code inspection. Most of them that I have done have been compile > tested > > > only. Part of the key is that des is a local variable and is not > aliased by > > > anything outside this function. > > > > Typically, the concern about being in atomic context has to do with > > whether GFP_KERNEL or GFP_ATOMIC should be used, ie whether allocation > > can sleep. > > I'd add that the concern about being in atomic context has mainly to do > with calling whatever function that may sleep. > > Some time ago I analyzed a calls chain which, under spinlocks and with > IRQ's disabled, led to console_lock() which is annotated with > might_sleep(). It took about 8000 ms to recover when executing in a 4 CPU / > 8 SMT System. Linus T. suggested to make this work asynchronous (commit > 1ee33b1ca2b8 ("tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous")). > > > It doesn't have to do with whether some data can be shared. > > Yes, FWIW I agree with you. > > > Is that the concern here? > > The concern here is about the locality of the pointer variable to which the > struct page has been mapped to. In atomic context we are not allowed to > kmap() (this is why in the code we had that in_atomic() test), instead we > can kmap_local_page() or kmap_atomic(). The latter is strongly discouraged > in favor of the former. I have the impression that you are first agreeing with me and then contradicting me :). Is your point that in general a concern about atomic context has to do with whether sleeping is allowed, but that the concern is something else here? I'm not familiar with these kmap functions. thanks, julia > > Furthermore, Alison was asking if we can prove that these kinds of > conversions can actually work when we have not the hardware for testing. As > Ira wrote, code inspection is sufficient to prove it. > > Thanks, > > Fabio M. De Francesco > > > >
On gioved? 14 aprile 2022 11:12:39 CEST Julia Lawall wrote: > > On Thu, 14 Apr 2022, Fabio M. De Francesco wrote: > > > On gioved? 14 aprile 2022 09:03:40 CEST Julia Lawall wrote: > > > > > > On Wed, 13 Apr 2022, Ira Weiny wrote: > > > > > > > On Wed, Apr 13, 2022 at 05:44:54PM -0700, Alison Schofield wrote: > > > > > On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco > > wrote: > > > > > > The use of kmap() is being deprecated in favor of kmap_local_page() > > > > > > where it is feasible. The same is true for kmap_atomic(). > > > > > > > > > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in > > atomic > > > > > > context and, if so, it calls kmap_atomic(), if not, it calls > > kmap(). > > > > > > > > > > > > First of all, in_atomic() shouldn't be used in drivers. This macro > > > > > > cannot always detect atomic context; in particular, it cannot know > > > > > > about held spinlocks in non-preemptible kernels. > > > > > > > > > > > > Notwithstanding what it is said above, this code doesn't need to > > care > > > > > > whether or not it is executing in atomic context. It can simply use > > > > > > kmap_local_page() / kunmap_local() that can instead do the mapping > > / > > > > > > unmapping regardless of the context. > > > > > > > > > > > > With kmap_local_page(), the mapping is per thread, CPU local and > > not > > > > > > globally visible. Therefore, hmm_store()() is a function where the > > use > > > > > > of kmap_local_page() in place of both kmap() and kmap_atomic() is > > > > > > correctly suited. > > > > > > > > > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > > > > > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > > > > > > unnecessary tests which test if the code is in atomic context. > > > > > > > > > > > > > > > > Not specifically about this patch, but more generally about all > > > > > such conversions - is there a 'proof' that shows this just works > > > > > > > > Just code inspection. Most of them that I have done have been compile > > tested > > > > only. Part of the key is that des is a local variable and is not > > aliased by > > > > anything outside this function. > > > > > > Typically, the concern about being in atomic context has to do with > > > whether GFP_KERNEL or GFP_ATOMIC should be used, ie whether allocation > > > can sleep. > > > > I'd add that the concern about being in atomic context has mainly to do > > with calling whatever function that may sleep. > > > > Some time ago I analyzed a calls chain which, under spinlocks and with > > IRQ's disabled, led to console_lock() which is annotated with > > might_sleep(). It took about 8000 ms to recover when executing in a 4 CPU / > > 8 SMT System. Linus T. suggested to make this work asynchronous (commit > > 1ee33b1ca2b8 ("tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous")). > > > > > It doesn't have to do with whether some data can be shared. > > > > Yes, FWIW I agree with you. > > > > > Is that the concern here? > > > > The concern here is about the locality of the pointer variable to which the > > struct page has been mapped to. In atomic context we are not allowed to > > kmap() (this is why in the code we had that in_atomic() test), instead we > > can kmap_local_page() or kmap_atomic(). The latter is strongly discouraged > > in favor of the former. > > I have the impression that you are first agreeing with me and then > contradicting me :). Is your point that in general a concern about atomic > context has to do with whether sleeping is allowed, but that the concern > is something else here? I'm not familiar with these kmap functions. Yes the concern is something else here (sorry for my poor English). It was not my intention to contradict you :) The concern for "locality" here is that if the variable is shared between different functions there are cases where we cannot use kmap_local_page(). For example, those mappings with kmap_local_page() are per thread and CPU local. I think that the best means to convey what I want to express is pointing to my patch to Documentation/vm/highmem.rst: https://lore.kernel.org/lkml/20220412124003.10736-1-fmdefrancesco@gmail.com/ I hope that this can help to understand why we care of "locality" in this specific case where we use kmap_local_page(). Again, sorry for not being clear. Thanks, Fabio M. De Francesco > thanks, > julia > > > > > > Furthermore, Alison was asking if we can prove that these kinds of > > conversions can actually work when we have not the hardware for testing. As > > Ira wrote, code inspection is sufficient to prove it. > > > > Thanks, > > > > Fabio M. De Francesco
On Thu, Apr 14, 2022 at 12:55:31AM +0200, Fabio M. De Francesco wrote: > The use of kmap() is being deprecated in favor of kmap_local_page() > where it is feasible. The same is true for kmap_atomic(). > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > First of all, in_atomic() shouldn't be used in drivers. This macro > cannot always detect atomic context; in particular, it cannot know > about held spinlocks in non-preemptible kernels. > > Notwithstanding what it is said above, this code doesn't need to care > whether or not it is executing in atomic context. It can simply use > kmap_local_page() / kunmap_local() that can instead do the mapping / > unmapping regardless of the context. > > With kmap_local_page(), the mapping is per thread, CPU local and not > globally visible. Therefore, hmm_store()() is a function where the use > of kmap_local_page() in place of both kmap() and kmap_atomic() is > correctly suited. > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > unnecessary tests which test if the code is in atomic context. > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> I've had to research this a bit myself because I've not seen this pattern before. kmap_atomic() had 2 uses: 1) a situation where the operation on the page requires pagefaults and/or preemption to be disabled 2) in a situation where one knows the code can't sleep kmap_local_page() removes the second use case because kmap() is no longer the only alternative; from the kdoc for kmap_atomic(): ... * kmap_atomic - Atomically map a page for temporary usage - Deprecated! ... * Effectively a wrapper around kmap_local_page() which disables pagefaults * and preemption. ... The deprecation is because any pagefault/preemption disabling should probably be done explicitly from now on but allows for existing kmap_atomic() callers to live on. In this case, I suspect the original driver writer wanted to use kmap() but hmm_store() was called from various contexts. So they incorrectly tried to protect against the (potentially) sleeping kmap() call. In reality, I think they could have simply called kmap_atomic() and skipped 'in_atomic()' check altogether. Regardless now that kmap_local_page() exists, this is correct. Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c > index 46ac082cd3f1..54188197c3dc 100644 > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > idx = (virt - bo->start) >> PAGE_SHIFT; > offset = (virt - bo->start) - (idx << PAGE_SHIFT); > > - if (in_atomic()) > - des = (char *)kmap_atomic(bo->page_obj[idx].page); > - else > - des = (char *)kmap(bo->page_obj[idx].page); > + des = (char *)kmap_local_page(bo->page_obj[idx].page); > > if (!des) { > dev_err(atomisp_dev, > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > clflush_cache_range(des, len); > > - if (in_atomic()) > - /* > - * Note: kunmap_atomic requires return addr from > - * kmap_atomic, not the page. See linux/highmem.h > - */ > - kunmap_atomic(des - offset); > - else > - kunmap(bo->page_obj[idx].page); > + kunmap_local(des); > } > > return 0; > -- > 2.34.1 >
On venerd? 15 aprile 2022 02:37:27 CEST Ira Weiny wrote: > Regardless now that kmap_local_page() exists, this is correct. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> I'm pretty sure this particular patch would have had a hard time getting accepted without any authoritative "Reviewed-by" tag from you or other people more experienced than me. Thank you very much! Fabio M. De Francesco
Hi, On 4/14/22 00:55, Fabio M. De Francesco wrote: > The use of kmap() is being deprecated in favor of kmap_local_page() > where it is feasible. The same is true for kmap_atomic(). > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > First of all, in_atomic() shouldn't be used in drivers. This macro > cannot always detect atomic context; in particular, it cannot know > about held spinlocks in non-preemptible kernels. > > Notwithstanding what it is said above, this code doesn't need to care > whether or not it is executing in atomic context. It can simply use > kmap_local_page() / kunmap_local() that can instead do the mapping / > unmapping regardless of the context. > > With kmap_local_page(), the mapping is per thread, CPU local and not > globally visible. Therefore, hmm_store()() is a function where the use > of kmap_local_page() in place of both kmap() and kmap_atomic() is > correctly suited. > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > unnecessary tests which test if the code is in atomic context. > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> I've successfully tested this on both the front and back cams of a chuwi hi8 tablet: Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c > index 46ac082cd3f1..54188197c3dc 100644 > --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c > +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c > @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > idx = (virt - bo->start) >> PAGE_SHIFT; > offset = (virt - bo->start) - (idx << PAGE_SHIFT); > > - if (in_atomic()) > - des = (char *)kmap_atomic(bo->page_obj[idx].page); > - else > - des = (char *)kmap(bo->page_obj[idx].page); > + des = (char *)kmap_local_page(bo->page_obj[idx].page); > > if (!des) { > dev_err(atomisp_dev, > @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) > > clflush_cache_range(des, len); > > - if (in_atomic()) > - /* > - * Note: kunmap_atomic requires return addr from > - * kmap_atomic, not the page. See linux/highmem.h > - */ > - kunmap_atomic(des - offset); > - else > - kunmap(bo->page_obj[idx].page); > + kunmap_local(des); > } > > return 0;
On giovedì 14 aprile 2022 00:55:31 CEST Fabio M. De Francesco wrote: > The use of kmap() is being deprecated in favor of kmap_local_page() > where it is feasible. The same is true for kmap_atomic(). > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > First of all, in_atomic() shouldn't be used in drivers. This macro > cannot always detect atomic context; in particular, it cannot know > about held spinlocks in non-preemptible kernels. > > Notwithstanding what it is said above, this code doesn't need to care > whether or not it is executing in atomic context. It can simply use > kmap_local_page() / kunmap_local() that can instead do the mapping / > unmapping regardless of the context. > > With kmap_local_page(), the mapping is per thread, CPU local and not > globally visible. Therefore, hmm_store()() is a function where the use > of kmap_local_page() in place of both kmap() and kmap_atomic() is > correctly suited. > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > unnecessary tests which test if the code is in atomic context. > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) Hi Mauro, I'm writing for just a gentle ping for this and two other staging/atomisp/ patches that still seem to be waiting to be applied. In the meantime I would like to remind you that Hans de Goede has successfully tested this patch and the other two on both the front and back cams of a chuwi hi8 tablet. Please let me know if there is anything else that is required to be done in order to accept the three patches. Thanks, Fabio M. De Francesco
On lunedì 25 aprile 2022 20:29:03 CEST Fabio M. De Francesco wrote: > On giovedì 14 aprile 2022 00:55:31 CEST Fabio M. De Francesco wrote: > > The use of kmap() is being deprecated in favor of kmap_local_page() > > where it is feasible. The same is true for kmap_atomic(). > > > > In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic > > context and, if so, it calls kmap_atomic(), if not, it calls kmap(). > > > > First of all, in_atomic() shouldn't be used in drivers. This macro > > cannot always detect atomic context; in particular, it cannot know > > about held spinlocks in non-preemptible kernels. > > > > Notwithstanding what it is said above, this code doesn't need to care > > whether or not it is executing in atomic context. It can simply use > > kmap_local_page() / kunmap_local() that can instead do the mapping / > > unmapping regardless of the context. > > > > With kmap_local_page(), the mapping is per thread, CPU local and not > > globally visible. Therefore, hmm_store()() is a function where the use > > of kmap_local_page() in place of both kmap() and kmap_atomic() is > > correctly suited. > > > > Convert the calls of kmap() / kunmap() and kmap_atomic() / > > kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the > > unnecessary tests which test if the code is in atomic context. > > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > Hi Mauro, > > I'm writing for just a gentle ping for this and two other staging/ atomisp/ > patches that still seem to be waiting to be applied. > > In the meantime I would like to remind you that Hans de Goede has > successfully tested this patch and the other two on both the front and back > cams of a chuwi hi8 tablet. Hi Mauro, In my previous message I forgot to remind you that two of these three patches have also been reviewed by Ira Weiny (Intel). I'd like to ask you again if there is still something left which prevents these three patches from being accepted. The first of the three patches was submitted on April 9, 2022. For you convenience here are the links to the patches, the "Reviewed-by:" and "Tested-by:" tags: [PATCH] staging: media: atomisp: Use kmap_local_page() in hmm_store() https://lore.kernel.org/lkml/20220413225531.9425-1-fmdefrancesco@gmail.com/ https://lore.kernel.org/lkml/Yli+R7iLZKqO8kVP@iweiny-desk3/ https://lore.kernel.org/lkml/2d096f20-dbaa-1d49-96e9-a7ae6c19f7fe@redhat.com/ [PATCH] staging: media: atomisp: Use kmap_local_page() in hmm_set() https://lore.kernel.org/lkml/20220413212210.18494-1-fmdefrancesco@gmail.com/ https://lore.kernel.org/lkml/YldNhErgt53RqYp7@iweiny-desk3/ https://lore.kernel.org/lkml/0b04ad1a-e442-1728-ef2c-bab386a4c64c@redhat.com/ [PATCH] staging: media: atomisp: Convert kmap() to kmap_local_page() https://lore.kernel.org/lkml/20220408223129.3844-1-fmdefrancesco@gmail.com/ https://lore.kernel.org/lkml/b0aed731-b56f-4378-b50e-fc0cbccbdb84@redhat.com/ Thanks, Fabio M. De Francesco P.S.: Do you want me to resend them all and add the above-mentioned tags?
diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c index 46ac082cd3f1..54188197c3dc 100644 --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c @@ -482,10 +482,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) idx = (virt - bo->start) >> PAGE_SHIFT; offset = (virt - bo->start) - (idx << PAGE_SHIFT); - if (in_atomic()) - des = (char *)kmap_atomic(bo->page_obj[idx].page); - else - des = (char *)kmap(bo->page_obj[idx].page); + des = (char *)kmap_local_page(bo->page_obj[idx].page); if (!des) { dev_err(atomisp_dev, @@ -512,14 +509,7 @@ int hmm_store(ia_css_ptr virt, const void *data, unsigned int bytes) clflush_cache_range(des, len); - if (in_atomic()) - /* - * Note: kunmap_atomic requires return addr from - * kmap_atomic, not the page. See linux/highmem.h - */ - kunmap_atomic(des - offset); - else - kunmap(bo->page_obj[idx].page); + kunmap_local(des); } return 0;
The use of kmap() is being deprecated in favor of kmap_local_page() where it is feasible. The same is true for kmap_atomic(). In file pci/hmm/hmm.c, function hmm_store() test if we are in atomic context and, if so, it calls kmap_atomic(), if not, it calls kmap(). First of all, in_atomic() shouldn't be used in drivers. This macro cannot always detect atomic context; in particular, it cannot know about held spinlocks in non-preemptible kernels. Notwithstanding what it is said above, this code doesn't need to care whether or not it is executing in atomic context. It can simply use kmap_local_page() / kunmap_local() that can instead do the mapping / unmapping regardless of the context. With kmap_local_page(), the mapping is per thread, CPU local and not globally visible. Therefore, hmm_store()() is a function where the use of kmap_local_page() in place of both kmap() and kmap_atomic() is correctly suited. Convert the calls of kmap() / kunmap() and kmap_atomic() / kunmap_atomic() to kmap_local_page() / kunmap_local() and drop the unnecessary tests which test if the code is in atomic context. Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- drivers/staging/media/atomisp/pci/hmm/hmm.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)