ser_gigaset fixes

PATCH NAK Tilman Schmidt 4 participants 18 messages Dec 8, 2015 – Dec 12, 2015

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Tilman Schmidt

author

2015-12-08 11:00 UTC #0 lore raw

cover ser_gigaset fixes

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

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2015-12-08 11:00 UTC #1 lore raw

PATCH 1/3 ser_gigaset: fix up NULL checks

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

Signed-off-by Tilman Schmidt <[email protected]>

Fixes f34d7a5b7010 ("tty: The big operations rework")

---
 drivers/isdn/gigaset/ser-gigaset.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/isdn/gigaset/ser-gigaset.c +3 -3

index 375be50..d8771b5 100644
@@ -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;

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2015-12-08 11:00 UTC #7 lore raw

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

In reply to 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 <[email protected]>

Fixes 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")

Reported-by Sasha Levin <[email protected]>

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

drivers/isdn/gigaset/ser-gigaset.c +7 -3

index d8771b5..2693cb2 100644
@@ -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;
 }
 
 /*

Signature · 1 line

1.9.2.459.g68773ac

Tilman Schmidt

author

2015-12-08 11:00 UTC #13 lore raw

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

In reply to 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 <[email protected]>

Reported-by Paul Bolle <[email protected]>

---
 drivers/isdn/gigaset/ser-gigaset.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/isdn/gigaset/ser-gigaset.c +0 -5

index 2693cb2..b7e0329 100644
@@ -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);

Signature · 1 line

1.9.2.459.g68773ac

Paul Bolle

Acked-by on v1 1/3

2015-12-08 19:45 UTC #2 lore raw

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

In reply to Tilman Schmidt

Hi Tilman,

Quoted 1 line · Tilman Schmidt

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

Quoted 4 lines

for tty->driver untouched. Fix.
Signed-off-by Tilman Schmidt <[email protected]>
Fixes f34d7a5b7010 ("tty: The big operations rework")

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

Quoted 32 lines

drivers/isdn/gigaset/ser-gigaset.c +3 -3

index 375be50..d8771b5 100644
@@ -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;

Quoted 1 line

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 <[email protected]>

Paul Bolle

One Thousand Gnomes

Nacked-by Alan Cox NAK

2015-12-08 22:16 UTC #3 lore raw

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

Quoted 2 lines

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.

Quoted 2 lines

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/

Quoted 5 lines

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 <[email protected]>

Nacked-by Alan Cox <[email protected]>

Paul Bolle

2015-12-08 23:12 UTC #8 lore raw

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

In reply to Tilman Schmidt

Hi Tilman,

Quoted 35 lines · 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 <[email protected]>
Fixes 2869b23e4b95 ("drivers/isdn/gigaset: new M101 driver (v2)")
Reported-by Sasha Levin <[email protected]>
---
 drivers/isdn/gigaset/ser-gigaset.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/isdn/gigaset/ser-gigaset.c +1 -3
index d8771b5..2693cb2 100644
@@ -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 */

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 387

Quoted 6 lines

drivers/isdn/gigaset/ser-gigaset.c +4 -0

 	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?

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 389

Quoted 2 lines

drivers/isdn/gigaset/ser-gigaset.c +2 -0
+	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.)

Quoted 3 lines

}
/*

Thanks,

Paul Bolle

2015-12-08 23:15 UTC #14 lore raw

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

In reply to Tilman Schmidt

Hi Tilman,

Quoted 6 lines · 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 <[email protected]>
Reported-by Paul Bolle <[email protected]>

s/Reported-by/Acked-by/

(Having both lines would be overdoing things.)

Thanks,

Paul Bolle

Tilman Schmidt

author

2015-12-09 10:45 UTC #4 lore raw

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

In reply to One Thousand Gnomes

Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>
Nacked-by Alan Cox <[email protected]>

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

Tilman Schmidt

author

2015-12-09 11:10 UTC #9 lore raw

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

In reply to Paul Bolle

Am 09.12.2015 um 00:12 schrieb Paul Bolle:

Quoted 18 lines

drivers/isdn/gigaset/ser-gigaset.c +1 -3

@@ -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);

Quoted 10 lines

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 387
Quoted 8 lines
drivers/isdn/gigaset/ser-gigaset.c +0 -0
 	/* adapted from platform_device_release() in
drivers/base/platform.c */
drivers/isdn/gigaset/ser-gigaset.c +4 -0
 	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.

