From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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;
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 Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> 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 <[email protected]> wrote:
>
> On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
> > 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 <[email protected]> wrote:
> >
> > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> >
> > > 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 <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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/[email protected]/T/#m7f97e10dbfde777f58493398a77933e6a2f3c15d
> >
> > Bart
> >
> > > Otherwise,
> > >
> > > Reviewed-by: Andrew Halaney <[email protected]>
> > >
> > > > 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 <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Mon, Oct 09, 2023 at 05:34:19PM +0200, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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/[email protected]/T/#m7f97e10dbfde777f58493398a77933e6a2f3c15d
>
> > >
> > > Bart
> > >
> > > > Otherwise,
> > > >
> > > > Reviewed-by: Andrew Halaney <[email protected]>
> > > >
> > > > > 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
> > > > >
> > > >
> > >
> >
>