[PATCH 0/3] ser_gigaset fixes

[PATCH 0/3] ser_gigaset fixes

[Tilman Schmidt]

Hi Paul,

this series is the result of our discussion on the "freeing an
active object" bug. I split my proposed patch into two patches
for the separate topics of moving the ser_cardstate kfree() and
dropping the useless kfree()s, and also included an unrelated
patch (1/3) that had fallen through the cracks in my last series.

Patch 2/3 should go into stable releases all the way back to 2.6.32.
It applies cleanly to release 3.*/4.* with at most offset 1.
For release 2.6.32 there is a trivial merge conflict with a removed
comment line.

Thanks,
Tilman

Tilman Schmidt (3):
  ser_gigaset: fix up NULL checks
  ser_gigaset: fix deallocation of platform device structure
  ser_gigaset: remove unnecessary kfree() calls from release method

 drivers/isdn/gigaset/ser-gigaset.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
1.9.2.459.g68773ac

[PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method

[Tilman Schmidt]

device->platform_data and platform_device->resource are never used
and remain NULL through their entire life. Drops the kfree() calls
for them from the device release method.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
Reported-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/isdn/gigaset/ser-gigaset.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 2693cb2..b7e0329 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -375,13 +375,8 @@ static void gigaset_freecshw(struct cardstate *cs)
 
 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);
-- 
1.9.2.459.g68773ac

Re: [PATCH 3/3] ser_gigaset: remove unnecessary kfree() calls from release method

[Paul Bolle]

Hi Tilman,

On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> device->platform_data and platform_device->resource are never used
> and remain NULL through their entire life. Drops the kfree() calls
> for them from the device release method.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Reported-by: Paul Bolle <pebolle@tiscali.nl>

s/Reported-by/Acked-by/

(Having both lines would be overdoing things.)

Thanks,


Paul Bolle

[PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Tilman Schmidt]

