[PATCH] swiotlb: Clean up some coding style and minor issues

[PATCH] swiotlb: Clean up some coding style and minor issues

[Tianyu Lan]

From: Tianyu Lan <tiala@microsoft.com>

- Fix the used field of struct io_tlb_area wasn't initialized
- Set area number to be 0 if input area number parameter is 0
- Use array_size() to calculate io_tlb_area array size
- Fix error handle of io_tlb_used debugfs node and introduce
  fops_io_tlb_used attribute
- Make parameters of swiotlb_do_find_slots() more reasonable

Fixes: 26ffb91fa5e0 ("swiotlb: split up the global swiotlb lock")
Signed-off-by: Tianyu Lan <tiala@microsoft.com>
---
 .../admin-guide/kernel-parameters.txt         |  3 +-
 kernel/dma/swiotlb.c                          | 42 ++++++++++++-------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 4a6ad177d4b8..ddca09550f76 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5907,7 +5907,8 @@
 			Format: { <int> [,<int>] | force | noforce }
 			<int> -- Number of I/O TLB slabs
 			<int> -- Second integer after comma. Number of swiotlb
-				 areas with their own lock. Must be power of 2.
+				 areas with their own lock. Will be rounded up
+				 to a power of 2.
 			force -- force using of bounce buffers even if they
 			         wouldn't be automatically used by the kernel
 			noforce -- Never use bounce buffers (for debugging)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c39483bf067d..5752db98a1f2 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -96,7 +96,13 @@ struct io_tlb_slot {
 
 static void swiotlb_adjust_nareas(unsigned int nareas)
 {
-	if (!is_power_of_2(nareas))
+	/*
+	 * Set area number to 1 when input area number
+	 * is zero.
+	 */
+	if (!nareas)
+		nareas  = 1;
+	else if (!is_power_of_2(nareas))
 		nareas = roundup_pow_of_two(nareas);
 
 	default_nareas = nareas;
@@ -270,6 +276,7 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
 	for (i = 0; i < mem->nareas; i++) {
 		spin_lock_init(&mem->areas[i].lock);
 		mem->areas[i].index = 0;
+		mem->areas[i].used = 0;
 	}
 
 	for (i = 0; i < mem->nslabs; i++) {
@@ -353,8 +360,8 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned int flags,
 		panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
 		      __func__, alloc_size, PAGE_SIZE);
 
-	mem->areas = memblock_alloc(sizeof(struct io_tlb_area) *
-		default_nareas, SMP_CACHE_BYTES);
+	mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
+		default_nareas), SMP_CACHE_BYTES);
 	if (!mem->areas)
 		panic("%s: Failed to allocate mem->areas.\n", __func__);
 
@@ -479,7 +486,7 @@ void __init swiotlb_exit(void)
 		free_pages((unsigned long)mem->slots, get_order(slots_size));
 	} else {
 		memblock_free_late(__pa(mem->areas),
-				   mem->nareas * sizeof(struct io_tlb_area));
+				   array_size(sizeof(*mem->areas), mem->nareas));
 		memblock_free_late(mem->start, tbl_size);
 		memblock_free_late(__pa(mem->slots), slots_size);
 	}
@@ -593,11 +600,12 @@ static unsigned int wrap_area_index(struct io_tlb_mem *mem, unsigned int index)
  * Find a suitable number of IO TLB entries size that will fit this request and
  * allocate a buffer from that IO TLB pool.
  */
