Re: [RFC] device.h: add device_set_platdata routine

Re: [RFC] device.h: add device_set_platdata routine

[Uwe Kleine-König]

[added gregkh and lkml to Cc:]

Hi Viresh,

On Tue, Mar 01, 2011 at 10:03:20AM +0530, Viresh Kumar wrote:
> device.h supports device_get_platdata but doesn't support device_set_platdata.
> This routine is required by platforms in which device structure is declared
> in a machine specific file and platform data comes from board specific file.
> 
> This will be used by SPEAr patches sent in separate patch series.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>  include/linux/device.h |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 1bf5cf0..6ce0f20 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -576,6 +576,11 @@ static inline void *dev_get_platdata(const struct device *dev)
>  	return dev->platform_data;
>  }
>  
> +static inline void dev_set_platdata(struct device *dev, void *platdata)
> +{
> +	dev->platform_data = platdata;
> +}
> +
Note that dev->platform_data was designed to hold dynamically allocated
memory, at least it's kfreed in platform_device_release.  And note there
is platform_device_add_data that kmemdups its argument into
pdev->dev.platform_data.

Compared to your dev_set_platdata platform_device_add_data only works
for platform_devices, don't know if it's worth to change that.

And regarding platform_device_add_data I wonder if it wouldn't be more
consistent to set platform_data = NULL if (!data)?  Greg?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC] device.h: add device_set_platdata routine

[viresh kumar]

On 03/01/2011 01:29 PM, Uwe Kleine-König wrote:
> [added gregkh and lkml to Cc:]
> 

thanks.

> On Tue, Mar 01, 2011 at 10:03:20AM +0530, Viresh Kumar wrote:
>> device.h supports device_get_platdata but doesn't support device_set_platdata.
>> This routine is required by platforms in which device structure is declared
>> in a machine specific file and platform data comes from board specific file.
>>
>> This will be used by SPEAr patches sent in separate patch series.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
>> ---
>>  include/linux/device.h |    5 +++++
>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 1bf5cf0..6ce0f20 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -576,6 +576,11 @@ static inline void *dev_get_platdata(const struct device *dev)
>>  	return dev->platform_data;
>>  }
>>  
>> +static inline void dev_set_platdata(struct device *dev, void *platdata)
>> +{
>> +	dev->platform_data = platdata;
>> +}
>> +
> Note that dev->platform_data was designed to hold dynamically allocated
> memory, at least it's kfreed in platform_device_release.  And note there
> is platform_device_add_data that kmemdups its argument into
> pdev->dev.platform_data.
> 

Ok. So we should use platform_device_add_data instead and mark our platform
data's struct as __init. So that it doesn't consume any memory after
this routine is done??

> Compared to your dev_set_platdata platform_device_add_data only works
> for platform_devices, don't know if it's worth to change that.
> 

Currently i need this for platform devs only. So its good enough for me.

-- 
viresh

Re: [RFC] device.h: add device_set_platdata routine

[Uwe Kleine-König]

Hi Viresh,

> > On Tue, Mar 01, 2011 at 10:03:20AM +0530, Viresh Kumar wrote:
> >> device.h supports device_get_platdata but doesn't support device_set_platdata.
> >> This routine is required by platforms in which device structure is declared
> >> in a machine specific file and platform data comes from board specific file.
> >>
> >> This will be used by SPEAr patches sent in separate patch series.
> >>
> >> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> >> ---
> >>  include/linux/device.h |    5 +++++
> >>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/linux/device.h b/include/linux/device.h
> >> index 1bf5cf0..6ce0f20 100644
> >> --- a/include/linux/device.h
> >> +++ b/include/linux/device.h
> >> @@ -576,6 +576,11 @@ static inline void *dev_get_platdata(const struct device *dev)
> >>  	return dev->platform_data;
> >>  }
> >>  
> >> +static inline void dev_set_platdata(struct device *dev, void *platdata)
> >> +{
> >> +	dev->platform_data = platdata;
> >> +}
> >> +
> > Note that dev->platform_data was designed to hold dynamically allocated
> > memory, at least it's kfreed in platform_device_release.  And note there
> > is platform_device_add_data that kmemdups its argument into
> > pdev->dev.platform_data.
> > 
> 
> Ok. So we should use platform_device_add_data instead and mark our platform
> data's struct as __init. So that it doesn't consume any memory after
> this routine is done??
Yeah, either __initdata or still better const + __initconst if possible.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RFC] device.h: add device_set_platdata routine

