Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 15 Apr 2019 01:24:43 +0200
From:   Nicholas Mc Guire <der.herr@hofr.at>
To:     Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc:     Nicholas Mc Guire <hofrat@osadl.org>,
        Jason Cooper <jason@lakedaemon.net>,
        Andrew Lunn <andrew@lunn.ch>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3 RFC] ARM: mvebu: at least warn on kzalloc failure
Message-ID: <20190414232443.GB1215@osadl.at>
References: <1555217391-3552-1-git-send-email-hofrat@osadl.org>
 <20190414172602.z6d4swzk6bjjumls@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190414172602.z6d4swzk6bjjumls@shell.armlinux.org.uk>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Apr 14, 2019 at 06:26:02PM +0100, Russell King - ARM Linux admin wrote:
> On Sun, Apr 14, 2019 at 06:49:49AM +0200, Nicholas Mc Guire wrote:
> > Although it is very unlikely that the allocation during init would
> > fail any such failure should point to the original cause rather
> > than waiting for a null-pointer dereference to splat.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > ---
> > 
> > Problem located with experimental coccinelle script
> > 
> > While this will not really help much - but kzalloc failures should not
> > go unhandled. 
> 
> Sorry, no, not like this.
>

ok - well I wsa not sure about it either - it just seems wrong
to leave a possible allocation failure without any response.

The issue of generating excessive outout make sense - so will fix
it up to a pr_err() and resend.

thx!
hofrat
 
> With this patch, rather than getting an oops and a stacktrace which
> people can capture and email, we instead end up getting a warning
> line, a stack trace, followed by an oops containing another stack
> trace.
> 
> We _already_ have problems getting people to send us kernel message
> debug information without editing out what they deem to be "unnecessary
> verbage", like all those numbers and function names that comprise a
> stack trace.  We don't need yet more of that stuff, especially when it
> is redundant.
> 
> So, I think throwing WARN_ON() at this case is way too excessive, and
> will only have a detrimental effect on the reports we receive - and
> that is extremely important.
> 
> IMHO, A better solution would be to just print a warning, rather than
> causing the kernel to print several kB of needless messages.
> 
> 	if (!new_compat)
> 		pr_err("new_compat allocation failure in %s()\n",
> 		       __func__);
> 
> > 
> > Patch was compile-tested: mvebu_v7_defconfig (implies MACH_MVEBU_ANY=y)
> > (with some unrelated sparse warnings about missing syscalls)
> > 
> > Patch is against 5.1-rc4 (localversion-next is 20190412)
> > 
> >  arch/arm/mach-mvebu/board-v7.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> > index 0b10acd..37f8cb6 100644
> > --- a/arch/arm/mach-mvebu/board-v7.c
> > +++ b/arch/arm/mach-mvebu/board-v7.c
> > @@ -128,6 +128,7 @@ static void __init i2c_quirk(void)
> >  		struct property *new_compat;
> >  
> >  		new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
> > +		WARN_ON(!new_compat);
> >  
> >  		new_compat->name = kstrdup("compatible", GFP_KERNEL);
> >  		new_compat->length = sizeof("marvell,mv78230-a0-i2c");
> > -- 
> > 2.1.4
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up