unbind/bind w1-gpio with device tree produce a crash

PATCH In review Ingo Flaschberger 2 participants 7 messages Mar 4, 2015 – Mar 22, 2016 1 message not in the archive

Subsystems (2)

Stable branch W1 dallas's 1-wire bus

lore

Share & tools

lore

Ingo Flaschberger

author

2015-03-04 03:51 UTC #0 lore raw

If w1-gpio is probed via device-tree configuration, pdata is allocated via devm_kzalloc.
When the device is unbind (and bind later) the allocated memory of pdate is freed - but it will not be allocacted again.

static int w1_gpio_probe(struct platform_device *pdev)

        struct w1_bus_master *master;
        struct w1_gpio_platform_data *pdata = pdev->dev.platform_data;
        int err;

        if(pdata == NULL) {
                if (of_have_populated_dt()) {
                        err = w1_gpio_probe_dt(pdev);
                        if (err < 0)
                                return err;
                }
        }

How to detect if pdata was allocated via device-tree devm_kzalloc and not "hardcoded"? Then I could set pdev->dev.platform_data to NULL in w1_gpio_remove which will solve the crash.

Kind regards,
Ingo Flascherger

Ingo Flaschberger

author

2015-03-04 16:37 UTC #1 lore raw

is it ok, to create a new global variable that tracks if pdata was alloced via devm_kzalloc and sets pdata to NULL in w1_gpio_remove?

Am 04.03.2015 um 04:53 schrieb Ingo Flaschberger:

Quoted 22 lines

If w1-gpio is probed via device-tree configuration, pdata is allocated via devm_kzalloc.
When the device is unbind (and bind later) the allocated memory of pdate is freed - but it will not be allocacted again.
static int w1_gpio_probe(struct platform_device *pdev)
        struct w1_bus_master *master;
        struct w1_gpio_platform_data *pdata = pdev->dev.platform_data;
        int err;

        if(pdata == NULL) {
                if (of_have_populated_dt()) {
                        err = w1_gpio_probe_dt(pdev);
                        if (err < 0)
                                return err;
                }
        }
How to detect if pdata was allocated via device-tree devm_kzalloc and not "hardcoded"? Then I could set pdev->dev.platform_data to NULL in w1_gpio_remove which will solve the crash.
Kind regards,
Ingo Flascherger

A message that is not in the archive (sent off-list or not yet received): [email protected]

Markus Pargmann

2015-03-05 07:35 UTC #3 lore raw

Hi Ingo,

Quoted 7 lines · Ingo Flaschberger

Dear Markus,
as I get no answer at the kernel mailinglist:
your patch to use devm_kzalloc instead of kzalloc produce driver crashes when the 1wire bus is unbound and bound again (see details below).
What do you suggest to solve this problem?

Good point. The easy way to solve this may be to check in the remove function if the devicetree node for this device is available. If it is, we know that platform_data was allocated with devm_* and we can set platform_data to NULL.

I think the better solution would be to create a private structure with the same data as w1_gpio_platform_data. It may even use the same struct. But it should not be stored in platform_data. Instead it should be handled seperately. For DT we can allocate it using devm_kzalloc(). And for probing with pdata, this could just be the pointer to the pdata within the device struct.

Adding the list as CC again.

Best Regards,

Markus

Quoted 45 lines

Kind regards,
Ingo Flaschberger
-------- Weitergeleitete Nachricht --------
Betreff: Re: unbind/bind w1-gpio with device tree produce a crash
Datum: Wed, 04 Mar 2015 17:38:48 +0100
Von: Ingo Flaschberger <[email protected]>
An: [email protected]
is it ok, to create a new global variable that tracks if pdata was alloced via devm_kzalloc and sets pdata to NULL in w1_gpio_remove?
Am 04.03.2015 um 04:53 schrieb Ingo Flaschberger:
Quoted 26 lines
If w1-gpio is probed via device-tree configuration, pdata is allocated via devm_kzalloc.
When the device is unbind (and bind later) the allocated memory of pdate is freed - but it will not be allocacted again.
static int w1_gpio_probe(struct platform_device *pdev)
       struct w1_bus_master *master;
       struct w1_gpio_platform_data *pdata = pdev->dev.platform_data;
       int err;

       if(pdata == NULL) {
               if (of_have_populated_dt()) {
                       err = w1_gpio_probe_dt(pdev);
                       if (err < 0)
                               return err;
               }
       }