Quoted 2 lines

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.

Quoted 5 lines

drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 389
Quoted 2 lines
drivers/isdn/gigaset/ser-gigaset.c +2 -0
+	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.

Quoted 4 lines

(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

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

One Thousand Gnomes

2015-12-09 12:13 UTC #5 lore raw

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

In reply to Tilman Schmidt

Quoted 5 lines · On Wed, 9 Dec 2015 11:45:57 +0100 Tilman Schmidt <[email protected]> wrote:

Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes:
Quoted 2 lines
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 ?

Quoted 4 lines

Quoted 2 lines
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.

Quoted 10 lines

Quoted 7 lines
Quoted 5 lines
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 <[email protected]>
Nacked-by Alan Cox <[email protected]>
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

Paul Bolle

2015-12-09 19:18 UTC #6 lore raw

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

Quoted 4 lines · One Thousand Gnomes

On Wed, 9 Dec 2015 11:45:57 +0100
Tilman Schmidt <[email protected]> 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

2015-12-10 11:20 UTC #10 lore raw

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

In reply to Tilman Schmidt

Quoted 10 lines · Tilman Schmidt

Am 09.12.2015 um 00:12 schrieb Paul Bolle:
Quoted 3 lines
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.

Quoted 8 lines

Quoted 5 lines
drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 389
Quoted 2 lines
drivers/isdn/gigaset/ser-gigaset.c +2 -0
+	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.

Quoted 3 lines

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

Acked-by on the v1 series

2015-12-10 11:31 UTC #15 lore raw

In reply to Tilman Schmidt

Hi Tilman,

Quoted 10 lines · Tilman Schmidt

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 <[email protected]>

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

Peter Hurley

2015-12-10 14:04 UTC #11 lore raw

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

In reply to Tilman Schmidt

Hi Tilman,

Quoted 20 lines · Tilman Schmidt

Am 09.12.2015 um 00:12 schrieb Paul Bolle:

Quoted 18 lines

drivers/isdn/gigaset/ser-gigaset.c +1 -3

@@ -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);

Quoted 8 lines

Quoted 8 lines
drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 387
Quoted 8 lines
drivers/isdn/gigaset/ser-gigaset.c +0 -0
 	/* adapted from platform_device_release() in
drivers/base/platform.c */
drivers/isdn/gigaset/ser-gigaset.c +4 -0
 	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.

Quoted 7 lines

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

Quoted 6 lines

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.
Quoted 2 lines
drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 389
Quoted 2 lines
drivers/isdn/gigaset/ser-gigaset.c +2 -0
+	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

Quoted 21 lines

Quoted 2 lines
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.
Quoted 4 lines
(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

author

2015-12-12 17:52 UTC #12 lore raw

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

Hi Peter,

Am 10.12.2015 um 15:04 schrieb Peter Hurley:

Quoted 18 lines

drivers/isdn/gigaset/ser-gigaset.c +1 -3

@@ -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);

Quoted 12 lines

Quoted 8 lines
Quoted 8 lines
drivers/isdn/gigaset/ser-gigaset.c static void gigaset_freecshw(struct cardstate *cs) · line 387
Quoted 8 lines
drivers/isdn/gigaset/ser-gigaset.c +0 -0
 	/* adapted from platform_device_release() in
drivers/base/platform.c */
drivers/isdn/gigaset/ser-gigaset.c +4 -0
 	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

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Tilman Schmidt

author

2015-12-12 18:09 UTC #16 lore raw

In reply to Paul Bolle

Hi Paul,

Am 10.12.2015 um 12:31 schrieb Paul Bolle:

Quoted 6 lines

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 <[email protected]>
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.

Quoted 4 lines

(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

Signature · 3 lines

Tilman Schmidt                              E-Mail: [email protected]
Bonn, Germany
Nous, on a des fleurs et des bougies pour nous protéger.

Paul Bolle

2015-12-12 18:32 UTC #17 lore raw

Hi Tilman,

Quoted 10 lines · Tilman Schmidt

Am 10.12.2015 um 12:31 schrieb Paul Bolle:
Quoted 6 lines
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 <[email protected]>
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!

Quoted 10 lines

Quoted 5 lines
(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