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

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

[Ingo Flaschberger]

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

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

[Ingo Flaschberger]

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

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

[Markus Pargmann]

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

Hi Ingo,

On Thu, Mar 05, 2015 at 03:56:20AM +0100, Ingo Flaschberger wrote:
> 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

> 
> 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 <ingo.flaschberger@gmail.com>
> An: 	linux-kernel@vger.kernel.org
> 
> 
> 
> 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:
> >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
> >
> 
> 
> 

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Ingo Flaschberger]

Dear Markus,

Am 05.03.2015 um 08:35 schrieb Markus Pargmann:
> 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.
>
>


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

[PATCH] w1: gpio: Fix problematic platform_data usage

[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 <ingo.flaschberger@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---

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

diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index b99a932ad901..63e7b1d2785f 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -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);
-- 
2.1.4

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

[Markus Pargmann]

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

Hi,

On Mon, Mar 09, 2015 at 11:18:58AM +0100, Markus Pargmann wrote:
> 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 <ingo.flaschberger@gmail.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

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 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Ingo Flaschberger]

Dear Markus,

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

Kind regards,
   Ingo Flaschberger

On Fri, Apr 10, 2015 at 12:22 PM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> On Mon, Mar 09, 2015 at 11:18:58AM +0100, Markus Pargmann wrote:
>> 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 <ingo.flaschberger@gmail.com>
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
>
> 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 |