[viresh kumar]

On 03/01/2011 02:35 PM, Uwe Kleine-König wrote:
>> > Ok. So we should use platform_device_add_data instead and mark our platform
>> > data's struct as __init. So that it doesn't consume any memory after
>> > this routine is done??
> Yeah, either __initdata or still better const + __initconst if possible.

Ok. Thanks for your help.

-- 
viresh

Re: [RFC] device.h: add device_set_platdata routine

[Greg KH]

On Tue, Mar 01, 2011 at 08:59:53AM +0100, Uwe Kleine-König wrote:
> [added gregkh and lkml to Cc:]
> 
> Hi Viresh,
> 
> On Tue, Mar 01, 2011 at 10:03:20AM +0530, Viresh Kumar wrote:
> > device.h supports device_get_platdata but doesn't support device_set_platdata.
> > This routine is required by platforms in which device structure is declared
> > in a machine specific file and platform data comes from board specific file.
> > 
> > This will be used by SPEAr patches sent in separate patch series.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> > ---
> >  include/linux/device.h |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1bf5cf0..6ce0f20 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -576,6 +576,11 @@ static inline void *dev_get_platdata(const struct device *dev)
> >  	return dev->platform_data;
> >  }
> >  
> > +static inline void dev_set_platdata(struct device *dev, void *platdata)
> > +{
> > +	dev->platform_data = platdata;
> > +}
> > +
> Note that dev->platform_data was designed to hold dynamically allocated
> memory, at least it's kfreed in platform_device_release.  And note there
> is platform_device_add_data that kmemdups its argument into
> pdev->dev.platform_data.
> 
> Compared to your dev_set_platdata platform_device_add_data only works
> for platform_devices, don't know if it's worth to change that.
> 
> And regarding platform_device_add_data I wonder if it wouldn't be more
> consistent to set platform_data = NULL if (!data)?  Greg?

Maybe, care to send a patch?

thanks,

greg k-h

[PATCH 1/5] driver core/platform_device_add_data: set platform_data to NULL if !data

[Uwe Kleine-König]

This makes the data = NULL case more consistent to the data != NULL case.
The functional change is that now

	platform_device_add_data(somepdev, NULL, somesize)

sets pdev->dev.platform_data to NULL instead of not touching it.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

if you think I should squash some of the first four patches together
just tell me.

Best regards
Uwe

 drivers/base/platform.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f051cff..f836f2e 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -219,17 +219,16 @@ EXPORT_SYMBOL_GPL(platform_device_add_resources);
 int platform_device_add_data(struct platform_device *pdev, const void *data,
 			     size_t size)
 {
-	void *d;
+	void *d = NULL;
 
-	if (!data)
-		return 0;
-
-	d = kmemdup(data, size, GFP_KERNEL);
-	if (d) {
-		pdev->dev.platform_data = d;
-		return 0;
+	if (data) {
+		d = kmemdup(data, size, GFP_KERNEL);
+		if (!d)
+			return -ENOMEM;
 	}
-	return -ENOMEM;
+
+	pdev->dev.platform_data = d;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_device_add_data);
 
-- 
1.7.2.3

Re: [PATCH 1/5] driver core/platform_device_add_data: set platform_data to NULL if !data

[Uwe Kleine-König]

Hi Greg,

do you care for the Reviewed-by: tag from Viresh or should I resend?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/5] driver core/platform_device_add_data: set platform_data to NULL if !data

