From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932610AbaE2GlJ (ORCPT ); Thu, 29 May 2014 02:41:09 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:42276 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933012AbaE2GlD (ORCPT ); Thu, 29 May 2014 02:41:03 -0400 Date: Thu, 29 May 2014 09:40:39 +0300 From: Dan Carpenter To: DaeSeok Youn Cc: devel , Lidza Louina , driverdev-devel@linuxdriverproject.org, linux-kernel , Greg KH , Mark Hounschell Subject: Re: [PATCH V2] staging: dgap: implement proper error handling in dgap_firmware_load() Message-ID: <20140529064039.GW17724@mwanda> References: <20140527070937.GA21621@devel.8.8.4.4> <20140527102002.GI17724@mwanda> <20140528070233.GP17724@mwanda> <20140528101153.GU15585@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: acsinet22.oracle.com [141.146.126.238] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 29, 2014 at 09:17:09AM +0900, DaeSeok Youn wrote: > Hi, Dan. > > 2014-05-28 19:11 GMT+09:00 Dan Carpenter : > > On Wed, May 28, 2014 at 06:29:38PM +0900, DaeSeok Youn wrote: > >> > In your patch it has: > >> > + dgap_tty_uninit(brd, false); > >> > > >> > But it should only be "false" if dgap_tty_init() failed. If > >> > dgap_tty_register_ports() fails then it should be "true". Another > >> Yes, you're right. There were no error handle for tty_port_register_device() and > >> dgap_create_tty_sysfs() in dgap_tty_register_ports(). I didn't catch it. :-( > >> It need to add error handlers for them, right? > > > > Eventually, yes. But I don't see a simple way to fix > > dgap_firmware_load() until after the code is cleaned up. > > > >> > >> > problem is that as you say, the earlier function are allocating > >> > resources like dgap_tty_register() but only the last two function calls > >> > have a "goto err_cleanup;" so the error handling is incomplete. > >> So remove "goto" in dgap_firmware_load() and add error handler in > >> dgap_tty_init() > > > > In the current code there isn't a goto in dgap_firmware_load(). Remove > > the call to dgap_tty_uninit() and add error handling in dgap_tty_init(). > Yes. I will try to fix it. > > > > That will clean up the code, and fix some NULL dereference bugs inside > > dgap_tty_uninit(). > > > >> and dgap_tty_register_ports(), right? > > > > Inside dgap_tty_register_ports(), then we should add a > > kfree(brd->serial_ports) if the "brd->printer_ports" allocation fails. > > That is not a complete fix, but it is a part fix and it is clean. > Actually, I sent a patch which is removing "kfree(brd->serial_ports)" and pushed > into staging-next branch. > see the 0ade4a34fd43 staging: dgap: remove unneeded kfree() in > dgap_tty_register_ports() > Because I think dgap_tty_uninit() will free "brd->serial_ports" with this patch. > > Can I send a patch after revert "0ade4a34fd43" commit? Oh, crud. I missed that. Yeah. Let's revert it. regards, dan carpenter