Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755543AbZFHT6h (ORCPT ); Mon, 8 Jun 2009 15:58:37 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751969AbZFHT62 (ORCPT ); Mon, 8 Jun 2009 15:58:28 -0400 Received: from mail-ew0-f210.google.com ([209.85.219.210]:56994 "EHLO mail-ew0-f210.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751638AbZFHT61 (ORCPT ); Mon, 8 Jun 2009 15:58:27 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:to:subject:date:user-agent:cc:references:in-reply-to :mime-version:content-disposition:message-id:content-type :content-transfer-encoding; b=PyydGplbkiFV8PDlzAKC/92ihac6niIJlb5J4idpbcbgr0iZV8cmWIzLeYVuUkPjbo Nyq1ST3gZhP9nklLQeoZY8RLNLpd1RrRgnJtwktU5u3qauueP+6IxZ4u9OIkIjhhZxI7 +z2u0efSUQzUhukaHKwSa/oOEWLsrKUsVENsE= From: Bartlomiej Zolnierkiewicz To: Borislav Petkov Subject: Re: [PATCH] ide-tape: fix proc warning Date: Mon, 8 Jun 2009 21:56:50 +0200 User-Agent: KMail/1.11.3 (Linux/2.6.30-rc8-next-20090605-07377-g5acd34e; KDE/4.2.3; i686; ; ) Cc: linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org References: <20090607092054.GA26408@liondog.tnic> <200906071528.05185.bzolnier@gmail.com> <20090607174533.GA15551@liondog.tnic> In-Reply-To: <20090607174533.GA15551@liondog.tnic> MIME-Version: 1.0 Content-Disposition: inline Message-Id: <200906082156.50923.bzolnier@gmail.com> Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4628 Lines: 100 On Sunday 07 June 2009 19:45:33 Borislav Petkov wrote: > Hi, > > On Sun, Jun 07, 2009 at 03:28:04PM +0200, Bartlomiej Zolnierkiewicz wrote: > > > because it tries to free its proc entry in drive_release_dev() > > > in the call chain resulting from ide_tape_put() but it chokes on > > > /proc/ide/ide[01]/hd?/name which is added during driver registration and > > > should go only at driver removal time. > > > > > > Add/remove those proc entries everytime the device is accessed. > > > > This looks more like broken ide-tape refcounting for chrdev interface > > than mismatch of /proc entries registration time (especially since other > > device drivers are not having the above problem).. > > This sounds more like it, I was wondering why that > /proc/ide/ide[01]/hd?/name attribute was disappearing. > > > Please see ide_tape_chrdev_get() -- it misses ide_device_get() call > > [present in ide_tape_get()] which can result in premature release of > > IDE device during ide_tape_put() call. > > By the way, why are we using two devs for the reference counting: > drive->gendev over ide_device_get() and fall back to {tape,cd,idkp}->dev > in get_device(). Isn't one enough? ->gendev and ->dev have different lifetime rules. > Anyways, here's the fixed version, tested with my Seagate STT8000A, > works fine. > > -- > From: Borislav Petkov > Date: Sun, 7 Jun 2009 19:35:56 +0200 > Subject: [PATCH] ide-tape: fix proc warning > > ide_tape_chrdev_get() was missing an ide_device_get() refcount increment > which lead to the following warning: > > [ 278.147906] ------------[ cut here ]------------ > [ 278.152685] WARNING: at fs/proc/generic.c:847 remove_proc_entry+0x199/0x1b8() > [ 278.160070] Hardware name: P4I45PE 1.00 > [ 278.160076] remove_proc_entry: removing non-empty directory 'ide0/hdb', leaking at least 'name' > [ 278.160080] Modules linked in: rtc intel_agp pcspkr thermal processor thermal_sys parport_pc parport agpgart button > [ 278.160100] Pid: 2312, comm: mt Not tainted 2.6.30-rc2 #3 > [ 278.160105] Call Trace: > [ 278.160117] [] warn_slowpath+0x71/0xa0 > [ 278.160126] [] ? _spin_unlock_irqrestore+0x29/0x2c > [ 278.160132] [] ? try_to_wake_up+0x1b6/0x1c0 > [ 278.160141] [] ? default_wake_function+0xb/0xd > [ 278.160149] [] ? pollwake+0x4a/0x55 > [ 278.160156] [] ? _spin_unlock+0x24/0x26 > [ 278.160163] [] ? add_partial+0x44/0x49 > [ 278.160169] [] ? __slab_free+0xba/0x29c > [ 278.160177] [] ? sysfs_delete_inode+0x0/0x3c > [ 278.160184] [] remove_proc_entry+0x199/0x1b8 > [ 278.160191] [] ? remove_dir+0x27/0x2e > [ 278.160199] [] ide_proc_unregister_device+0x40/0x4c > [ 278.160207] [] drive_release_dev+0x14/0x47 > [ 278.160214] [] device_release+0x35/0x5a > [ 278.160221] [] kobject_release+0x40/0x50 > [ 278.160226] [] ? kobject_release+0x0/0x50 > [ 278.160232] [] kref_put+0x3c/0x4a > [ 278.160238] [] kobject_put+0x37/0x3c > [ 278.160243] [] put_device+0xf/0x11 > [ 278.160249] [] ide_device_put+0x2d/0x30 > [ 278.160255] [] ide_tape_put+0x24/0x32 > [ 278.160261] [] idetape_chrdev_release+0x17f/0x18e > [ 278.160269] [] __fput+0xca/0x175 > [ 278.160275] [] fput+0x19/0x1b > [ 278.160280] [] filp_close+0x51/0x5b > [ 278.160286] [] sys_close+0x73/0xad > [ 278.160293] [] syscall_call+0x7/0xb > [ 278.160298] ---[ end trace f16d907ea1f89336 ]--- > > Instead of trivially fixing it by adding the missing call, > ide_tape_chrdev_get() and ide_tape_get() were merged into one function > since both were almost identical. The only difference was that > ide_tape_chrdev_get() was accessing the ide-tape reference through the > idetape_devs[] array of minors instead of through the gendisk. > > Accomodate that by adding two additional parameters to ide_tape_get() to > annotate the call site and invoke the proper behavior. > > As a result, remove ide_tape_chrdev_get(). > > There should be no functional change resulting from this patch. Doesn't it fix the warning? :) [ I removed this line while merging the patch. ] > Signed-off-by: Borislav Petkov applied -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/