[Greg KH]

On Mon, Apr 11, 2011 at 08:50:35PM +0200, Uwe Kleine-König wrote:
> Hi Greg,
> 
> do you care for the Reviewed-by: tag from Viresh or should I resend?

Feel free to resend, I'm still working through my patch backlog for
being away for a few weeks...

thanks,

greg k-h

[PATCH 2/5] driver core/platform_device_add_data: free platform data before overwriting

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f836f2e..01caf61 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -227,6 +227,7 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
 			return -ENOMEM;
 	}
 
+	kfree(pdev->dev.platform_data);
 	pdev->dev.platform_data = d;
 	return 0;
 }
-- 
1.7.2.3

[PATCH 3/5] driver core/platform_device_add_resources: set resource to NULL if !res

[Uwe Kleine-König]

This makes the res = NULL case more consistant to the res != NULL case
as now both overwrite pdev->resource.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 01caf61..812d8ff 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -191,18 +191,17 @@ EXPORT_SYMBOL_GPL(platform_device_alloc);
 int platform_device_add_resources(struct platform_device *pdev,
 				  const struct resource *res, unsigned int num)
 {
-	struct resource *r;
+	struct resource *r = NULL;
 
-	if (!res)
-		return 0;
-
-	r = kmemdup(res, sizeof(struct resource) * num, GFP_KERNEL);
-	if (r) {
-		pdev->resource = r;
-		pdev->num_resources = num;
-		return 0;
+	if (res) {
+		r = kmemdup(res, sizeof(struct resource) * num, GFP_KERNEL);
+		if (!r)
+			return -ENOMEM;
 	}
-	return -ENOMEM;
+
+	pdev->resource = r;
+	pdev->num_resources = num;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_device_add_resources);
 
-- 
1.7.2.3

[PATCH 4/5] driver core/platform_device_add_resources: free resource before overwriting

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 812d8ff..4a73cbc 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -199,6 +199,7 @@ int platform_device_add_resources(struct platform_device *pdev,
 			return -ENOMEM;
 	}
 
+	kfree(pdev->resource);
 	pdev->resource = r;
 	pdev->num_resources = num;
 	return 0;
-- 
1.7.2.3

[PATCH 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I wasn't sure what to return when dev_set_drvdata is called with
dev=NULL.  I choosed 0, but -EINVAL would be OK for me, too. What do you
think?

And we could add __must_check, maybe do this later =:-)

Best regards
Uwe

 drivers/base/dd.c      |    8 +++++---
 include/linux/device.h |    2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index da57ee9..29ff339 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -408,17 +408,19 @@ void *dev_get_drvdata(const struct device *dev)
 }
 EXPORT_SYMBOL(dev_get_drvdata);
 
-void dev_set_drvdata(struct device *dev, void *data)
+int dev_set_drvdata(struct device *dev, void *data)
 {
 	int error;
 
 	if (!dev)
-		return;
+		return 0;
+
 	if (!dev->p) {
 		error = device_private_init(dev);
 		if (error)
-			return;
+			return error;
 	}
 	dev->p->driver_data = data;
+	return 0;
 }
 EXPORT_SYMBOL(dev_set_drvdata);
