2011-03-01 08:00:04

by Uwe Kleine-König

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

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

2011-03-01 08:28:22

by Viresh Kumar

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

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

2011-03-01 09:05:29

by Uwe Kleine-König

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

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

2011-03-01 09:13:29

by Viresh Kumar

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

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

2011-03-01 15:34:13

by Greg KH

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

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

2011-04-06 09:24:38

by Uwe Kleine-König

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

Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-06 09:24:45

by Uwe Kleine-König

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

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

2011-04-06 09:24:53

by Uwe Kleine-König

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

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

2011-04-06 09:25:13

by Uwe Kleine-König

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

Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-06 09:24:37

by Uwe Kleine-König

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

Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-06 09:37:09

by Michał Mirosław

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

2011/4/6 Uwe Kleine-König <[email protected]>:
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> 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

2011-04-06 10:11:16

by Viresh Kumar

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

On 04/06/2011 02:54 PM, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> 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 <[email protected]>

--
viresh

2011-04-06 11:06:30

by Uwe Kleine-König

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

Hello,

On Wed, Apr 06, 2011 at 11:36:46AM +0200, Michał Mirosław wrote:
> 2011/4/6 Uwe Kleine-König <[email protected]>:
> > Signed-off-by: Uwe Kleine-König <[email protected]>
> > ---
> > 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/ |

2011-04-06 11:25:26

by Michał Mirosław

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

2011/4/6 Uwe Kleine-K?nig <[email protected]>:
> Hello,
> On Wed, Apr 06, 2011 at 11:36:46AM +0200, Micha? Miros?aw wrote:
>> 2011/4/6 Uwe Kleine-K?nig <[email protected]>:
>> > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
>> > ---
>> > 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

2011-04-06 11:41:23

by Michał Mirosław

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

2011/4/6 Uwe Kleine-K?nig <[email protected]>:
> Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> ---
> 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 <[email protected]>
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

2011-04-11 18:43:11

by Uwe Kleine-König

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

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

2011-04-11 18:50:40

by Uwe Kleine-König

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

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

2011-04-11 19:09:49

by Greg KH

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

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

2011-04-20 00:01:30

by Greg KH

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

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

2011-04-20 07:42:59

by Uwe Kleine-König

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

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

2011-04-20 07:44:57

by Uwe Kleine-König

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

Reviewed-by: Viresh Kumar <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-20 07:45:07

by Uwe Kleine-König

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

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

Reviewed-by: Viresh Kumar <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-20 07:44:56

by Uwe Kleine-König

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

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 <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-20 07:45:37

by Uwe Kleine-König

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

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

2011-04-20 07:45:35

by Uwe Kleine-König

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

Reviewed-by: Viresh Kumar <[email protected]>
Signed-off-by: Uwe Kleine-König <[email protected]>
---
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

2011-04-20 09:10:19

by Michał Mirosław

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

2011/4/20 Greg KH <[email protected]>:
> 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

2011-04-20 17:20:41

by Greg KH

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

On Wed, Apr 20, 2011 at 11:09:56AM +0200, Michał Mirosław wrote:
> 2011/4/20 Greg KH <[email protected]>:
> > 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

2011-04-20 17:20:53

by Greg KH

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

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

2011-04-20 17:59:52

by Michał Mirosław

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

2011/4/20 Greg KH <[email protected]>:
> On Wed, Apr 20, 2011 at 11:09:56AM +0200, Michał Mirosław wrote:
>> 2011/4/20 Greg KH <[email protected]>:
>> > 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

2011-04-29 08:13:14

by Russell King - ARM Linux

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

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

2011-04-29 09:16:26

by Uwe Kleine-König

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

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