When shutting down the device, the struct ser_cardstate must not be
kfree()d immediately after the call to platform_device_unregister()
since the embedded struct platform_device is still in use.
Move the kfree() call to the release method instead.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by: Sasha Levin <sasha.levin@oracle.com>
---
 drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index d8771b5..2693cb2 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -370,19 +370,23 @@ 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;
 }
 
 /*
-- 
1.9.2.459.g68773ac

Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Paul Bolle]

Hi Tilman,

On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> When shutting down the device, the struct ser_cardstate must not be
> kfree()d immediately after the call to platform_device_unregister()
> since the embedded struct platform_device is still in use.
> Move the kfree() call to the release method instead.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Fixes: 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> ---
>  drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> b/drivers/isdn/gigaset/ser-gigaset.c
> index d8771b5..2693cb2 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -370,19 +370,23 @@ 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);

dev equals cs->hw.ser->dev.dev, doesn't it? So what does setting
cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?

> +	kfree(cs->hw.ser);
> +	cs->hw.ser = NULL;

I might be missing something, but what does setting this to NULL buy us
here?

(I realize that I'm asking questions to code that isn't actually new but
only moved around, but I think that's still an opportunity to have
another look at that code.)

>  }
>  
>  /*

Thanks,


Paul Bolle

Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Tilman Schmidt]

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

Am 09.12.2015 um 00:12 schrieb Paul Bolle:

>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>> @@ -370,19 +370,23 @@ 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);
> 
> dev equals cs->hw.ser->dev.dev, doesn't it?

Correct.

> So what does setting
> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?

We're freeing cs->hw.ser, not cs->hw.ser->dev.
Clearing the reference to cs from the device structure before freeing cs
guards against possible use-after-free.

>> +	kfree(cs->hw.ser);
>> +	cs->hw.ser = NULL;
> 
> I might be missing something, but what does setting this to NULL buy us
> here?

Just defensive programming. Guarding against possible use-after-free or
double-free.

> 
> (I realize that I'm asking questions to code that isn't actually new but
> only moved around, but I think that's still an opportunity to have
> another look at that code.)

I'm a big fan of one change per patch. If we also want to modify the
moved code then that should be done in a separate patch. It makes
bisecting so much easier. Same reason why I separated out patch 3/3. And
btw same reason why I think patch 1/3 should go in as-is, as an obvious
fix to commit f34d7a5b, and any concerns about whether those tests are
useful should be addressed by a separate patch.

Regards,
Tilman

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

Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Paul Bolle]

On wo, 2015-12-09 at 12:10 +0100, Tilman Schmidt wrote:
> Am 09.12.2015 um 00:12 schrieb Paul Bolle:

> > So what does setting
> > cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy
> > us?
> 
> We're freeing cs->hw.ser, not cs->hw.ser->dev.
> Clearing the reference to cs from the device structure before freeing 
> cs guards against possible use-after-free.
> 
> > > +	kfree(cs->hw.ser);
> > > +	cs->hw.ser = NULL;
> > 
> > I might be missing something, but what does setting this to NULL buy 
> > us here?
> 
> Just defensive programming. Guarding against possible use-after-free 
> or double-free.

I'm inclined to think this is not the best way to guard against such
nasty bugs. But then again, I'm only a few months into my shift of
looking after the gigaset drivers and haven't had to track down such
bugs yet. But I'd be surprised if many other drivers do it that way and
think this is a job for (tree wide) debugging tools. But, whatever the
merits of our views, we can defer this discussion to some future date.
See below.

> I'm a big fan of one change per patch. If we also want to modify the
> moved code then that should be done in a separate patch. It makes
> bisecting so much easier. Same reason why I separated out patch 3/3.


Fair enough.


Paul Bolle

Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Peter Hurley]

Hi Tilman,

On 12/09/2015 03:10 AM, Tilman Schmidt wrote:
> Am 09.12.2015 um 00:12 schrieb Paul Bolle:
> 
>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>> @@ -370,19 +370,23 @@ 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);

This is of marginal value and (I think) unnecessary; it implies
the core will use the device after release, which would trigger
many problems if true.


>> dev equals cs->hw.ser->dev.dev, doesn't it?
> 
> Correct.
> 
>> So what does setting
>> cs->hw.ser->dev.dev.driver_data to NULL just before freeing it buy us?
> 
> We're freeing cs->hw.ser, not cs->hw.ser->dev.
> Clearing the reference to cs from the device structure before freeing cs
> guards against possible use-after-free.
> 
>>> +	kfree(cs->hw.ser);
>>> +	cs->hw.ser = NULL;

This pattern is common, and defends against much more common
driver bugs.

Unfortunately, much of the good this pattern is intended to do in finding
use-after-free bugs is undone by explicit tests for null everywhere else.
Not saying that's the case here; rather, generally speaking.

Like the
	if (!tty && !tty->ops && ....)

code.

Better just to let it crash.

Regards,
Peter Hurley


>> I might be missing something, but what does setting this to NULL buy us
>> here?
> 
> Just defensive programming. Guarding against possible use-after-free or
> double-free.
> 
>>
>> (I realize that I'm asking questions to code that isn't actually new but
>> only moved around, but I think that's still an opportunity to have
>> another look at that code.)
> 
> I'm a big fan of one change per patch. If we also want to modify the
> moved code then that should be done in a separate patch. It makes
> bisecting so much easier. Same reason why I separated out patch 3/3. And
> btw same reason why I think patch 1/3 should go in as-is, as an obvious
> fix to commit f34d7a5b, and any concerns about whether those tests are
> useful should be addressed by a separate patch.
> 
> Regards,
> Tilman
> 

Re: [PATCH 2/3] ser_gigaset: fix deallocation of platform device structure

[Tilman Schmidt]

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

Hi Peter,

Am 10.12.2015 um 15:04 schrieb Peter Hurley:

>>>> --- a/drivers/isdn/gigaset/ser-gigaset.c
>>>> +++ b/drivers/isdn/gigaset/ser-gigaset.c
>>>> @@ -370,19 +370,23 @@ 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);
> 
> This is of marginal value and (I think) unnecessary; it implies
> the core will use the device after release, which would trigger
> many problems if true.

Agreed, but I'm just moving existing code here. Dropping the
dev_set_drvdata() call would be an unrelated change which should be done
in a separate patch if I understand the rules correctly.

Regards,
Tilman

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

[PATCH 1/3] ser_gigaset: fix up NULL checks

[Tilman Schmidt]

Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks
for tty->driver untouched. Fix.

Signed-off-by: Tilman Schmidt <tilman@imap.cc>
Fixes: f34d7a5b7010 ("tty: The big operations rework")
---
 drivers/isdn/gigaset/ser-gigaset.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c
index 375be50..d8771b5 100644
--- a/drivers/isdn/gigaset/ser-gigaset.c
+++ b/drivers/isdn/gigaset/ser-gigaset.c
@@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
 	struct sk_buff *skb = bcs->tx_skb;
 	int sent = -EOPNOTSUPP;
 
-	if (!tty || !tty->driver || !skb)
+	if (!tty || !tty->ops || !skb)
 		return -EINVAL;
 
 	if (!skb->len) {
@@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
 	unsigned long flags;
 	int sent = 0;
 
-	if (!tty || !tty->driver)
+	if (!tty || !tty->ops)
 		return -EFAULT;
 
 	cb = cs->cmdbuf;
@@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs, unsigned old_state,
 	struct tty_struct *tty = cs->hw.ser->tty;
 	unsigned int set, clear;
 
-	if (!tty || !tty->driver || !tty->ops->tiocmset)
+	if (!tty || !tty->ops || !tty->ops->tiocmset)
 		return -EINVAL;
 	set = new_state & ~old_state;
 	clear = old_state & ~new_state;
-- 
1.9.2.459.g68773ac

Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

[Paul Bolle]

Hi Tilman,

On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks

(This makes checkpatch complain, but the correct commit description
style is used in the Fixes: tag, so it's not a big deal.)

> for tty->driver untouched. Fix.
> 
> Signed-off-by: Tilman Schmidt <tilman@imap.cc>
> Fixes: f34d7a5b7010 ("tty: The big operations rework")

Should we backport this all the way to v2.6.32 (currently the oldest
stable tree)?

> diff --git a/drivers/isdn/gigaset/ser-gigaset.c
> b/drivers/isdn/gigaset/ser-gigaset.c
> index 375be50..d8771b5 100644
> --- a/drivers/isdn/gigaset/ser-gigaset.c
> +++ b/drivers/isdn/gigaset/ser-gigaset.c
> @@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs)
>  	struct sk_buff *skb = bcs->tx_skb;
>  	int sent = -EOPNOTSUPP;
>  
> -	if (!tty || !tty->driver || !skb)
> +	if (!tty || !tty->ops || !skb)
>  		return -EINVAL;
>  
>  	if (!skb->len) {
> @@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs)
>  	unsigned long flags;
>  	int sent = 0;
>  
> -	if (!tty || !tty->driver)
> +	if (!tty || !tty->ops)
>  		return -EFAULT;
>  
>  	cb = cs->cmdbuf;
> @@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate
> *cs, unsigned old_state,
>  	struct tty_struct *tty = cs->hw.ser->tty;
>  	unsigned int set, clear;
>  
> -	if (!tty || !tty->driver || !tty->ops->tiocmset)
> +	if (!tty || !tty->ops || !tty->ops->tiocmset)
>  		return -EINVAL;
>  	set = new_state & ~old_state;
>  	clear = old_state & ~new_state;

It's pretty obvious that this should have been part of commit
 f34d7a5b7010 ("tty: The big operations rework"). That being said, these
test puzzle me. It's not obvious why they're needed. Ie, can the null
dereferences they try to catch really happen? But I can try to figure
out that in the future, if I ever feel the urge to do so. Anyhow:

Acked-by: Paul Bolle <pebolle@tiscali.nl>


Paul Bolle

Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

[One Thousand Gnomes]

> Should we backport this all the way to v2.6.32 (currently the oldest
> stable tree)?

We need to be able to explain how the case being tested can occur, then
explain the situation in which it actually prevents a race condition.

If nobody can do that then it shouldn't be backported because its change
without value and just risk. 

The right fix as far as I can see is to remove the tests although
WARN_ON() combined with your tty->ops change might be safer.

> It's pretty obvious that this should have been part of commit
>  f34d7a5b7010 ("tty: The big operations rework"). That being said, these

It ahould probably have been fixed around the same time or in one of the
tty locking reviews, but drivers/isdn and net/irda weren't traditionally
part of the general tty maintenance but handled separately/

> test puzzle me. It's not obvious why they're needed. Ie, can the null
> dereferences they try to catch really happen? But I can try to figure
> out that in the future, if I ever feel the urge to do so. Anyhow:
> 
> Acked-by: Paul Bolle <pebolle@tiscali.nl>

Nacked-by: Alan Cox <alan@linux.intel.com>

Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

[Tilman Schmidt]

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

Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
> The right fix as far as I can see is to remove the tests although
> WARN_ON() combined with your tty->ops change might be safer.

Feel free to submit a patch.

>> It's pretty obvious that this should have been part of commit
>>  f34d7a5b7010 ("tty: The big operations rework"). That being said, these
> 
> It ahould probably have been fixed around the same time or in one of the
> tty locking reviews, but drivers/isdn and net/irda weren't traditionally
> part of the general tty maintenance but handled separately/

Or just ignored.

>> test puzzle me. It's not obvious why they're needed. Ie, can the null
>> dereferences they try to catch really happen? But I can try to figure
>> out that in the future, if I ever feel the urge to do so. Anyhow:
>>
>> Acked-by: Paul Bolle <pebolle@tiscali.nl>
> 
> Nacked-by: Alan Cox <alan@linux.intel.com>

So you feel it's better to maintain the current inconsistent state
created by commit f34d7a5b? Please elaborate.


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

Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

[One Thousand Gnomes]

On Wed, 9 Dec 2015 11:45:57 +0100
Tilman Schmidt <tilman@imap.cc> wrote:

> Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
> > The right fix as far as I can see is to remove the tests although
> > WARN_ON() combined with your tty->ops change might be safer.
> 
> Feel free to submit a patch.

Will do later today. Want a patch on top of Paul's change or a single
patch including both and crediting him ?

> > tty locking reviews, but drivers/isdn and net/irda weren't traditionally
> > part of the general tty maintenance but handled separately/
> 
> Or just ignored.

Unfortunately at the time that seemed to happen a lot.

> >> test puzzle me. It's not obvious why they're needed. Ie, can the null
> >> dereferences they try to catch really happen? But I can try to figure
> >> out that in the future, if I ever feel the urge to do so. Anyhow:
> >>
> >> Acked-by: Paul Bolle <pebolle@tiscali.nl>
> > 
> > Nacked-by: Alan Cox <alan@linux.intel.com>
> 
> So you feel it's better to maintain the current inconsistent state
> created by commit f34d7a5b? Please elaborate.

No I'd rather we didn't make it look magically better then forget about
the mess in question.

Alan

Re: [PATCH 1/3] ser_gigaset: fix up NULL checks

[Paul Bolle]

On wo, 2015-12-09 at 12:12 +0000, One Thousand Gnomes wrote:
> On Wed, 9 Dec 2015 11:45:57 +0100
> Tilman Schmidt <tilman@imap.cc> wrote:
> Want a patch on top of Paul's change or a single
> patch including both and crediting him ?

There's no change that can be attributed to me, I think. We're
discussing a series submitted by Tilman.

Confused,


Paul Bolle

Re: [PATCH 0/3] ser_gigaset fixes

[Paul Bolle]

Hi Tilman,

On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote:
> this series is the result of our discussion on the "freeing an
> active object" bug. I split my proposed patch into two patches
> for the separate topics of moving the ser_cardstate kfree() and
> dropping the useless kfree()s, and also included an unrelated
> patch (1/3) that had fallen through the cracks in my last series.
> 
> Patch 2/3 should go into stable releases all the way back to 2.6.32.
> It applies cleanly to release 3.*/4.* with at most offset 1.
> For release 2.6.32 there is a trivial merge conflict with a removed
> comment line.

