From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Rapoport Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() Date: Wed, 16 Jan 2019 17:13:49 +0200 Message-ID: <20190116151348.GD6643@rapoport-lnx> References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Message-Id:In-Reply-To:MIME-Version: References:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=V3wGTrK6/NKTZ1dyEKnvsNZL+bUE3zSvxxXajDP9AJw=; b=BHS/kVGe3vBr1T t4yKN1g2Ap5clbnDRABTid0ZKGnFlslFZ0N52CSsMg8/AaPcGWlJXTU5O1/YxmU1J1NXzDXd2bxg9 lEHVUxFgnUw6jRCZzXG1Ez6DKwCbENgicIXEIYy5g7Y4U7fWRXmFW7qMyVSbjMfTBdIWjG/Jb74qb f7DqqM2ib7w7eiCG+442zM3yEf6awiIrsGFRieRI3JsStpsIFoU8gOInt55zG0gVMFLKagK/FmJVL jxsoH1rGz16afLE2ye2ldcoIVSjtjtfb11Jg9oflhrDBw6e3eLiFmRSs9ygoBbVGzEEgAo+ZjsvBA 5Z+HpjG/XBf+fFN37iUA==; Content-Disposition: inline In-Reply-To: List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-snps-arc" Errors-To: linux-snps-arc-bounces+gla-linux-snps-arc=m.gmane.org@lists.infradead.org To: Geert Uytterhoeven Cc: Rich Felker , "linux-ia64@vger.kernel.org" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Catalin Marinas , Heiko Carstens , the arch/x86 maintainers , linux-mips@vger.kernel.org, Max Filippov , Guo Ren , sparclinux , Christoph Hellwig , linux-s390 , linux-c6x-dev@linux-c6x.org, Yoshinori Sato , Richard Weinberger , Linux-sh list , Russell King , kasan-dev@googlegroups.com, Mark Salter , Dennis Zhou , Matt Turner , arcml , On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 16 Jan 2019 17:13:49 +0200 From: Mike Rapoport Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Message-Id: <20190116151348.GD6643@rapoport-lnx> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org List-Archive: To: Geert Uytterhoeven Cc: Rich Felker , "linux-ia64@vger.kernel.org" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Catalin Marinas , Heiko Carstens , the arch/x86 maintainers , linux-mips@vger.kernel.org, Max Filippov , Guo Ren , sparclinux , Christoph Hellwig , linux-s390 , linux-c6x-dev@linux-c6x.org, Yoshinori Sato , Richard Weinberger , Linux-sh list , Russell King , kasan-dev@googlegroups.com, Mark Salter , Dennis Zhou , Matt Turner , arcml , "moderated list:H8/300 ARCHITECTURE" , Petr Mladek , linux-xtensa@linux-xtensa.org, alpha , linux-um@lists.infradead.org, linux-m68k , Rob Herring , Greentime Hu , xen-devel@lists.xenproject.org, Stafford Horne , Guan Xuetao , Linux ARM , Michal Simek , Tony Luck , Linux MM , Greg Kroah-Hartman , USB list , Linux Kernel Mailing List , Paul Burton , Vineet Gupta , Michael Ellerman , Andrew Morton , linuxppc-dev , "David S. Miller" , Openrisc List-ID: On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 From: rppt@linux.ibm.com (Mike Rapoport) Date: Wed, 16 Jan 2019 17:13:49 +0200 Subject: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() In-Reply-To: References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> List-ID: Message-ID: <20190116151348.GD6643@rapoport-lnx> To: linux-snps-arc@lists.infradead.org On Wed, Jan 16, 2019@03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019@2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >>From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 16 Jan 2019 17:13:49 +0200 From: Mike Rapoport Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Message-Id: <20190116151348.GD6643@rapoport-lnx> To: Geert Uytterhoeven Cc: Linux MM , Andrew Morton , Catalin Marinas , Christoph Hellwig , "David S. Miller" , Dennis Zhou , Greentime Hu , Greg Kroah-Hartman , Guan Xuetao , Guo Ren , Heiko Carstens , Mark Salter , Matt Turner , Max Filippov , Michael Ellerman , Michal Simek , Paul Burton , Petr Mladek , Rich Felker , Richard Weinberger , Rob Herring , Russell King , Stafford Horne , Tony Luck , Vineet Gupta , Yoshinori Sato , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , kasan-dev@googlegroups.com, alpha , Linux ARM , linux-c6x-dev@linux-c6x.org, "linux-ia64@vger.kernel.org" , Linux Kernel Mailing List , linux-m68k , linux-mips@vger.kernel.org, linux-s390 , Linux-sh list , arcml , linux-um@lists.infradead.org, USB list , linux-xtensa@linux-xtensa.org, linuxppc-dev , Openrisc , sparclinux , "moderated list:H8/300 ARCHITECTURE" , the arch/x86 maintainers , xen-devel@lists.xenproject.org List-ID: On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Rapoport Date: Wed, 16 Jan 2019 17:13:49 +0200 Subject: [OpenRISC] [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() In-Reply-To: References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> Message-ID: <20190116151348.GD6643@rapoport-lnx> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: openrisc@lists.librecores.org On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F3E8FC43387 for ; Wed, 16 Jan 2019 15:15:58 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3287B2087E for ; Wed, 16 Jan 2019 15:15:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3287B2087E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.ibm.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 43frPk73VtzDqlc for ; Thu, 17 Jan 2019 02:15:54 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=linux.ibm.com (client-ip=148.163.158.5; helo=mx0a-001b2d01.pphosted.com; envelope-from=rppt@linux.ibm.com; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 43frMm4j6rzDqdv for ; Thu, 17 Jan 2019 02:14:12 +1100 (AEDT) Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id x0GF9ECr108476 for ; Wed, 16 Jan 2019 10:14:10 -0500 Received: from e06smtp02.uk.ibm.com (e06smtp02.uk.ibm.com [195.75.94.98]) by mx0b-001b2d01.pphosted.com with ESMTP id 2q26rb13v0-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 16 Jan 2019 10:14:10 -0500 Received: from localhost by e06smtp02.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 16 Jan 2019 15:14:07 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp02.uk.ibm.com (192.168.101.132) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Wed, 16 Jan 2019 15:13:55 -0000 Received: from b06wcsmtp001.portsmouth.uk.ibm.com (b06wcsmtp001.portsmouth.uk.ibm.com [9.149.105.160]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id x0GFDsYN29294608 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 16 Jan 2019 15:13:54 GMT Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id F3306A4066; Wed, 16 Jan 2019 15:13:53 +0000 (GMT) Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C613EA405F; Wed, 16 Jan 2019 15:13:50 +0000 (GMT) Received: from rapoport-lnx (unknown [9.148.8.226]) by b06wcsmtp001.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Wed, 16 Jan 2019 15:13:50 +0000 (GMT) Date: Wed, 16 Jan 2019 17:13:49 +0200 From: Mike Rapoport To: Geert Uytterhoeven Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-TM-AS-GCONF: 00 x-cbid: 19011615-0008-0000-0000-000002B1DF75 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19011615-0009-0000-0000-0000221DF943 Message-Id: <20190116151348.GD6643@rapoport-lnx> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2019-01-16_06:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1901160125 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rich Felker , "linux-ia64@vger.kernel.org" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Catalin Marinas , Heiko Carstens , the arch/x86 maintainers , linux-mips@vger.kernel.org, Max Filippov , Guo Ren , sparclinux , Christoph Hellwig , linux-s390 , linux-c6x-dev@linux-c6x.org, Yoshinori Sato , Richard Weinberger , Linux-sh list , Russell King , kasan-dev@googlegroups.com, Mark Salter , Dennis Zhou , Matt Turner , arcml , "moderated list:H8/300 ARCHITECTURE" , Petr Mladek , linux-xtensa@linux-xtensa.org, alpha , linux-um@lists.infradead.org, linux-m68k , Rob Herring , Greentime Hu , xen-devel@lists.xenproject.org, Stafford Horne , Guan Xuetao , Linux ARM , Michal Simek , Tony Luck , Linux MM , Greg Kroah-Hartman , USB list , Linux Kernel Mailing List , Paul Burton , Vineet Gupta , Andrew Morton , linuxppc-dev , "David S. Miller" , Openrisc Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Rapoport Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() Date: Wed, 16 Jan 2019 17:13:49 +0200 Message-ID: <20190116151348.GD6643@rapoport-lnx> References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-snps-arc" Errors-To: linux-snps-arc-bounces+gla-linux-snps-arc=m.gmane.org@lists.infradead.org To: Geert Uytterhoeven Cc: Rich Felker , "linux-ia64@vger.kernel.org" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , Catalin Marinas , Heiko Carstens , the arch/x86 maintainers , linux-mips@vger.kernel.org, Max Filippov , Guo Ren , sparclinux , Christoph Hellwig , linux-s390 , linux-c6x-dev@linux-c6x.org, Yoshinori Sato , Richard Weinberger , Linux-sh list , Russell King , kasan-dev@googlegroups.com, Mark Salter , Dennis Zhou , Matt Turner , arcml List-Id: devicetree@vger.kernel.org On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >>From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-f197.google.com (mail-pl1-f197.google.com [209.85.214.197]) by kanga.kvack.org (Postfix) with ESMTP id A66BC8E0002 for ; Wed, 16 Jan 2019 10:14:12 -0500 (EST) Received: by mail-pl1-f197.google.com with SMTP id c14so3972852pls.21 for ; Wed, 16 Jan 2019 07:14:12 -0800 (PST) Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com. [148.163.156.1]) by mx.google.com with ESMTPS id o32si1715942pld.407.2019.01.16.07.14.10 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Jan 2019 07:14:11 -0800 (PST) Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id x0GF9GTD085608 for ; Wed, 16 Jan 2019 10:14:10 -0500 Received: from e06smtp02.uk.ibm.com (e06smtp02.uk.ibm.com [195.75.94.98]) by mx0a-001b2d01.pphosted.com with ESMTP id 2q26gbsx7t-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Wed, 16 Jan 2019 10:14:10 -0500 Received: from localhost by e06smtp02.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 16 Jan 2019 15:14:07 -0000 Date: Wed, 16 Jan 2019 17:13:49 +0200 From: Mike Rapoport Subject: Re: [PATCH 19/21] treewide: add checks for the return value of memblock_alloc*() References: <1547646261-32535-1-git-send-email-rppt@linux.ibm.com> <1547646261-32535-20-git-send-email-rppt@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Message-Id: <20190116151348.GD6643@rapoport-lnx> Sender: owner-linux-mm@kvack.org List-ID: To: Geert Uytterhoeven Cc: Linux MM , Andrew Morton , Catalin Marinas , Christoph Hellwig , "David S. Miller" , Dennis Zhou , Greentime Hu , Greg Kroah-Hartman , Guan Xuetao , Guo Ren , Heiko Carstens , Mark Salter , Matt Turner , Max Filippov , Michael Ellerman , Michal Simek , Paul Burton , Petr Mladek , Rich Felker , Richard Weinberger , Rob Herring , Russell King , Stafford Horne , Tony Luck , Vineet Gupta , Yoshinori Sato , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , kasan-dev@googlegroups.com, alpha , Linux ARM , linux-c6x-dev@linux-c6x.org, "linux-ia64@vger.kernel.org" , Linux Kernel Mailing List , linux-m68k , linux-mips@vger.kernel.org, linux-s390 , Linux-sh list , arcml , linux-um@lists.infradead.org, USB list , linux-xtensa@linux-xtensa.org, linuxppc-dev , Openrisc , sparclinux , "moderated list:H8/300 ARCHITECTURE" , the arch/x86 maintainers , xen-devel@lists.xenproject.org On Wed, Jan 16, 2019 at 03:27:35PM +0100, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Jan 16, 2019 at 2:46 PM Mike Rapoport wrote: > > Add check for the return value of memblock_alloc*() functions and call > > panic() in case of error. > > The panic message repeats the one used by panicing memblock allocators with > > adjustment of parameters to include only relevant ones. > > > > The replacement was mostly automated with semantic patches like the one > > below with manual massaging of format strings. > > > > @@ > > expression ptr, size, align; > > @@ > > ptr = memblock_alloc(size, align); > > + if (!ptr) > > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n", __func__, > > In general, you want to use %zu for size_t > > > size, align); > > > > Signed-off-by: Mike Rapoport > > Thanks for your patch! > > > 74 files changed, 415 insertions(+), 29 deletions(-) > > I'm wondering if this is really an improvement? >>From memblock perspective it's definitely an improvement :) git diff --stat mmotm/master include/linux/memblock.h mm/memblock.c include/linux/memblock.h | 59 ++--------- mm/memblock.c | 249 ++++++++++++++++------------------------------- 2 files changed, 90 insertions(+), 218 deletions(-) > For the normal memory allocator, the trend is to remove printing of errors > from all callers, as the core takes care of that. It's more about allocation errors handling than printing of the errors. Indeed, there is not much that can be done if an early allocation fails, but I believe having an explicit pattern ptr = alloc(); if (!ptr) do_something_about_it(); is clearer than relying on the allocator to panic(). Besides, the diversity of panic and nopanic variants creates a confusion and I've caught several places that call nopanic variant and do not check its return value. > > --- a/arch/alpha/kernel/core_marvel.c > > +++ b/arch/alpha/kernel/core_marvel.c > > @@ -83,6 +83,9 @@ mk_resource_name(int pe, int port, char *str) > > > > sprintf(tmp, "PCI %s PE %d PORT %d", str, pe, port); > > name = memblock_alloc(strlen(tmp) + 1, SMP_CACHE_BYTES); > > + if (!name) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as strlen() returns size_t. Thanks for spotting it, will fix. > > + strlen(tmp) + 1); > > strcpy(name, tmp); > > > > return name; > > @@ -118,6 +121,9 @@ alloc_io7(unsigned int pe) > > } > > > > io7 = memblock_alloc(sizeof(*io7), SMP_CACHE_BYTES); > > + if (!io7) > > + panic("%s: Failed to allocate %lu bytes\n", __func__, > > %zu, as sizeof() returns size_t. > Probably there are more. Yes, it's hard to get them right in all callers. Yeah :) > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds > -- Sincerely yours, Mike.