-static int swiotlb_do_find_slots(struct io_tlb_mem *mem,
-		struct io_tlb_area *area, int area_index,
-		struct device *dev, phys_addr_t orig_addr,
+static int swiotlb_do_find_slots(struct device *dev,
+		int area_index, phys_addr_t orig_addr,
 		size_t alloc_size, unsigned int alloc_align_mask)
 {
+	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
+	struct io_tlb_area *area = mem->areas + area_index;
 	unsigned long boundary_mask = dma_get_seg_boundary(dev);
 	dma_addr_t tbl_dma_addr =
 		phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
@@ -686,13 +694,12 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t orig_addr,
 		size_t alloc_size, unsigned int alloc_align_mask)
 {
 	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
-	int start = raw_smp_processor_id() & ((1U << __fls(mem->nareas)) - 1);
+	int start = raw_smp_processor_id() & (mem->nareas - 1);
 	int i = start, index;
 
 	do {
-		index = swiotlb_do_find_slots(mem, mem->areas + i, i,
-					      dev, orig_addr, alloc_size,
-					      alloc_align_mask);
+		index = swiotlb_do_find_slots(dev, i, orig_addr,
+					      alloc_size, alloc_align_mask);
 		if (index >= 0)
 			return index;
 		if (++i >= mem->nareas)
@@ -903,17 +910,24 @@ bool is_swiotlb_active(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(is_swiotlb_active);
 
+static int io_tlb_used_get(void *data, u64 *val)
+{
+	*val = mem_used(&io_tlb_default_mem);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_io_tlb_used, io_tlb_used_get, NULL, "%llu\n");
+
 static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem,
 					 const char *dirname)
 {
-	unsigned long used = mem_used(mem);
-
 	mem->debugfs = debugfs_create_dir(dirname, io_tlb_default_mem.debugfs);
 	if (!mem->nslabs)
 		return;
 
 	debugfs_create_ulong("io_tlb_nslabs", 0400, mem->debugfs, &mem->nslabs);
-	debugfs_create_ulong("io_tlb_used", 0400, mem->debugfs, &used);
+	debugfs_create_file_unsafe("io_tlb_used", 0400, mem->debugfs, NULL,
+			&fops_io_tlb_used);
 }
 
 static int __init __maybe_unused swiotlb_create_default_debugfs(void)
-- 
2.25.1

RE: [PATCH] swiotlb: Clean up some coding style and minor issues

[Michael Kelley (LINUX)]

From: Tianyu Lan <ltykernel@gmail.com> Sent: Thursday, July 21, 2022 8:39 PM
> 
> - Fix the used field of struct io_tlb_area wasn't initialized
> - Set area number to be 0 if input area number parameter is 0
> - Use array_size() to calculate io_tlb_area array size
> - Fix error handle of io_tlb_used debugfs node and introduce
>   fops_io_tlb_used attribute
> - Make parameters of swiotlb_do_find_slots() more reasonable
> 

I think you missed one of the bugs I pointed out in my previous
comments.  In the function rmem_swiotlb_device_init(), the two
calls to kfree() in the error path are in the wrong order.   It's a
path that will probably never happen, but it still should be fixed.

The other fixes look good to me.

Michael

> Fixes: 26ffb91fa5e0 ("swiotlb: split up the global swiotlb lock")
> Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  3 +-
>  kernel/dma/swiotlb.c                          | 42 ++++++++++++-------
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index 4a6ad177d4b8..ddca09550f76 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5907,7 +5907,8 @@
>  			Format: { <int> [,<int>] | force | noforce }
>  			<int> -- Number of I/O TLB slabs
>  			<int> -- Second integer after comma. Number of swiotlb
> -				 areas with their own lock. Must be power of 2.
> +				 areas with their own lock. Will be rounded up
> +				 to a power of 2.
>  			force -- force using of bounce buffers even if they
>  			         wouldn't be automatically used by the kernel
>  			noforce -- Never use bounce buffers (for debugging)
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c39483bf067d..5752db98a1f2 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -96,7 +96,13 @@ struct io_tlb_slot {
> 
>  static void swiotlb_adjust_nareas(unsigned int nareas)
>  {
> -	if (!is_power_of_2(nareas))
> +	/*
> +	 * Set area number to 1 when input area number
> +	 * is zero.
> +	 */
> +	if (!nareas)
> +		nareas  = 1;
> +	else if (!is_power_of_2(nareas))
>  		nareas = roundup_pow_of_two(nareas);
> 
>  	default_nareas = nareas;
> @@ -270,6 +276,7 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem,
> phys_addr_t start,
>  	for (i = 0; i < mem->nareas; i++) {
>  		spin_lock_init(&mem->areas[i].lock);
>  		mem->areas[i].index = 0;
> +		mem->areas[i].used = 0;
>  	}
> 
>  	for (i = 0; i < mem->nslabs; i++) {
> @@ -353,8 +360,8 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned
> int flags,
>  		panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
>  		      __func__, alloc_size, PAGE_SIZE);
> 
> -	mem->areas = memblock_alloc(sizeof(struct io_tlb_area) *
> -		default_nareas, SMP_CACHE_BYTES);
> +	mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
> +		default_nareas), SMP_CACHE_BYTES);
>  	if (!mem->areas)
>  		panic("%s: Failed to allocate mem->areas.\n", __func__);
> 
> @@ -479,7 +486,7 @@ void __init swiotlb_exit(void)
>  		free_pages((unsigned long)mem->slots, get_order(slots_size));
>  	} else {
>  		memblock_free_late(__pa(mem->areas),
> -				   mem->nareas * sizeof(struct io_tlb_area));
> +				   array_size(sizeof(*mem->areas), mem->nareas));
>  		memblock_free_late(mem->start, tbl_size);
>  		memblock_free_late(__pa(mem->slots), slots_size);
>  	}
> @@ -593,11 +600,12 @@ static unsigned int wrap_area_index(struct io_tlb_mem
> *mem, unsigned int index)
>   * Find a suitable number of IO TLB entries size that will fit this request and
>   * allocate a buffer from that IO TLB pool.
>   */
> -static int swiotlb_do_find_slots(struct io_tlb_mem *mem,
> -		struct io_tlb_area *area, int area_index,
> -		struct device *dev, phys_addr_t orig_addr,
> +static int swiotlb_do_find_slots(struct device *dev,
> +		int area_index, phys_addr_t orig_addr,
>  		size_t alloc_size, unsigned int alloc_align_mask)
>  {
> +	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> +	struct io_tlb_area *area = mem->areas + area_index;
>  	unsigned long boundary_mask = dma_get_seg_boundary(dev);
>  	dma_addr_t tbl_dma_addr =
>  		phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> @@ -686,13 +694,12 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t
> orig_addr,
>  		size_t alloc_size, unsigned int alloc_align_mask)
>  {
>  	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> -	int start = raw_smp_processor_id() & ((1U << __fls(mem->nareas)) - 1);
> +	int start = raw_smp_processor_id() & (mem->nareas - 1);
>  	int i = start, index;
> 
>  	do {
> -		index = swiotlb_do_find_slots(mem, mem->areas + i, i,
> -					      dev, orig_addr, alloc_size,
> -					      alloc_align_mask);
> +		index = swiotlb_do_find_slots(dev, i, orig_addr,
> +					      alloc_size, alloc_align_mask);
>  		if (index >= 0)
>  			return index;
>  		if (++i >= mem->nareas)
> @@ -903,17 +910,24 @@ bool is_swiotlb_active(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(is_swiotlb_active);
> 
> +static int io_tlb_used_get(void *data, u64 *val)
> +{
> +	*val = mem_used(&io_tlb_default_mem);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_io_tlb_used, io_tlb_used_get, NULL, "%llu\n");
> +
>  static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem,
>  					 const char *dirname)
>  {
> -	unsigned long used = mem_used(mem);
> -
>  	mem->debugfs = debugfs_create_dir(dirname, io_tlb_default_mem.debugfs);
>  	if (!mem->nslabs)
>  		return;
> 
>  	debugfs_create_ulong("io_tlb_nslabs", 0400, mem->debugfs, &mem->nslabs);
> -	debugfs_create_ulong("io_tlb_used", 0400, mem->debugfs, &used);
> +	debugfs_create_file_unsafe("io_tlb_used", 0400, mem->debugfs, NULL,
> +			&fops_io_tlb_used);
>  }
> 
>  static int __init __maybe_unused swiotlb_create_default_debugfs(void)
> --
> 2.25.1

Re: [PATCH] swiotlb: Clean up some coding style and minor issues

[hch]

On Fri, Jul 22, 2022 at 04:03:21AM +0000, Michael Kelley (LINUX) wrote:
> I think you missed one of the bugs I pointed out in my previous
> comments.  In the function rmem_swiotlb_device_init(), the two
> calls to kfree() in the error path are in the wrong order.   It's a
> path that will probably never happen, but it still should be fixed.

That is already fixed in the dma-mapping tree:

http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/4a97739474c402e0a14cf6a432f1920262f6811c

> The other fixes look good to me.

Can you make that a formal Reviewed-by?

> 
> Michael
> 
> > Fixes: 26ffb91fa5e0 ("swiotlb: split up the global swiotlb lock")
> > Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> > ---
> >  .../admin-guide/kernel-parameters.txt         |  3 +-
> >  kernel/dma/swiotlb.c                          | 42 ++++++++++++-------
> >  2 files changed, 30 insertions(+), 15 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 4a6ad177d4b8..ddca09550f76 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -5907,7 +5907,8 @@
> >  			Format: { <int> [,<int>] | force | noforce }
> >  			<int> -- Number of I/O TLB slabs
> >  			<int> -- Second integer after comma. Number of swiotlb
> > -				 areas with their own lock. Must be power of 2.
> > +				 areas with their own lock. Will be rounded up
> > +				 to a power of 2.
> >  			force -- force using of bounce buffers even if they
> >  			         wouldn't be automatically used by the kernel
> >  			noforce -- Never use bounce buffers (for debugging)
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c39483bf067d..5752db98a1f2 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -96,7 +96,13 @@ struct io_tlb_slot {
> > 
> >  static void swiotlb_adjust_nareas(unsigned int nareas)
> >  {
> > -	if (!is_power_of_2(nareas))
> > +	/*
> > +	 * Set area number to 1 when input area number
> > +	 * is zero.
> > +	 */
> > +	if (!nareas)
> > +		nareas  = 1;
> > +	else if (!is_power_of_2(nareas))
> >  		nareas = roundup_pow_of_two(nareas);
> > 
> >  	default_nareas = nareas;
> > @@ -270,6 +276,7 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem,
> > phys_addr_t start,
> >  	for (i = 0; i < mem->nareas; i++) {
> >  		spin_lock_init(&mem->areas[i].lock);
> >  		mem->areas[i].index = 0;
> > +		mem->areas[i].used = 0;
> >  	}
> > 
> >  	for (i = 0; i < mem->nslabs; i++) {
> > @@ -353,8 +360,8 @@ void __init swiotlb_init_remap(bool addressing_limit, unsigned
> > int flags,
> >  		panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> >  		      __func__, alloc_size, PAGE_SIZE);
> > 
> > -	mem->areas = memblock_alloc(sizeof(struct io_tlb_area) *
> > -		default_nareas, SMP_CACHE_BYTES);
> > +	mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
> > +		default_nareas), SMP_CACHE_BYTES);
> >  	if (!mem->areas)
> >  		panic("%s: Failed to allocate mem->areas.\n", __func__);
> > 
> > @@ -479,7 +486,7 @@ void __init swiotlb_exit(void)
> >  		free_pages((unsigned long)mem->slots, get_order(slots_size));
> >  	} else {
> >  		memblock_free_late(__pa(mem->areas),
> > -				   mem->nareas * sizeof(struct io_tlb_area));
> > +				   array_size(sizeof(*mem->areas), mem->nareas));
> >  		memblock_free_late(mem->start, tbl_size);
> >  		memblock_free_late(__pa(mem->slots), slots_size);
> >  	}
> > @@ -593,11 +600,12 @@ static unsigned int wrap_area_index(struct io_tlb_mem
> > *mem, unsigned int index)
> >   * Find a suitable number of IO TLB entries size that will fit this request and
> >   * allocate a buffer from that IO TLB pool.
> >   */
> > -static int swiotlb_do_find_slots(struct io_tlb_mem *mem,
> > -		struct io_tlb_area *area, int area_index,
> > -		struct device *dev, phys_addr_t orig_addr,
> > +static int swiotlb_do_find_slots(struct device *dev,
> > +		int area_index, phys_addr_t orig_addr,
> >  		size_t alloc_size, unsigned int alloc_align_mask)
> >  {
> > +	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > +	struct io_tlb_area *area = mem->areas + area_index;
> >  	unsigned long boundary_mask = dma_get_seg_boundary(dev);
> >  	dma_addr_t tbl_dma_addr =
> >  		phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> > @@ -686,13 +694,12 @@ static int swiotlb_find_slots(struct device *dev, phys_addr_t
> > orig_addr,
> >  		size_t alloc_size, unsigned int alloc_align_mask)
> >  {
> >  	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > -	int start = raw_smp_processor_id() & ((1U << __fls(mem->nareas)) - 1);
> > +	int start = raw_smp_processor_id() & (mem->nareas - 1);
> >  	int i = start, index;
> > 
> >  	do {
> > -		index = swiotlb_do_find_slots(mem, mem->areas + i, i,
> > -					      dev, orig_addr, alloc_size,
> > -					      alloc_align_mask);
> > +		index = swiotlb_do_find_slots(dev, i, orig_addr,
> > +					      alloc_size, alloc_align_mask);
> >  		if (index >= 0)
> >  			return index;
> >  		if (++i >= mem->nareas)
> > @@ -903,17 +910,24 @@ bool is_swiotlb_active(struct device *dev)
> >  }
> >  EXPORT_SYMBOL_GPL(is_swiotlb_active);
> > 
> > +static int io_tlb_used_get(void *data, u64 *val)
> > +{
> > +	*val = mem_used(&io_tlb_default_mem);
> > +
> > +	return 0;
> > +}
> > +DEFINE_DEBUGFS_ATTRIBUTE(fops_io_tlb_used, io_tlb_used_get, NULL, "%llu\n");
> > +
> >  static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem,
> >  					 const char *dirname)
> >  {
> > -	unsigned long used = mem_used(mem);
> > -
> >  	mem->debugfs = debugfs_create_dir(dirname, io_tlb_default_mem.debugfs);
> >  	if (!mem->nslabs)
> >  		return;
> > 
> >  	debugfs_create_ulong("io_tlb_nslabs", 0400, mem->debugfs, &mem->nslabs);
> > -	debugfs_create_ulong("io_tlb_used", 0400, mem->debugfs, &used);
> > +	debugfs_create_file_unsafe("io_tlb_used", 0400, mem->debugfs, NULL,
> > +			&fops_io_tlb_used);
> >  }
> > 
> >  static int __init __maybe_unused swiotlb_create_default_debugfs(void)
> > --
> > 2.25.1
> 
---end quoted text---

RE: [PATCH] swiotlb: Clean up some coding style and minor issues

[Michael Kelley (LINUX)]

From: hch@infradead.org <hch@infradead.org> Sent: Thursday, July 21, 2022 10:05 PM
> 
> On Fri, Jul 22, 2022 at 04:03:21AM +0000, Michael Kelley (LINUX) wrote:
> > I think you missed one of the bugs I pointed out in my previous
> > comments.  In the function rmem_swiotlb_device_init(), the two
> > calls to kfree() in the error path are in the wrong order.   It's a
> > path that will probably never happen, but it still should be fixed.
> 
> That is already fixed in the dma-mapping tree:
> 
> http://git.infradead.org/users/hch/dma-mapping.git/commitdiff/4a97739474c402e0a14cf6a432f1920262f6811c

Indeed.

> 
> > The other fixes look good to me.
> 
> Can you make that a formal Reviewed-by?

Yes.

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

> 
> >
> > Michael
> >
> > > Fixes: 26ffb91fa5e0 ("swiotlb: split up the global swiotlb lock")
> > > Signed-off-by: Tianyu Lan <tiala@microsoft.com>
> > > ---
> > >  .../admin-guide/kernel-parameters.txt         |  3 +-
> > >  kernel/dma/swiotlb.c                          | 42 ++++++++++++-------
> > >  2 files changed, 30 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt
> > > b/Documentation/admin-guide/kernel-parameters.txt
> > > index 4a6ad177d4b8..ddca09550f76 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -5907,7 +5907,8 @@
> > >  			Format: { <int> [,<int>] | force | noforce }
> > >  			<int> -- Number of I/O TLB slabs
> > >  			<int> -- Second integer after comma. Number of swiotlb
> > > -				 areas with their own lock. Must be power of 2.
> > > +				 areas with their own lock. Will be rounded up
> > > +				 to a power of 2.
> > >  			force -- force using of bounce buffers even if they
> > >  			         wouldn't be automatically used by the kernel
> > >  			noforce -- Never use bounce buffers (for debugging)
> > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > > index c39483bf067d..5752db98a1f2 100644
> > > --- a/kernel/dma/swiotlb.c
> > > +++ b/kernel/dma/swiotlb.c
> > > @@ -96,7 +96,13 @@ struct io_tlb_slot {
> > >
> > >  static void swiotlb_adjust_nareas(unsigned int nareas)
> > >  {
> > > -	if (!is_power_of_2(nareas))
> > > +	/*
> > > +	 * Set area number to 1 when input area number
> > > +	 * is zero.
> > > +	 */
> > > +	if (!nareas)
> > > +		nareas  = 1;
> > > +	else if (!is_power_of_2(nareas))
> > >  		nareas = roundup_pow_of_two(nareas);
> > >
> > >  	default_nareas = nareas;
> > > @@ -270,6 +276,7 @@ static void swiotlb_init_io_tlb_mem(struct io_tlb_mem
> *mem,
> > > phys_addr_t start,
> > >  	for (i = 0; i < mem->nareas; i++) {
> > >  		spin_lock_init(&mem->areas[i].lock);
> > >  		mem->areas[i].index = 0;
> > > +		mem->areas[i].used = 0;
> > >  	}
> > >
> > >  	for (i = 0; i < mem->nslabs; i++) {
> > > @@ -353,8 +360,8 @@ void __init swiotlb_init_remap(bool addressing_limit,
> unsigned
> > > int flags,
> > >  		panic("%s: Failed to allocate %zu bytes align=0x%lx\n",
> > >  		      __func__, alloc_size, PAGE_SIZE);
> > >
> > > -	mem->areas = memblock_alloc(sizeof(struct io_tlb_area) *
> > > -		default_nareas, SMP_CACHE_BYTES);
> > > +	mem->areas = memblock_alloc(array_size(sizeof(struct io_tlb_area),
> > > +		default_nareas), SMP_CACHE_BYTES);
> > >  	if (!mem->areas)
> > >  		panic("%s: Failed to allocate mem->areas.\n", __func__);
> > >
> > > @@ -479,7 +486,7 @@ void __init swiotlb_exit(void)
> > >  		free_pages((unsigned long)mem->slots, get_order(slots_size));
> > >  	} else {
> > >  		memblock_free_late(__pa(mem->areas),
> > > -				   mem->nareas * sizeof(struct io_tlb_area));
> > > +				   array_size(sizeof(*mem->areas), mem->nareas));
> > >  		memblock_free_late(mem->start, tbl_size);
> > >  		memblock_free_late(__pa(mem->slots), slots_size);
> > >  	}
> > > @@ -593,11 +600,12 @@ static unsigned int wrap_area_index(struct io_tlb_mem
> > > *mem, unsigned int index)
> > >   * Find a suitable number of IO TLB entries size that will fit this request and
> > >   * allocate a buffer from that IO TLB pool.
> > >   */
> > > -static int swiotlb_do_find_slots(struct io_tlb_mem *mem,
> > > -		struct io_tlb_area *area, int area_index,
> > > -		struct device *dev, phys_addr_t orig_addr,
> > > +static int swiotlb_do_find_slots(struct device *dev,
> > > +		int area_index, phys_addr_t orig_addr,
> > >  		size_t alloc_size, unsigned int alloc_align_mask)
> > >  {
> > > +	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > > +	struct io_tlb_area *area = mem->areas + area_index;
> > >  	unsigned long boundary_mask = dma_get_seg_boundary(dev);
> > >  	dma_addr_t tbl_dma_addr =
> > >  		phys_to_dma_unencrypted(dev, mem->start) & boundary_mask;
> > > @@ -686,13 +694,12 @@ static int swiotlb_find_slots(struct device *dev,
> phys_addr_t
> > > orig_addr,
> > >  		size_t alloc_size, unsigned int alloc_align_mask)
> > >  {
> > >  	struct io_tlb_mem *mem = dev->dma_io_tlb_mem;
> > > -	int start = raw_smp_processor_id() & ((1U << __fls(mem->nareas)) - 1);
> > > +	int start = raw_smp_processor_id() & (mem->nareas - 1);
> > >  	int i = start, index;
> > >
> > >  	do {
> > > -		index = swiotlb_do_find_slots(mem, mem->areas + i, i,
> > > -					      dev, orig_addr, alloc_size,
> > > -					      alloc_align_mask);
> > > +		index = swiotlb_do_find_slots(dev, i, orig_addr,
> > > +					      alloc_size, alloc_align_mask);
> > >  		if (index >= 0)
> > >  			return index;
> > >  		if (++i >= mem->nareas)
> > > @@ -903,17 +910,24 @@ bool is_swiotlb_active(struct device *dev)
> > >  }
> > >  EXPORT_SYMBOL_GPL(is_swiotlb_active);
> > >
> > > +static int io_tlb_used_get(void *data, u64 *val)
> > > +{
> > > +	*val = mem_used(&io_tlb_default_mem);
> > > +
> > > +	return 0;
> > > +}
> > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_io_tlb_used, io_tlb_used_get, NULL, "%llu\n");
> > > +
> > >  static void swiotlb_create_debugfs_files(struct io_tlb_mem *mem,
> > >  					 const char *dirname)
> > >  {
> > > -	unsigned long used = mem_used(mem);
> > > -
> > >  	mem->debugfs = debugfs_create_dir(dirname, io_tlb_default_mem.debugfs);
> > >  	if (!mem->nslabs)
> > >  		return;
> > >
> > >  	debugfs_create_ulong("io_tlb_nslabs", 0400, mem->debugfs, &mem->nslabs);
> > > -	debugfs_create_ulong("io_tlb_used", 0400, mem->debugfs, &used);
> > > +	debugfs_create_file_unsafe("io_tlb_used", 0400, mem->debugfs, NULL,
> > > +			&fops_io_tlb_used);
> > >  }
> > >
> > >  static int __init __maybe_unused swiotlb_create_default_debugfs(void)
> > > --
> > > 2.25.1
> >
> ---end quoted text---

Re: [PATCH] swiotlb: Clean up some coding style and minor issues

[Christoph Hellwig]

I've applied this, but then dropped the debugfs changes.  I can't find
any good reason why we'd use the _unsafe variant here.  Can you resend
that bit and write a commit log clearly documenting the considerations
for it?

Re: [PATCH] swiotlb: Clean up some coding style and minor issues

[Tianyu Lan]

On 7/22/2022 11:22 PM, Christoph Hellwig wrote:
> I've applied this, but then dropped the debugfs changes.  I can't find
> any good reason why we'd use the _unsafe variant here.  Can you resend
> that bit and write a commit log clearly documenting the considerations
> for it?

Sure. I will do that.