From patchwork Sun Dec 6 15:29:32 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tilman Schmidt X-Patchwork-Id: 553139 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 5D1951402A0 for ; Mon, 7 Dec 2015 02:30:49 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=imap.cc header.i=@imap.cc header.b=ysldH1bB; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=messagingengine.com header.i=@messagingengine.com header.b=I2xH5gp8; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753043AbbLFP3s (ORCPT ); Sun, 6 Dec 2015 10:29:48 -0500 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:45233 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753117AbbLFP3q (ORCPT ); Sun, 6 Dec 2015 10:29:46 -0500 Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id 7CBAB20796 for ; Sun, 6 Dec 2015 10:29:45 -0500 (EST) Received: from frontend1 ([10.202.2.160]) by compute2.internal (MEProxy); Sun, 06 Dec 2015 10:29:45 -0500 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=imap.cc; h=cc :content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=/5lZO Cczb5W2v3nlnHa0quwzF8Q=; b=ysldH1bBTqRy8aSVce7r0RJ/RSkbHkzOiif2J 9bBRhg/deBiL8QuGEGn7WOUm9ZHH5HXH7DHG8N2YAMl8kynFw88jtO386IKrW+fq IxY0rl1G7OXa6q5YqRiOdcDp26tBOOQc9MUim2RyiJgKZPtBrT2MdvOuJhc2XyFU +jEZA8= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-sasl-enc :x-sasl-enc; s=smtpout; bh=/5lZOCczb5W2v3nlnHa0quwzF8Q=; b=I2xH5 gp8Os3qs1Fdrt0xwxK65CyLVcSHrEN7uGhxUmUxOU9qW5kAV/0tQ1AkLSBTuM7j7 OimGKQVU4ber3fXo2/ToAb9EkTGJ+5TDqIVkT54PpoqzOOtRnCRKbDWEk6LXBqbT qF7CQ7MGAHmpFm6OQ0VKNwjo5EHFqYSNLdbD1k= X-Sasl-enc: sd5N9BlA1t93Cc6wKPCdpWo8Rgvw6+/PqLzJcrmLZrb+ 1449415784 Received: from [192.168.59.117] (p5de8ec74.dip0.t-ipconnect.de [93.232.236.116]) by mail.messagingengine.com (Postfix) with ESMTPA id EBC28C0001C; Sun, 6 Dec 2015 10:29:43 -0500 (EST) Subject: Re: gigaset: freeing an active object To: Paul Bolle , Peter Hurley , Sasha Levin References: <56587467.8050102@oracle.com> <565B1A1B.8020503@imap.cc> <565B4256.6080101@hurleysoftware.com> <565B4844.9020600@imap.cc> <1448828800.2603.17.camel@tiscali.nl> <1448839396.2891.14.camel@tiscali.nl> <1448906497.3546.16.camel@tiscali.nl> <565F8341.7010704@hurleysoftware.com> <1449408690.2515.15.camel@tiscali.nl> Cc: isdn@linux-pingi.de, davem@davemloft.net, gigaset307x-common@lists.sourceforge.net, LKML , "netdev@vger.kernel.org" , syzkaller From: Tilman Schmidt X-Enigmail-Draft-Status: N1110 Message-ID: <5664545C.90607@imap.cc> Date: Sun, 6 Dec 2015 16:29:32 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1449408690.2515.15.camel@tiscali.nl> Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Am 06.12.2015 um 14:31 schrieb Paul Bolle: > On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote: >> On 11/30/2015 01:01 PM, Paul Bolle wrote: >>> --- a/drivers/isdn/gigaset/ser-gigaset.c >>> +++ b/drivers/isdn/gigaset/ser-gigaset.c >>> @@ -42,8 +42,9 @@ MODULE_PARM_DESC(cidmode, "stay in CID mode when >>> idle"); >>> >>> static struct gigaset_driver *driver; >>> >>> +static struct platform_device pdev; >>> + >>> struct ser_cardstate { >>> - struct platform_device dev; >>> struct tty_struct *tty; >>> atomic_t refcnt; >>> struct completion dead_cmp; >>> @@ -370,8 +371,8 @@ static void gigaset_freecshw(struct cardstate >>> *cs) >>> tasklet_kill(&cs->write_tasklet); >>> if (!cs->hw.ser) >>> return; >>> - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); >>> - platform_device_unregister(&cs->hw.ser->dev); >>> + dev_set_drvdata(&pdev.dev, NULL); >>> + platform_device_unregister(&pdev); >>> kfree(cs->hw.ser); >> >> Tilman, >> >> Is there a 1:1 correspondence and lifetime for the embedded platform >> device and it's containing memory? > > (Haven't heard from Tilman, so I'll give this a try.) Sorry for that. Been busy. > That containing memory is a struct ser_cardstate. And currently > instances of struct _ser_cardstate are malloced and freed in routines > that also call platform_device_register() and > platform_device_unregister(). So yes, I think there's a 1:1 > correspondence. Correct. >> I ask because the typical approach for device teardown is to put the >> kfree() in the release method; > > (Side note: the (struct device) release method of this driver > -gigaset_device_release() - is actually a nop. It only frees device > ->platform_data and platform_device->resource, but neither are actually > used: they remain NULL through their entire life.) Yeah, that was just copied unthinkingly from driver/base/platform.c. So the solution might be as simple as moving the kfree() call from gigaset_freecshw() to gigaset_device_release(). Something like this: /* (Off the top of my hat, completely untested, don't even know if that will compile.) --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -370,19 +370,18 @@ static void gigaset_freecshw(struct cardstate *cs) tasklet_kill(&cs->write_tasklet); if (!cs->hw.ser) return; - dev_set_drvdata(&cs->hw.ser->dev.dev, NULL); platform_device_unregister(&cs->hw.ser->dev); - kfree(cs->hw.ser); - cs->hw.ser = NULL; } static void gigaset_device_release(struct device *dev) { - struct platform_device *pdev = to_platform_device(dev); + struct cardstate *cs = dev_get_drvdata(dev); - /* adapted from platform_device_release() in drivers/base/platform.c */ - kfree(dev->platform_data); - kfree(pdev->resource); + if (!cs) + return; + dev_set_drvdata(dev, NULL); + kfree(cs->hw.ser); + cs->hw.ser = NULL; }