How to detect if pdata was allocated via device-tree devm_kzalloc and not "hardcoded"?
Then I could set pdev->dev.platform_data to NULL in w1_gpio_remove which will solve the crash.
Kind regards,
Ingo Flascherger

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Ingo Flaschberger

author

2015-03-05 11:54 UTC #4 lore raw

Dear Markus,

Am 05.03.2015 um 08:35 schrieb Markus Pargmann:

Quoted 5 lines

Good point. The easy way to solve this may be to check in the remove function if the devicetree node for this device is available. If it is, we know that platform_data was allocated with devm_* and we can set platform_data to NULL.

Quoted 8 lines

I think the better solution would be to create a private structure with the same data as w1_gpio_platform_data. It may even use the same struct. But it should not be stored in platform_data. Instead it should be handled seperately. For DT we can allocate it using devm_kzalloc(). And for probing with pdata, this could just be the pointer to the pdata within the device struct.

Could you give me more details about the "private structure" idea, perhaps off-list?

The backup method via w1_gpio_*_orig collides when more than 1 bus is used, this data could be stored in the private structure too.

Kind regards,
Ingo Flaschberger

Markus Pargmann

2015-03-09 10:18 UTC #5 lore raw

PATCH 1/1 w1: gpio: Fix problematic platform_data usage

pdev->dev.platform_data should not be overwritten by a driver. This patch fixes this issue by not using platform_get_pdata to get pdata. The DT routine can then simply return a pdata struct with those information.

Without this patch the platform_data may be freed twice, by devres and the driver framework.

Reported-by Ingo Flaschberger <[email protected]>

Cc <[email protected]>

Signed-off-by Markus Pargmann <[email protected]>

---

Hi,

Could you please try this patch? This essentially does not store anything to platform_data anymore so it should hopefully fix your issue.

Best Regards,

Markus

 drivers/w1/masters/w1-gpio.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

drivers/w1/masters/w1-gpio.c +22 ~~-20~~

index b99a932ad901..63e7b1d2785f 100644
@@ -75,15 +75,18 @@ static struct of_device_id w1_gpio_dt_ids[] = {
 MODULE_DEVICE_TABLE(of, w1_gpio_dt_ids);
 #endif
 
-static int w1_gpio_probe_dt(struct platform_device *pdev)
+static struct w1_gpio_platform_data *w1_gpio_probe_dt(struct platform_device *pdev)
 {
 	struct w1_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct device_node *np = pdev->dev.of_node;
 	int gpio;
 
+	if (pdata)
+		return pdata;
+
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	if (of_get_property(np, "linux,open-drain", NULL))
 		pdata->is_open_drain = 1;

@@ -95,19 +98,17 @@ static int w1_gpio_probe_dt(struct platform_device *pdev)
 					"Failed to parse gpio property for data pin (%d)\n",
 					gpio);
 
-		return gpio;
+		return ERR_PTR(gpio);
 	}
 	pdata->pin = gpio;
 
 	gpio = of_get_gpio(np, 1);
 	if (gpio == -EPROBE_DEFER)
-		return gpio;
+		return ERR_PTR(gpio);
 	/* ignore other errors as the pullup gpio is optional */
 	pdata->ext_pullup_enable_pin = gpio;
 
-	pdev->dev.platform_data = pdata;
-
-	return 0;
+	return pdata;
 }
 
 static int w1_gpio_probe(struct platform_device *pdev)