1/3 ran into objections and, I think, Alan Cox is working on an
alternative for it. Would you mind resending 2/3 and 3/3 as a two
patches series? Feel free to add
    Acked-by: Paul Bolle <pebolle@tiscali.nl>

to both.

(The previous gigaset series, which you sent in July this year, was
picked up from netdev directly by David Miller. Unless people actually
prefer these patches to also be signed-off by me, I'm perfectly fine
with that.)

Thanks,


Paul Bolle

Re: [PATCH 0/3] ser_gigaset fixes

[Tilman Schmidt]

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

Hi Paul,

Am 10.12.2015 um 12:31 schrieb Paul Bolle:
> 1/3 ran into objections and, I think, Alan Cox is working on an
> alternative for it. Would you mind resending 2/3 and 3/3 as a two
> patches series? Feel free to add
>     Acked-by: Paul Bolle <pebolle@tiscali.nl>
> 
> to both.

Alan's patch dated 09 Dec 2015 20:33:24 applies on top of 1/3 so I
reckon it should be kept.

> (The previous gigaset series, which you sent in July this year, was
> picked up from netdev directly by David Miller. Unless people actually
> prefer these patches to also be signed-off by me, I'm perfectly fine
> with that.)

I think as a maintainer you are supposed to sign off patches for the
code you maintain. My signed-off as recently retired ex-maintainer was
probably still good enough in July. I'm unsure whether it still is today.

