[PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The only reason why swiotlb_init_with_tbl() can fail is an
allocation failure in the memblock_alloc() function. But
this function just calls panic() in case it can't fulfill
the request and never returns an error, therefore
swiotlb_init_with_tbl() also never actually returns an
error.

There are three call-sites of this function in the kernel.
All of them check for an error being returned, and two of
them call panic() itself in case the function returns an
error. The third call-site handles the error case and
doesn't crash the system. This is in the swiotlb_init()
function.

Since there is a call-site which handles an error and does
not crash the system, change the function to return
allocation failures to the caller.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 kernel/dma/swiotlb.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c873f9cc2146..4619d078c67c 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -203,12 +203,15 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
 	 * between io_tlb_start and io_tlb_end.
 	 */
-	io_tlb_list = memblock_alloc(
+	io_tlb_list = memblock_alloc_nopanic(
 				PAGE_ALIGN(io_tlb_nslabs * sizeof(int)),
 				PAGE_SIZE);
-	io_tlb_orig_addr = memblock_alloc(
+	io_tlb_orig_addr = memblock_alloc_nopanic(
 				PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)),
 				PAGE_SIZE);
+	if (io_tlb_list == NULL || io_tlb_orig_addr == NULL)
+		goto out_fail;
+
 	for (i = 0; i < io_tlb_nslabs; i++) {
 		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
 		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
@@ -219,7 +222,26 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 		swiotlb_print_info();
 
 	swiotlb_set_max_segment(io_tlb_nslabs << IO_TLB_SHIFT);
+
 	return 0;
+
+out_fail:
+	if (io_tlb_list)
+		memblock_free(io_tlb_list,
+			      PAGE_ALIGN(io_tlb_nslabs * sizeof(int)));
+
+	if (io_tlb_orig_addr)
+		memblock_free(io_tlb_orig_addr,
+			      PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
+
+	io_tlb_list      = NULL;
+	io_tlb_orig_addr = NULL;
+	io_tlb_end       = 0;
+	io_tlb_start     = 0;
+	io_tlb_nslabs    = 0;
+	max_segment      = 0;
+
+	return -ENOMEM;
 }
 
 /*
-- 
2.16.4

Re: [PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[Christoph Hellwig]

On Thu, Jan 31, 2019 at 05:24:24PM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> The only reason why swiotlb_init_with_tbl() can fail is an
> allocation failure in the memblock_alloc() function. But
> this function just calls panic() in case it can't fulfill
> the request and never returns an error, therefore
> swiotlb_init_with_tbl() also never actually returns an
> error.

Please use up all 72 characters per line available in the commit log
format.

> -	io_tlb_list = memblock_alloc(
> +	io_tlb_list = memblock_alloc_nopanic(
>  				PAGE_ALIGN(io_tlb_nslabs * sizeof(int)),
>  				PAGE_SIZE);

Mike just killed the _nopanic versions and made the normal ones not
panic.

Re: [PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[Joerg Roedel]

On Fri, Feb 01, 2019 at 09:12:08AM +0100, Christoph Hellwig wrote:
> On Thu, Jan 31, 2019 at 05:24:24PM +0100, Joerg Roedel wrote:
> > -	io_tlb_list = memblock_alloc(
> > +	io_tlb_list = memblock_alloc_nopanic(
> >  				PAGE_ALIGN(io_tlb_nslabs * sizeof(int)),
> >  				PAGE_SIZE);
> 
> Mike just killed the _nopanic versions and made the normal ones not
> panic.

This sounds like a change to break quite a couple of places in the
kernel. But okay, it just makes this hunk obsolete.


	Joerg

Re: [PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[Christoph Hellwig]

On Fri, Feb 01, 2019 at 05:50:29PM +0100, Joerg Roedel wrote:
> > Mike just killed the _nopanic versions and made the normal ones not
> > panic.
> 
> This sounds like a change to break quite a couple of places in the
> kernel. But okay, it just makes this hunk obsolete.

He also added either explicit panic calls or error handling (panic
in case of swiotlb):

https://patchwork.kernel.org/patch/10766115/

Re: [PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[Joerg Roedel]

On Fri, Feb 01, 2019 at 05:56:10PM +0100, Christoph Hellwig wrote:
> On Fri, Feb 01, 2019 at 05:50:29PM +0100, Joerg Roedel wrote:
> > > Mike just killed the _nopanic versions and made the normal ones not
> > > panic.
> > 
> > This sounds like a change to break quite a couple of places in the
> > kernel. But okay, it just makes this hunk obsolete.
> 
> He also added either explicit panic calls or error handling (panic
> in case of swiotlb):
> 
> https://patchwork.kernel.org/patch/10766115/

I see, are these patches queued somewhere? My patch needs to be based on
his changes.


	Joerg

Re: [PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[Christoph Hellwig]

On Fri, Feb 01, 2019 at 05:59:01PM +0100, Joerg Roedel wrote:
> > > This sounds like a change to break quite a couple of places in the
> > > kernel. But okay, it just makes this hunk obsolete.
> > 
> > He also added either explicit panic calls or error handling (panic
> > in case of swiotlb):
> > 
> > https://patchwork.kernel.org/patch/10766115/
> 
> I see, are these patches queued somewhere? My patch needs to be based on
> his changes.

I think akpm queued them up in -mm.  At least I got some mails claiming
patches from the series have been applied.

Re: [PATCH] swiotlb: Return error from swiotlb_init_with_tbl()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3943 bytes --]

Hi Joerg,

I love your patch! Perhaps something to improve:

[auto build test WARNING on swiotlb/linux-next]
[also build test WARNING on v5.0-rc4]
[cannot apply to next-20190201]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Joerg-Roedel/swiotlb-Return-error-from-swiotlb_init_with_tbl/20190203-013129
base:   https://git.kernel.org/pub/scm/linux/kernel/git/konrad/swiotlb.git linux-next
config: riscv-tinyconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.2.0 make.cross ARCH=riscv 

All warnings (new ones prefixed by >>):

   kernel/dma/swiotlb.c: In function 'swiotlb_init_with_tbl':
>> kernel/dma/swiotlb.c:230:17: warning: passing argument 1 of 'memblock_free' makes integer from pointer without a cast [-Wint-conversion]
      memblock_free(io_tlb_list,
                    ^~~~~~~~~~~
   In file included from kernel/dma/swiotlb.c:42:
   include/linux/memblock.h:123:31: note: expected 'phys_addr_t' {aka 'long long unsigned int'} but argument is of type 'unsigned int *'
    int memblock_free(phys_addr_t base, phys_addr_t size);
                      ~~~~~~~~~~~~^~~~
   kernel/dma/swiotlb.c:234:17: warning: passing argument 1 of 'memblock_free' makes integer from pointer without a cast [-Wint-conversion]
      memblock_free(io_tlb_orig_addr,
                    ^~~~~~~~~~~~~~~~
   In file included from kernel/dma/swiotlb.c:42:
   include/linux/memblock.h:123:31: note: expected 'phys_addr_t' {aka 'long long unsigned int'} but argument is of type 'phys_addr_t *' {aka 'long long unsigned int *'}
    int memblock_free(phys_addr_t base, phys_addr_t size);
                      ~~~~~~~~~~~~^~~~

vim +/memblock_free +230 kernel/dma/swiotlb.c

   190	
   191	int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
   192	{
   193		unsigned long i, bytes;
   194	
   195		bytes = nslabs << IO_TLB_SHIFT;
   196	
   197		io_tlb_nslabs = nslabs;
   198		io_tlb_start = __pa(tlb);
   199		io_tlb_end = io_tlb_start + bytes;
   200	
   201		/*
   202		 * Allocate and initialize the free list array.  This array is used
   203		 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
   204		 * between io_tlb_start and io_tlb_end.
   205		 */
   206		io_tlb_list = memblock_alloc_nopanic(
   207					PAGE_ALIGN(io_tlb_nslabs * sizeof(int)),
   208					PAGE_SIZE);
   209		io_tlb_orig_addr = memblock_alloc_nopanic(
   210					PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)),
   211					PAGE_SIZE);
   212		if (io_tlb_list == NULL || io_tlb_orig_addr == NULL)
   213			goto out_fail;
   214	
   215		for (i = 0; i < io_tlb_nslabs; i++) {
   216			io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
   217			io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
   218		}
   219		io_tlb_index = 0;
   220	
   221		if (verbose)
   222			swiotlb_print_info();
   223	
   224		swiotlb_set_max_segment(io_tlb_nslabs << IO_TLB_SHIFT);
   225	
   226		return 0;
   227	
   228	out_fail:
   229		if (io_tlb_list)
 > 230			memblock_free(io_tlb_list,
   231				      PAGE_ALIGN(io_tlb_nslabs * sizeof(int)));
   232	
   233		if (io_tlb_orig_addr)
   234			memblock_free(io_tlb_orig_addr,
   235				      PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
   236	
   237		io_tlb_list      = NULL;
   238		io_tlb_orig_addr = NULL;
   239		io_tlb_end       = 0;
   240		io_tlb_start     = 0;
   241		io_tlb_nslabs    = 0;
   242		max_segment      = 0;
   243	
   244		return -ENOMEM;
   245	}
   246	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 4490 bytes --]