Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280148 > unrolled thread

Re: gigaset: freeing an active object

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: gigaset: freeing an active object Paul Bolle <pebolle@tiscali.nl> - 2015-11-30 19:10 +0100
    Re: gigaset: freeing an active object Tilman Schmidt <tilman@imap.cc> - 2015-11-30 19:40 +0100
      Re: gigaset: freeing an active object Paul Bolle <pebolle@tiscali.nl> - 2015-11-30 22:10 +0100
        Re: gigaset: freeing an active object Tilman Schmidt <tilman@imap.cc> - 2015-12-01 10:40 +0100
          Re: gigaset: freeing an active object Paul Bolle <pebolle@tiscali.nl> - 2015-12-01 11:10 +0100
    Re: gigaset: freeing an active object Peter Hurley <peter@hurleysoftware.com> - 2015-12-03 00:50 +0100
      Re: gigaset: freeing an active object Paul Bolle <pebolle@tiscali.nl> - 2015-12-06 14:40 +0100
        Re: gigaset: freeing an active object Tilman Schmidt <tilman@imap.cc> - 2015-12-06 16:40 +0100
          Re: gigaset: freeing an active object Paul Bolle <pebolle@tiscali.nl> - 2015-12-06 21:20 +0100
            Re: gigaset: freeing an active object Tilman Schmidt <tilman@imap.cc> - 2015-12-07 10:30 +0100
              Re: gigaset: freeing an active object Paul Bolle <pebolle@tiscali.nl> - 2015-12-07 13:30 +0100
                Re: gigaset: freeing an active object Tilman Schmidt <tilman@imap.cc> - 2015-12-07 19:50 +0100

#1280148 — Re: gigaset: freeing an active object

On ma, 2015-11-30 at 00:23 +0100, Paul Bolle wrote:
> Relevant part of dmesg attached at the end of this message. This
> should give me (and Tilman too?) an entry to get to bottom of this. 
> Since this is relevant for anyone with just the ser-gigaset module 
> installed, I hope to do that soon.

I'm planning to send something similar to the attached draft to netdev
in a few days. It fixes the issue on my machine. Sascha, does it fix
this issue for syzkaller too? 

Should (something like) this go into stable too?

Any further comments on that draft are appreciated too, of course.


Paul Bolle
------
[DRAFT] gigaset: don't free() a struct platform_device

One is not supposed to free() a struct platform_device. Instead one
should, in the common case, only call platform_device_unregister(). That
will drop the platform device's reference count. (Actually it's the
reference count of the embedded kobject that is important here. But for
users of platform devices that's basically irrelevant.)

So move struct platform_device dev out of struct ser_cardstate, because
ser_cardstate is (malloc'ed and) free'd.

Reported-by: Sasha Levin <sasha.levin@oracle.com>
Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 375be509e95f..f8ffa253496e 100644
--- 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);
 	cs->hw.ser = NULL;
 }
@@ -401,17 +402,17 @@ static int gigaset_initcshw(struct cardstate *cs)
 	}
 	cs->hw.ser = scs;
 
-	cs->hw.ser->dev.name = GIGASET_MODULENAME;
-	cs->hw.ser->dev.id = cs->minor_index;
-	cs->hw.ser->dev.dev.release = gigaset_device_release;
-	rc = platform_device_register(&cs->hw.ser->dev);
+	pdev.name = GIGASET_MODULENAME;
+	pdev.id = cs->minor_index;
+	pdev.dev.release = gigaset_device_release;
+	rc = platform_device_register(&pdev);
 	if (rc != 0) {
 		pr_err("error %d registering platform device\n", rc);
 		kfree(cs->hw.ser);
 		cs->hw.ser = NULL;
 		return rc;
 	}
-	dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
+	dev_set_drvdata(&pdev.dev, cs);
 
 	tasklet_init(&cs->write_tasklet,
 		     gigaset_modem_fill, (unsigned long) cs);
