Re: gigaset: freeing an active object

[Tilman Schmidt <tilman@imap.cc>]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

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:

--- 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;
 }

 /*

(Off the top of my hat, completely untested, don't even know if that
will compile.)

-- 
Tilman Schmidt                              E-Mail: tilman@imap.cc
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]