Regards,
Tilman

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

Re: [PATCH 0/3] ser_gigaset fixes

[Paul Bolle]

Hi Tilman,

On za, 2015-12-12 at 19:09 +0100, Tilman Schmidt wrote:
> Am 10.12.2015 um 12:31 schrieb Paul Bolle:
> > 1/3 ran into objections and, I think, Alan Cox is working on an
> > alternative for it. Would you mind resending 2/3 and 3/3 as a two
> > patches series? Feel free to add
> >     Acked-by: Paul Bolle <pebolle@tiscali.nl>
> > 
> > to both.
> 
> Alan's patch dated 09 Dec 2015 20:33:24 applies on top of 1/3 so I
> reckon it should be kept.

Which apparently never hit my INBOX. If it had, we wouldn't have been
discussing my request to resend. Thanks for pointing me at that patch!

> > (The previous gigaset series, which you sent in July this year, was
> > picked up from netdev directly by David Miller. Unless people
> > actually
> > prefer these patches to also be signed-off by me, I'm perfectly fine
> > with that.)
> 
> I think as a maintainer you are supposed to sign off patches for the
> code you maintain. My signed-off as recently retired ex-maintainer was
> probably still good enough in July. I'm unsure whether it still is 
> today.

So I'll jot you down as someone preferring that I sign off.

I'll take your series, and Alan's patch - which you just forwarded to me
in a separate message - and kick them around a bit more. Then I expect
to send everything to netdev by next Monday.

Thanks,


Paul Bolle