@@ -520,7 +521,7 @@ gigaset_tty_open(struct tty_struct *tty)
 		goto error;
 	}
 
-	cs->dev = &cs->hw.ser->dev.dev;
+	cs->dev = &pdev.dev;
 	cs->hw.ser->tty = tty;
 	atomic_set(&cs->hw.ser->refcnt, 1);
 	init_completion(&cs->hw.ser->dead_cmp);
-- 
2.4.3
--
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/

[toc] | [next] | [standalone]

#1280189

[Multipart message — attachments visible in raw view] — view raw

Am 30.11.2015 um 19:01 schrieb Paul Bolle:
> [DRAFT] gigaset: don't free() a struct platform_device
> 
> One is not supposed to free() a struct platform_device. Instead one
> should, in the common case, only call platform_device_unregister(). That
> will drop the platform device's reference count. (Actually it's the
> reference count of the embedded kobject that is important here. But for
> users of platform devices that's basically irrelevant.)
> 
> So move struct platform_device dev out of struct ser_cardstate, because
> ser_cardstate is (malloc'ed and) free'd.

I wonder how that will behave if someone attaches two of the devices to
different serial ports. Not likely, but not forbidden either.

Regards,
Tilman

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

[toc] | [prev] | [next] | [standalone]

#1280286

On ma, 2015-11-30 at 19:30 +0100, Tilman Schmidt wrote:
> I wonder how that will behave if someone attaches two of the devices to
> different serial ports. Not likely, but not forbidden either.

I see.

Perhaps I should respin and a use a pointer to a struct platform_device
in struct ser_cardstate, use the two step approach of
platform_device_alloc() and friends, etc. Only slightly more
complicated.

How would attaching two devices work with GIGASET_MINORS hardcoded to 1?
Because I haven't yet stumbled on the mechanism with which ttyGS1 (and
up) would then be created.