diff --git a/include/linux/device.h b/include/linux/device.h
index 1bf5cf0..9e754c7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -558,7 +558,7 @@ extern int device_move(struct device *dev, struct device *new_parent,
 extern const char *device_get_devnode(struct device *dev,
 				      mode_t *mode, const char **tmp);
 extern void *dev_get_drvdata(const struct device *dev);
-extern void dev_set_drvdata(struct device *dev, void *data);
+extern int dev_set_drvdata(struct device *dev, void *data);
 
 /*
  * Root device objects for grouping under /sys/devices
-- 
1.7.2.3

Re: [PATCH 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Michał Mirosław]

2011/4/6 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> I wasn't sure what to return when dev_set_drvdata is called with
> dev=NULL.  I choosed 0, but -EINVAL would be OK for me, too. What do you
> think?

Why not just BUG_ON(!dev)? Is there a case when you might call this
with dev==NULL that's not a driver bug?

Best Regards,
Michał Mirosław

Re: [PATCH 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Uwe Kleine-König]

Hello,

On Wed, Apr 06, 2011 at 11:36:46AM +0200, Michał Mirosław wrote:
> 2011/4/6 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> >
> > I wasn't sure what to return when dev_set_drvdata is called with
> > dev=NULL.  I choosed 0, but -EINVAL would be OK for me, too. What do you
> > think?
> 
> Why not just BUG_ON(!dev)? Is there a case when you might call this
> with dev==NULL that's not a driver bug?
I think BUG_ON is too harsh. Will resend with -EINVAL.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Michał Mirosław]

2011/4/6 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
> On Wed, Apr 06, 2011 at 11:36:46AM +0200, Michał Mirosław wrote:
>> 2011/4/6 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> > ---
>> > Hello,
>> >
>> > I wasn't sure what to return when dev_set_drvdata is called with
>> > dev=NULL.  I choosed 0, but -EINVAL would be OK for me, too. What do you
>> > think?
>> Why not just BUG_ON(!dev)? Is there a case when you might call this
>> with dev==NULL that's not a driver bug?
> I think BUG_ON is too harsh. Will resend with -EINVAL.

Maybe just WARN_ON, then? Please? ;-)

Error return with no other visible sign is easy to miss for driver
writers. Big bad backtrace in dmesg on the other hand attracts
attention.

Best Regards,
Michał Mirosław

[PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Uwe Kleine-König]

Before commit

	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)

calling dev_set_drvdata with dev=NULL was an unchecked error. After some
discussion about what to return in this case removing the check (and so
producing a null pointer exception) seems fine.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

@Viresh: as I didn't choose to return -EINVAL I didn't add your Reviewed-by:

Best regards
Uwe
 drivers/base/dd.c      |    7 +++----
 include/linux/device.h |    2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index da57ee9..f9d69d7 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -408,17 +408,16 @@ void *dev_get_drvdata(const struct device *dev)
 }
 EXPORT_SYMBOL(dev_get_drvdata);
 
-void dev_set_drvdata(struct device *dev, void *data)
+int dev_set_drvdata(struct device *dev, void *data)
 {
 	int error;
 
-	if (!dev)
-		return;
 	if (!dev->p) {
 		error = device_private_init(dev);
 		if (error)
-			return;
+			return error;
 	}
 	dev->p->driver_data = data;
+	return 0;
 }
 EXPORT_SYMBOL(dev_set_drvdata);
diff --git a/include/linux/device.h b/include/linux/device.h
index ab8dfc0..e5e07eb 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -557,7 +557,7 @@ extern int device_move(struct device *dev, struct device *new_parent,
 extern const char *device_get_devnode(struct device *dev,
 				      mode_t *mode, const char **tmp);
 extern void *dev_get_drvdata(const struct device *dev);
-extern void dev_set_drvdata(struct device *dev, void *data);
+extern int dev_set_drvdata(struct device *dev, void *data);
 
 /*
  * Root device objects for grouping under /sys/devices
-- 
1.7.2.3

Re: [PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Greg KH]

On Mon, Apr 11, 2011 at 08:42:58PM +0200, Uwe Kleine-König wrote:
> Before commit
> 
> 	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> 
> calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> discussion about what to return in this case removing the check (and so
> producing a null pointer exception) seems fine.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> @Viresh: as I didn't choose to return -EINVAL I didn't add your Reviewed-by:

I'm confused by this thread, care to resend all of these in a series
against the latest linux-next tree?

thanks,

greg k-h

Re: [PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Uwe Kleine-König]

On Tue, Apr 19, 2011 at 04:58:12PM -0700, Greg KH wrote:
> On Mon, Apr 11, 2011 at 08:42:58PM +0200, Uwe Kleine-König wrote:
> > Before commit
> > 
> > 	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> > 
> > calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> > discussion about what to return in this case removing the check (and so
> > producing a null pointer exception) seems fine.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > @Viresh: as I didn't choose to return -EINVAL I didn't add your Reviewed-by:
> 
> I'm confused by this thread, care to resend all of these in a series
> against the latest linux-next tree?
No problem, here it comes. This are patches 1 to 4 of the original
series + v2 of patch 5 (that didn't have 5/5 in the subject).

Best regards,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 1/5] driver core/platform_device_add_data: set platform_data to NULL if !data

[Uwe Kleine-König]

This makes the data = NULL case more consistent to the data != NULL case.
The functional change is that now

	platform_device_add_data(somepdev, NULL, somesize)

sets pdev->dev.platform_data to NULL instead of not touching it.

Reviewed-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 9e0e4fc..65cb4c3 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -220,17 +220,16 @@ EXPORT_SYMBOL_GPL(platform_device_add_resources);
 int platform_device_add_data(struct platform_device *pdev, const void *data,
 			     size_t size)
 {
-	void *d;
+	void *d = NULL;
 
-	if (!data)
-		return 0;
-
-	d = kmemdup(data, size, GFP_KERNEL);
-	if (d) {
-		pdev->dev.platform_data = d;
-		return 0;
+	if (data) {
+		d = kmemdup(data, size, GFP_KERNEL);
+		if (!d)
+			return -ENOMEM;
 	}
-	return -ENOMEM;
+
+	pdev->dev.platform_data = d;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_device_add_data);
 
-- 
1.7.4.1

[PATCH v2 2/5] driver core/platform_device_add_data: free platform data before overwriting

[Uwe Kleine-König]

Reviewed-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 65cb4c3..58ad8e8 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -228,6 +228,7 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
 			return -ENOMEM;
 	}
 
+	kfree(pdev->dev.platform_data);
 	pdev->dev.platform_data = d;
 	return 0;
 }
-- 
1.7.4.1

[PATCH v2 3/5] driver core/platform_device_add_resources: set resource to NULL if !res

[Uwe Kleine-König]

This makes the res = NULL case more consistant to the res != NULL case
as now both overwrite pdev->resource.

Reviewed-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 58ad8e8..667f282 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -192,18 +192,17 @@ EXPORT_SYMBOL_GPL(platform_device_alloc);
 int platform_device_add_resources(struct platform_device *pdev,
 				  const struct resource *res, unsigned int num)
 {
-	struct resource *r;
+	struct resource *r = NULL;
 
-	if (!res)
-		return 0;
-
-	r = kmemdup(res, sizeof(struct resource) * num, GFP_KERNEL);
-	if (r) {
-		pdev->resource = r;
-		pdev->num_resources = num;
-		return 0;
+	if (res) {
+		r = kmemdup(res, sizeof(struct resource) * num, GFP_KERNEL);
+		if (!r)
+			return -ENOMEM;
 	}
-	return -ENOMEM;
+
+	pdev->resource = r;
+	pdev->num_resources = num;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(platform_device_add_resources);
 
-- 
1.7.4.1

[PATCH v2 4/5] driver core/platform_device_add_resources: free resource before overwriting

[Uwe Kleine-König]

Reviewed-by: Viresh Kumar <viresh.kumar@st.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/platform.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 667f282..7d4bdaf 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -200,6 +200,7 @@ int platform_device_add_resources(struct platform_device *pdev,
 			return -ENOMEM;
 	}
 
+	kfree(pdev->resource);
 	pdev->resource = r;
 	pdev->num_resources = num;
 	return 0;
-- 
1.7.4.1

[PATCH v2 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Uwe Kleine-König]

Before commit

	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)

calling dev_set_drvdata with dev=NULL was an unchecked error. After some
discussion about what to return in this case removing the check (and so
producing a null pointer exception) seems fine.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/base/dd.c      |    7 +++----
 include/linux/device.h |    2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index da57ee9..f9d69d7 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -408,17 +408,16 @@ void *dev_get_drvdata(const struct device *dev)
 }
 EXPORT_SYMBOL(dev_get_drvdata);
 
-void dev_set_drvdata(struct device *dev, void *data)
+int dev_set_drvdata(struct device *dev, void *data)
 {
 	int error;
 
-	if (!dev)
-		return;
 	if (!dev->p) {
 		error = device_private_init(dev);
 		if (error)
-			return;
+			return error;
 	}
 	dev->p->driver_data = data;
+	return 0;
 }
 EXPORT_SYMBOL(dev_set_drvdata);
diff --git a/include/linux/device.h b/include/linux/device.h
index 350ceda..2215d01 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -557,7 +557,7 @@ extern int device_move(struct device *dev, struct device *new_parent,
 extern const char *device_get_devnode(struct device *dev,
 				      mode_t *mode, const char **tmp);
 extern void *dev_get_drvdata(const struct device *dev);
-extern void dev_set_drvdata(struct device *dev, void *data);
+extern int dev_set_drvdata(struct device *dev, void *data);
 
 /*
  * Root device objects for grouping under /sys/devices
-- 
1.7.4.1

Re: [PATCH v2 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Russell King - ARM Linux]

On Wed, Apr 20, 2011 at 09:44:46AM +0200, Uwe Kleine-König wrote:
> Before commit
> 
> 	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> 
> calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> discussion about what to return in this case removing the check (and so
> producing a null pointer exception) seems fine.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/base/dd.c      |    7 +++----
>  include/linux/device.h |    2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index da57ee9..f9d69d7 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -408,17 +408,16 @@ void *dev_get_drvdata(const struct device *dev)
>  }
>  EXPORT_SYMBOL(dev_get_drvdata);
>  
> -void dev_set_drvdata(struct device *dev, void *data)
> +int dev_set_drvdata(struct device *dev, void *data)
>  {
>  	int error;
>  
> -	if (!dev)
> -		return;
>  	if (!dev->p) {
>  		error = device_private_init(dev);
>  		if (error)
> -			return;
> +			return error;
>  	}
>  	dev->p->driver_data = data;
> +	return 0;

Who is going to modify all the thousands of drivers we have in the kernel
tree to check this return value?

If the answer is no one, its pointless returning an error value in the
first place (which I think is what the original author already thought
about.)

Re: [PATCH v2 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Uwe Kleine-König]

Hello,

On Fri, Apr 29, 2011 at 09:12:57AM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 20, 2011 at 09:44:46AM +0200, Uwe Kleine-König wrote:
> > Before commit
> > 
> > 	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> > 
> > calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> > discussion about what to return in this case removing the check (and so
> > producing a null pointer exception) seems fine.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/base/dd.c      |    7 +++----
> >  include/linux/device.h |    2 +-
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index da57ee9..f9d69d7 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -408,17 +408,16 @@ void *dev_get_drvdata(const struct device *dev)
> >  }
> >  EXPORT_SYMBOL(dev_get_drvdata);
> >  
> > -void dev_set_drvdata(struct device *dev, void *data)
> > +int dev_set_drvdata(struct device *dev, void *data)
> >  {
> >  	int error;
> >  
> > -	if (!dev)
> > -		return;
> >  	if (!dev->p) {
> >  		error = device_private_init(dev);
> >  		if (error)
> > -			return;
> > +			return error;
> >  	}
> >  	dev->p->driver_data = data;
> > +	return 0;
> 
> Who is going to modify all the thousands of drivers we have in the kernel
> tree to check this return value?
> 
> If the answer is no one, its pointless returning an error value in the
> first place (which I think is what the original author already thought
> about.)
In the meantime I learned that dev->p is valid when the device is
registered. As calling dev_set_drvdata on an unregisted device is not
allowed maybe issuing a warning instead would be OK for me, too.

Thoughts?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Greg KH]

On Wed, Apr 20, 2011 at 09:42:48AM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 19, 2011 at 04:58:12PM -0700, Greg KH wrote:
> > On Mon, Apr 11, 2011 at 08:42:58PM +0200, Uwe Kleine-König wrote:
> > > Before commit
> > > 
> > > 	b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> > > 
> > > calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> > > discussion about what to return in this case removing the check (and so
> > > producing a null pointer exception) seems fine.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > @Viresh: as I didn't choose to return -EINVAL I didn't add your Reviewed-by:
> > 
> > I'm confused by this thread, care to resend all of these in a series
> > against the latest linux-next tree?
> No problem, here it comes. This are patches 1 to 4 of the original
> series + v2 of patch 5 (that didn't have 5/5 in the subject).


Thanks, I'll queue these up soon.

greg k-h

Re: [PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Michał Mirosław]

2011/4/20 Greg KH <greg@kroah.com>:
> On Mon, Apr 11, 2011 at 08:42:58PM +0200, Uwe Kleine-König wrote:
>> Before commit
>>
>>       b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
>>
>> calling dev_set_drvdata with dev=NULL was an unchecked error. After some
>> discussion about what to return in this case removing the check (and so
>> producing a null pointer exception) seems fine.
> I'm confused by this thread, care to resend all of these in a series
> against the latest linux-next tree?

I'd argue that dev_set_drvdata() should never fail. All current
drivers depend on this, and if dev_set_drvdata() fails, user will get
an OOPS a short while after the device finishes initializing (or maybe
even before that if callbacks are involved).
Allowing dev_set_drvdata() to fail will need putting a lot of
boilerplate code into drivers for no real gain.

Please consider reverting commit
b4028437876866aba4747a655ede00f892089e14 instead of "fixing" issues it
generates.

Best Regards,
Michał Mirosław

Re: [PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Greg KH]

On Wed, Apr 20, 2011 at 11:09:56AM +0200, Michał Mirosław wrote:
> 2011/4/20 Greg KH <greg@kroah.com>:
> > On Mon, Apr 11, 2011 at 08:42:58PM +0200, Uwe Kleine-König wrote:
> >> Before commit
> >>
> >>       b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
> >>
> >> calling dev_set_drvdata with dev=NULL was an unchecked error. After some
> >> discussion about what to return in this case removing the check (and so
> >> producing a null pointer exception) seems fine.
> > I'm confused by this thread, care to resend all of these in a series
> > against the latest linux-next tree?
> 
> I'd argue that dev_set_drvdata() should never fail. All current
> drivers depend on this, and if dev_set_drvdata() fails, user will get
> an OOPS a short while after the device finishes initializing (or maybe
> even before that if callbacks are involved).
> Allowing dev_set_drvdata() to fail will need putting a lot of
> boilerplate code into drivers for no real gain.
> 
> Please consider reverting commit
> b4028437876866aba4747a655ede00f892089e14 instead of "fixing" issues it
> generates.

That patch was from 2009, surely if there were real issues with that
change, it would have shown up in the past 2 years, right?

And no, I don't want to revert that, we need that for future work in
this area.

I have no problem migrating the error code for that function on down,
very few drivers call this function directly, it should be wrapped by
bus-specific functions instead, right?  They can handle the error
handling on their own and not force the individual drivers to handle it
if needed.

Have you ever seen this function fail?

thanks,

greg k-h

Re: [PATCH v2] driver core: let dev_set_drvdata return int instead of void as it can fail

[Michał Mirosław]

2011/4/20 Greg KH <gregkh@suse.de>:
> On Wed, Apr 20, 2011 at 11:09:56AM +0200, Michał Mirosław wrote:
>> 2011/4/20 Greg KH <greg@kroah.com>:
>> > On Mon, Apr 11, 2011 at 08:42:58PM +0200, Uwe Kleine-König wrote:
>> >> Before commit
>> >>
>> >>       b402843 (Driver core: move dev_get/set_drvdata to drivers/base/dd.c)
>> >>
>> >> calling dev_set_drvdata with dev=NULL was an unchecked error. After some
>> >> discussion about what to return in this case removing the check (and so
>> >> producing a null pointer exception) seems fine.
>> > I'm confused by this thread, care to resend all of these in a series
>> > against the latest linux-next tree?
>>
>> I'd argue that dev_set_drvdata() should never fail. All current
>> drivers depend on this, and if dev_set_drvdata() fails, user will get
>> an OOPS a short while after the device finishes initializing (or maybe
>> even before that if callbacks are involved).
>> Allowing dev_set_drvdata() to fail will need putting a lot of
>> boilerplate code into drivers for no real gain.
>>
>> Please consider reverting commit
>> b4028437876866aba4747a655ede00f892089e14 instead of "fixing" issues it
>> generates.
>
> That patch was from 2009, surely if there were real issues with that
> change, it would have shown up in the past 2 years, right?
>
> And no, I don't want to revert that, we need that for future work in
> this area.
>
> I have no problem migrating the error code for that function on down,
> very few drivers call this function directly, it should be wrapped by
> bus-specific functions instead, right?  They can handle the error
> handling on their own and not force the individual drivers to handle it
> if needed.

> Have you ever seen this function fail?

When the allocation in device_private_init() fails, dev_set_drvdata()
leaves driver_data pointer not set.
But it looks like dev_set_drvdata() should not be called before
device_register(), so this check and allocation call there is
redundant.

So maybe the function should just look like this:

void dev_set_drvdata(struct device *dev, void *data)
{
  /* dev == NULL is a BUG; dev->p is allocated at device_register() time */
  BUG_ON(!dev->p);
  dev->p->driver_data = data;
}

