| Message ID | 20231009153427.20951-8-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Let's use the new TZ memory allocator to obtain a buffer for this call > instead of using dma_alloc_coherent(). > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > index 71e98b666391..754f6056b99f 100644 > --- a/drivers/firmware/qcom/qcom_scm.c > +++ b/drivers/firmware/qcom/qcom_scm.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/arm-smccc.h> > +#include <linux/cleanup.h> > #include <linux/clk.h> > #include <linux/completion.h> > #include <linux/cpumask.h> > @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > struct qcom_scm_mem_map_info *mem_to_map; > phys_addr_t mem_to_map_phys; > phys_addr_t dest_phys; > - dma_addr_t ptr_phys; > + phys_addr_t ptr_phys; > size_t mem_to_map_sz; > size_t dest_sz; > size_t src_sz; > size_t ptr_sz; > int next_vm; > __le32 *src; > - void *ptr; nit: couldn't you keep this up here? Otherwise, Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > int ret, i, b; > u64 srcvm_bits = *srcvm; > > @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + > ALIGN(dest_sz, SZ_64); > > - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > + ptr_sz, GFP_KERNEL); > if (!ptr) > return -ENOMEM; > > + ptr_phys = qcom_tzmem_to_phys(ptr); > + > /* Fill source vmid detail */ > src = ptr; > i = 0; > @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, > ptr_phys, src_sz, dest_phys, dest_sz); > - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); > if (ret) { > dev_err(__scm->dev, > "Assign memory protection call failed %d\n", ret); > -- > 2.39.2 >
On Wed, Oct 11, 2023 at 12:19 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Let's use the new TZ memory allocator to obtain a buffer for this call > > instead of using dma_alloc_coherent(). > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > index 71e98b666391..754f6056b99f 100644 > > --- a/drivers/firmware/qcom/qcom_scm.c > > +++ b/drivers/firmware/qcom/qcom_scm.c > > @@ -4,6 +4,7 @@ > > */ > > > > #include <linux/arm-smccc.h> > > +#include <linux/cleanup.h> > > #include <linux/clk.h> > > #include <linux/completion.h> > > #include <linux/cpumask.h> > > @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > struct qcom_scm_mem_map_info *mem_to_map; > > phys_addr_t mem_to_map_phys; > > phys_addr_t dest_phys; > > - dma_addr_t ptr_phys; > > + phys_addr_t ptr_phys; > > size_t mem_to_map_sz; > > size_t dest_sz; > > size_t src_sz; > > size_t ptr_sz; > > int next_vm; > > __le32 *src; > > - void *ptr; > > nit: couldn't you keep this up here? > This still needs to make its way into the coding style guide but I got yelled at by Linus Torvalds personally for not declaring the managed variables where they are initialized. So this is the correct approach. Bart > Otherwise, > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > int ret, i, b; > > u64 srcvm_bits = *srcvm; > > > > @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + > > ALIGN(dest_sz, SZ_64); > > > > - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); > > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > > + ptr_sz, GFP_KERNEL); > > if (!ptr) > > return -ENOMEM; > > > > + ptr_phys = qcom_tzmem_to_phys(ptr); > > + > > /* Fill source vmid detail */ > > src = ptr; > > i = 0; > > @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, > > ptr_phys, src_sz, dest_phys, dest_sz); > > - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); > > if (ret) { > > dev_err(__scm->dev, > > "Assign memory protection call failed %d\n", ret); > > -- > > 2.39.2 > > >
On Wed, Oct 11, 2023 at 09:41:49AM +0200, Bartosz Golaszewski wrote: > On Wed, Oct 11, 2023 at 12:19 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Let's use the new TZ memory allocator to obtain a buffer for this call > > > instead of using dma_alloc_coherent(). > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > index 71e98b666391..754f6056b99f 100644 > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > @@ -4,6 +4,7 @@ > > > */ > > > > > > #include <linux/arm-smccc.h> > > > +#include <linux/cleanup.h> > > > #include <linux/clk.h> > > > #include <linux/completion.h> > > > #include <linux/cpumask.h> > > > @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > struct qcom_scm_mem_map_info *mem_to_map; > > > phys_addr_t mem_to_map_phys; > > > phys_addr_t dest_phys; > > > - dma_addr_t ptr_phys; > > > + phys_addr_t ptr_phys; > > > size_t mem_to_map_sz; > > > size_t dest_sz; > > > size_t src_sz; > > > size_t ptr_sz; > > > int next_vm; > > > __le32 *src; > > > - void *ptr; > > > > nit: couldn't you keep this up here? > > > > This still needs to make its way into the coding style guide but I got > yelled at by Linus Torvalds personally for not declaring the managed > variables where they are initialized. So this is the correct approach. I'm being a stick in the mud, but couldn't you initialize to NULL and keep them all up top? That seems more in line with the current "declare all variables at the start of function" guideline the kernel follows. Not a big deal... yours call! but /me shrugs > > Bart > > > Otherwise, > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > > > int ret, i, b; > > > u64 srcvm_bits = *srcvm; > > > > > > @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + > > > ALIGN(dest_sz, SZ_64); > > > > > > - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); > > > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > > > + ptr_sz, GFP_KERNEL); > > > if (!ptr) > > > return -ENOMEM; > > > > > > + ptr_phys = qcom_tzmem_to_phys(ptr); > > > + > > > /* Fill source vmid detail */ > > > src = ptr; > > > i = 0; > > > @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > > > ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, > > > ptr_phys, src_sz, dest_phys, dest_sz); > > > - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); > > > if (ret) { > > > dev_err(__scm->dev, > > > "Assign memory protection call failed %d\n", ret); > > > -- > > > 2.39.2 > > > > > >
On Wed, Oct 11, 2023 at 3:54 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > On Wed, Oct 11, 2023 at 09:41:49AM +0200, Bartosz Golaszewski wrote: > > On Wed, Oct 11, 2023 at 12:19 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Let's use the new TZ memory allocator to obtain a buffer for this call > > > > instead of using dma_alloc_coherent(). > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > --- > > > > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > > index 71e98b666391..754f6056b99f 100644 > > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > > @@ -4,6 +4,7 @@ > > > > */ > > > > > > > > #include <linux/arm-smccc.h> > > > > +#include <linux/cleanup.h> > > > > #include <linux/clk.h> > > > > #include <linux/completion.h> > > > > #include <linux/cpumask.h> > > > > @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > struct qcom_scm_mem_map_info *mem_to_map; > > > > phys_addr_t mem_to_map_phys; > > > > phys_addr_t dest_phys; > > > > - dma_addr_t ptr_phys; > > > > + phys_addr_t ptr_phys; > > > > size_t mem_to_map_sz; > > > > size_t dest_sz; > > > > size_t src_sz; > > > > size_t ptr_sz; > > > > int next_vm; > > > > __le32 *src; > > > > - void *ptr; > > > > > > nit: couldn't you keep this up here? > > > > > > > This still needs to make its way into the coding style guide but I got > > yelled at by Linus Torvalds personally for not declaring the managed > > variables where they are initialized. So this is the correct approach. > > I'm being a stick in the mud, but couldn't you initialize to NULL and > keep them all up top? That seems more in line with the current "declare > all variables at the start of function" guideline the kernel follows. > > Not a big deal... yours call! but /me shrugs > I agree with you but it's not my call to make. Please see[1]. Bartosz [1] https://lore.kernel.org/lkml/20230919193516.GA20937@noisy.programming.kicks-ass.net/T/#m7f97e10dbfde777f58493398a77933e6a2f3c15d > > > > Bart > > > > > Otherwise, > > > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > > > > > int ret, i, b; > > > > u64 srcvm_bits = *srcvm; > > > > > > > > @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + > > > > ALIGN(dest_sz, SZ_64); > > > > > > > > - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); > > > > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > > > > + ptr_sz, GFP_KERNEL); > > > > if (!ptr) > > > > return -ENOMEM; > > > > > > > > + ptr_phys = qcom_tzmem_to_phys(ptr); > > > > + > > > > /* Fill source vmid detail */ > > > > src = ptr; > > > > i = 0; > > > > @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > > > > > ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, > > > > ptr_phys, src_sz, dest_phys, dest_sz); > > > > - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); > > > > if (ret) { > > > > dev_err(__scm->dev, > > > > "Assign memory protection call failed %d\n", ret); > > > > -- > > > > 2.39.2 > > > > > > > > > >
On Wed, Oct 11, 2023 at 04:33:53PM +0200, Bartosz Golaszewski wrote: > On Wed, Oct 11, 2023 at 3:54 PM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > On Wed, Oct 11, 2023 at 09:41:49AM +0200, Bartosz Golaszewski wrote: > > > On Wed, Oct 11, 2023 at 12:19 AM Andrew Halaney <ahalaney@redhat.com> wrote: > > > > > > > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > > > Let's use the new TZ memory allocator to obtain a buffer for this call > > > > > instead of using dma_alloc_coherent(). > > > > > > > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > --- > > > > > drivers/firmware/qcom/qcom_scm.c | 10 ++++++---- > > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c > > > > > index 71e98b666391..754f6056b99f 100644 > > > > > --- a/drivers/firmware/qcom/qcom_scm.c > > > > > +++ b/drivers/firmware/qcom/qcom_scm.c > > > > > @@ -4,6 +4,7 @@ > > > > > */ > > > > > > > > > > #include <linux/arm-smccc.h> > > > > > +#include <linux/cleanup.h> > > > > > #include <linux/clk.h> > > > > > #include <linux/completion.h> > > > > > #include <linux/cpumask.h> > > > > > @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > > struct qcom_scm_mem_map_info *mem_to_map; > > > > > phys_addr_t mem_to_map_phys; > > > > > phys_addr_t dest_phys; > > > > > - dma_addr_t ptr_phys; > > > > > + phys_addr_t ptr_phys; > > > > > size_t mem_to_map_sz; > > > > > size_t dest_sz; > > > > > size_t src_sz; > > > > > size_t ptr_sz; > > > > > int next_vm; > > > > > __le32 *src; > > > > > - void *ptr; > > > > > > > > nit: couldn't you keep this up here? > > > > > > > > > > This still needs to make its way into the coding style guide but I got > > > yelled at by Linus Torvalds personally for not declaring the managed > > > variables where they are initialized. So this is the correct approach. > > > > I'm being a stick in the mud, but couldn't you initialize to NULL and > > keep them all up top? That seems more in line with the current "declare > > all variables at the start of function" guideline the kernel follows. > > > > Not a big deal... yours call! but /me shrugs > > > > I agree with you but it's not my call to make. Please see[1]. > Yeah, I see you're following the guidance there (declare + initialize together unless there's a conditional, etc, preventing that) in this series. Thanks for the pointer. > Bartosz > > [1] https://lore.kernel.org/lkml/20230919193516.GA20937@noisy.programming.kicks-ass.net/T/#m7f97e10dbfde777f58493398a77933e6a2f3c15d > > > > > > > Bart > > > > > > > Otherwise, > > > > > > > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com> > > > > > > > > > int ret, i, b; > > > > > u64 srcvm_bits = *srcvm; > > > > > > > > > > @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > > ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + > > > > > ALIGN(dest_sz, SZ_64); > > > > > > > > > > - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); > > > > > + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, > > > > > + ptr_sz, GFP_KERNEL); > > > > > if (!ptr) > > > > > return -ENOMEM; > > > > > > > > > > + ptr_phys = qcom_tzmem_to_phys(ptr); > > > > > + > > > > > /* Fill source vmid detail */ > > > > > src = ptr; > > > > > i = 0; > > > > > @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, > > > > > > > > > > ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, > > > > > ptr_phys, src_sz, dest_phys, dest_sz); > > > > > - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); > > > > > if (ret) { > > > > > dev_err(__scm->dev, > > > > > "Assign memory protection call failed %d\n", ret); > > > > > -- > > > > > 2.39.2 > > > > > > > > > > > > > > >
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c index 71e98b666391..754f6056b99f 100644 --- a/drivers/firmware/qcom/qcom_scm.c +++ b/drivers/firmware/qcom/qcom_scm.c @@ -4,6 +4,7 @@ */ #include <linux/arm-smccc.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/completion.h> #include <linux/cpumask.h> @@ -998,14 +999,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, struct qcom_scm_mem_map_info *mem_to_map; phys_addr_t mem_to_map_phys; phys_addr_t dest_phys; - dma_addr_t ptr_phys; + phys_addr_t ptr_phys; size_t mem_to_map_sz; size_t dest_sz; size_t src_sz; size_t ptr_sz; int next_vm; __le32 *src; - void *ptr; int ret, i, b; u64 srcvm_bits = *srcvm; @@ -1015,10 +1015,13 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, ptr_sz = ALIGN(src_sz, SZ_64) + ALIGN(mem_to_map_sz, SZ_64) + ALIGN(dest_sz, SZ_64); - ptr = dma_alloc_coherent(__scm->dev, ptr_sz, &ptr_phys, GFP_KERNEL); + void *ptr __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool, + ptr_sz, GFP_KERNEL); if (!ptr) return -ENOMEM; + ptr_phys = qcom_tzmem_to_phys(ptr); + /* Fill source vmid detail */ src = ptr; i = 0; @@ -1047,7 +1050,6 @@ int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz, ret = __qcom_scm_assign_mem(__scm->dev, mem_to_map_phys, mem_to_map_sz, ptr_phys, src_sz, dest_phys, dest_sz); - dma_free_coherent(__scm->dev, ptr_sz, ptr, ptr_phys); if (ret) { dev_err(__scm->dev, "Assign memory protection call failed %d\n", ret);