(I do have a second M105's in a box somewhere, so I could check myself
what happens when a second USB device is added, for what that's worth.)

Thanks,


Paul Bolle
--
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/

[toc] | [prev] | [next] | [standalone]

#1280696

[Multipart message — attachments visible in raw view] — view raw

Am 30.11.2015 um 22:07 schrieb Paul Bolle:
> How would attaching two devices work with GIGASET_MINORS hardcoded to 1?

Ah, it wouldn't. You'd have to recompile with a bigger GIGASET_MINORS.

(I wonder if that would actually work. Somehow I still think of
GIGASET_MINORS as a configurable value, but it has never been anything
but 1 in the entire in-tree history of the driver.)

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

[toc] | [prev] | [next] | [standalone]

#1280718

On di, 2015-12-01 at 10:30 +0100, Tilman Schmidt wrote:
> Am 30.11.2015 um 22:07 schrieb Paul Bolle:
> > How would attaching two devices work with GIGASET_MINORS hardcoded
> > to 1?
> 
> Ah, it wouldn't. You'd have to recompile with a bigger GIGASET_MINORS.
> 
> (I wonder if that would actually work. Somehow I still think of
> GIGASET_MINORS as a configurable value, but it has never been anything
> but 1 in the entire in-tree history of the driver.)

That ends a prolonged session of head-scratching. I'll update the commit
explanation to reflect this conversation.

(Perhaps it's reasonable to wish to use two identical gigaset adapters
simultaneously. But given that the gigaset drivers have never supported
it, that hardly matters. So I might remove the proto-support for
multiple devices one day. That's far from urgent.)

Thanks for sticking around!


Paul Bolle
--
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/

[toc] | [prev] | [next] | [standalone]

#1282603

On 11/30/2015 01:01 PM, Paul Bolle wrote:
> On ma, 2015-11-30 at 00:23 +0100, Paul Bolle wrote:
>> Relevant part of dmesg attached at the end of this message. This
>> should give me (and Tilman too?) an entry to get to bottom of this. 
>> Since this is relevant for anyone with just the ser-gigaset module 
>> installed, I hope to do that soon.
> 
> I'm planning to send something similar to the attached draft to netdev
> in a few days. It fixes the issue on my machine. Sascha, does it fix
> this issue for syzkaller too? 
> 
> Should (something like) this go into stable too?

Definitely for stable since it has a userspace triggerable component.

> Any further comments on that draft are appreciated too, of course.
> 
> 
> Paul Bolle
> ------
> [DRAFT] gigaset: don't free() a struct platform_device
> 
> One is not supposed to free() a struct platform_device. Instead one
> should, in the common case, only call platform_device_unregister(). That
> will drop the platform device's reference count. (Actually it's the
> reference count of the embedded kobject that is important here. But for
> users of platform devices that's basically irrelevant.)
> 
> So move struct platform_device dev out of struct ser_cardstate, because
> ser_cardstate is (malloc'ed and) free'd.
> 
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Not-yet-signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
>  drivers/isdn/gigaset/ser-gigaset.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
> index 375be509e95f..f8ffa253496e 100644
> --- 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?

I ask because the typical approach for device teardown is to put the
kfree() in the release method; naturally, that won't work if there
is some other lifetime issue.

Regards,
Peter Hurley


>  	cs->hw.ser = NULL;
>  }
> @@ -401,17 +402,17 @@ static int gigaset_initcshw(struct cardstate *cs)
>  	}
>  	cs->hw.ser = scs;
>  
> -	cs->hw.ser->dev.name = GIGASET_MODULENAME;
> -	cs->hw.ser->dev.id = cs->minor_index;
> -	cs->hw.ser->dev.dev.release = gigaset_device_release;
> -	rc = platform_device_register(&cs->hw.ser->dev);
> +	pdev.name = GIGASET_MODULENAME;
> +	pdev.id = cs->minor_index;
> +	pdev.dev.release = gigaset_device_release;
> +	rc = platform_device_register(&pdev);
>  	if (rc != 0) {
>  		pr_err("error %d registering platform device\n", rc);
>  		kfree(cs->hw.ser);
>  		cs->hw.ser = NULL;
>  		return rc;
>  	}
> -	dev_set_drvdata(&cs->hw.ser->dev.dev, cs);
> +	dev_set_drvdata(&pdev.dev, cs);
>  
>  	tasklet_init(&cs->write_tasklet,
>  		     gigaset_modem_fill, (unsigned long) cs);
> @@ -520,7 +521,7 @@ gigaset_tty_open(struct tty_struct *tty)
>  		goto error;
>  	}
>  
> -	cs->dev = &cs->hw.ser->dev.dev;
> +	cs->dev = &pdev.dev;
>  	cs->hw.ser->tty = tty;
>  	atomic_set(&cs->hw.ser->refcnt, 1);
>  	init_completion(&cs->hw.ser->dead_cmp);
> 

--
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/

[toc] | [prev] | [next] | [standalone]

#1284868

On wo, 2015-12-02 at 18:48 -0500, Peter Hurley wrote:
> On 11/30/2015 01:01 PM, Paul Bolle wrote:
> > Should (something like) this go into stable too?
> 
> Definitely for stable since it has a userspace triggerable component.

Thanks, will do.

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

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.

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

>  naturally, that won't work if there
> is some other lifetime issue.

I'm not sure I follow what you mean here. Could you point me at a driver
that uses that approach, so that I can have a look at it?

Thanks,


Paul Bolle
--
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/

[toc] | [prev] | [next] | [standalone]

#1284875

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1284908

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 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/

[toc] | [prev] | [next] | [standalone]

#1285121

[Multipart message — attachments visible in raw view] — view raw

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

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

[toc] | [prev] | [next] | [standalone]

#1285265

[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 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/

[toc] | [prev] | [next] | [standalone]

#1285855

[Multipart message — attachments visible in raw view] — view raw

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web