Passing dev == NULL to dev_get_drvdata() is also a BUG, so:

void *dev_get_drvdata(const struct device *dev)
{
  return dev->p->driver_data;
}

Best Regards,
Michał Mirosław

Re: [PATCH 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[viresh kumar]

On 04/06/2011 02:54 PM, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I wasn't sure what to return when dev_set_drvdata is called with
> dev=NULL.  I choosed 0, but -EINVAL would be OK for me, too. What do you
> think?

Uwe,

I feel -EINVAL would be more appropriate here.
Return value of 0 would mean drvdata is set properly, which is not the case.

Otherwise, patch series looks fine to me. Please add my reviewed-by for all patches.

Reviewed-by: Viresh Kumar <viresh.kumar@st.com>

-- 
viresh

Re: [PATCH 5/5] driver core: let dev_set_drvdata return int instead of void as it can fail

[Michał Mirosław]

2011/4/6 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> I wasn't sure what to return when dev_set_drvdata is called with
> dev=NULL.  I choosed 0, but -EINVAL would be OK for me, too. What do you
> think?

This code was introduced by:

commit b4028437876866aba4747a655ede00f892089e14
Author: Greg Kroah-Hartman <gregkh@suse.de>
Date:   Mon May 11 14:16:57 2009 -0700

    Driver core: move dev_get/set_drvdata to drivers/base/dd.c

Before this patch, driver writers could assume that dev_set_drvdata()
never fails. And, dev==NULL would cause an imediate NULL dereference
(equivalent to BUG_ON(!dev), BTW). And, if dev_set_drvdata() fails
(silently as it is now) it's going to BUG later anyway.

I think it's best to revert that commit instead of fixing this up.

Best Regards,
Michał Mirosław