gigaset: freeing an active object

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.)

On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote:
> 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;
>  }

This solution assumes that the struct platform_device is moved out of
the struct ser_cardstate, doesn't it? In other words, this is something
to do on top of my (draft) patch. Otherwise we'd still be freeing memory
managed through reference counting.

Thanks,


Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 06.12.2015 um 21:12 schrieb Paul Bolle:
> On zo, 2015-12-06 at 16:29 +0100, Tilman Schmidt wrote:
>> 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;
>>  }
> 
> This solution assumes that the struct platform_device is moved out of
> the struct ser_cardstate, doesn't it? In other words, this is something
> to do on top of my (draft) patch.

No, that wasn't my intention. I thought of that solution as an
alternative, not an increment to your patch.

> Otherwise we'd still be freeing memory
> managed through reference counting.

Now I#m confused. I thought by following Peter's suggestion to put the
kfree() in the release method we'd avoid just that.

Regards,
Tilman

[Re-added mailinglist that got dropped somehow.]

On ma, 2015-12-07 at 10:27 +0100, Tilman Schmidt wrote:
> Am 06.12.2015 um 21:12 schrieb Paul Bolle:
> > This solution assumes that the struct platform_device is moved out
> > of
> > the struct ser_cardstate, doesn't it? In other words, this is
> > something
> > to do on top of my (draft) patch.
> 
> No, that wasn't my intention. I thought of that solution as an
> alternative, not an increment to your patch.
> 
> > Otherwise we'd still be freeing memory
> > managed through reference counting.
> 
> Now I#m confused. I thought by following Peter's suggestion to put the
> kfree() in the release method we'd avoid just that.

(Your patch compiles just fine.)

Apparently it does, because I can't trigger the WARNING we're discussing
here with your patch applied. I'll have to dive into this stuff again,
because apparently my mental model of what's going on is incomplete at
best.

In the mean time you might want to turn your patch into something that
can actually be applied (with or without my Sign-off or Ack; I don't
care how it finds its way into the tree). Please add add
    Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")

(Perhaps with a comment that v2.6.32 needs a trivial context change; I'm
not sure how that needs to be communicated.)

But I'm fine with cobbling together a commit explanation myself if
you're too busy right now.

Thanks,


Paul Bolle
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 07.12.2015 um 13:25 schrieb Paul Bolle:

>>> Otherwise we'd still be freeing memory
>>> managed through reference counting.
>>
>> Now I#m confused. I thought by following Peter's suggestion to put the
>> kfree() in the release method we'd avoid just that.
> 
> Apparently it does, because I can't trigger the WARNING we're discussing
> here with your patch applied.

Nice.

> I'll have to dive into this stuff again,
> because apparently my mental model of what's going on is incomplete at
> best.

I won't claim anything like completeness for mine.

> In the mean time you might want to turn your patch into something that
> can actually be applied (with or without my Sign-off or Ack; I don't
> care how it finds its way into the tree). Please add add
>     Fixes: 2869b23e4b95 ("[PATCH] drivers/isdn/gigaset: new M101 driver (v2)")

Will do. (Not today, though.)

Regards,
Tilman