@@ -116,17 +117,16 @@ static int w1_gpio_probe(struct platform_device *pdev)
 	struct w1_gpio_platform_data *pdata;
 	int err;
 
-	if (of_have_populated_dt()) {
-		err = w1_gpio_probe_dt(pdev);
-		if (err < 0)
-			return err;
-	}
-
-	pdata = dev_get_platdata(&pdev->dev);
+	if (of_have_populated_dt())
+		pdata = w1_gpio_probe_dt(pdev);
+	else
+		pdata = dev_get_platdata(&pdev->dev);
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "No configuration data\n");
-		return -ENXIO;
+	if (IS_ERR_OR_NULL(pdata)) {
+		dev_err(&pdev->dev, "No configuration data or configuration failed\n");
+		if (!pdata)
+			return -ENXIO;
+		return PTR_ERR(pdata);
 	}
 
 	master = devm_kzalloc(&pdev->dev, sizeof(struct w1_bus_master),

@@ -185,7 +185,7 @@ static int w1_gpio_probe(struct platform_device *pdev)
 static int w1_gpio_remove(struct platform_device *pdev)
 {
 	struct w1_bus_master *master = platform_get_drvdata(pdev);
-	struct w1_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct w1_gpio_platform_data *pdata = master->data;
 
 	if (pdata->enable_external_pullup)
 		pdata->enable_external_pullup(0);

@@ -202,7 +202,8 @@ static int w1_gpio_remove(struct platform_device *pdev)
 
 static int w1_gpio_suspend(struct platform_device *pdev, pm_message_t state)
 {
-	struct w1_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct w1_bus_master *master = platform_get_drvdata(pdev);
+	struct w1_gpio_platform_data *pdata = master->data;
 
 	if (pdata->enable_external_pullup)
 		pdata->enable_external_pullup(0);

@@ -212,7 +213,8 @@ static int w1_gpio_suspend(struct platform_device *pdev, pm_message_t state)
 
 static int w1_gpio_resume(struct platform_device *pdev)
 {
-	struct w1_gpio_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct w1_bus_master *master = platform_get_drvdata(pdev);
+	struct w1_gpio_platform_data *pdata = master->data;
 
 	if (pdata->enable_external_pullup)
 		pdata->enable_external_pullup(1);

Signature · 1 line

2.1.4

Markus Pargmann

2015-04-10 10:22 UTC #6 lore raw

Re: [PATCH] w1: gpio: Fix problematic platform_data usage

Hi,

Quoted 10 lines · Markus Pargmann

pdev->dev.platform_data should not be overwritten by a driver. This patch fixes this issue by not using platform_get_pdata to get pdata. The DT routine can then simply return a pdata struct with those information.
Without this patch the platform_data may be freed twice, by devres and the driver framework.
Reported-by Ingo Flaschberger <[email protected]>
Cc <[email protected]>
Signed-off-by Markus Pargmann <[email protected]>

Did you have time to test this? I would like to send this mainline but a Tested-by of you would be nice.

Best Regards,

Markus

Signature · 4 lines

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Ingo Flaschberger

author

2016-03-22 11:59 UTC #7 lore raw

Re: [PATCH] w1: gpio: Fix problematic platform_data usage

Dear Markus,

sorry - missed your reply last year.
The patch seems to work.

Kind regards,
Ingo Flaschberger

Quoted 26 lines · Markus Pargmann

Hi,
Quoted 10 lines · Markus Pargmann
pdev->dev.platform_data should not be overwritten by a driver. This patch fixes this issue by not using platform_get_pdata to get pdata. The DT routine can then simply return a pdata struct with those information.
Without this patch the platform_data may be freed twice, by devres and the driver framework.
Reported-by Ingo Flaschberger <[email protected]>
Cc <[email protected]>
Signed-off-by Markus Pargmann <[email protected]>
Did you have time to test this? I would like to send this mainline but a Tested-by of you would be nice.
Best Regards,
Markus
--
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |