[PATCH] drivers:input:set driver data to NULL for pcap_keys

[PATCH] drivers:input:set driver data to NULL for pcap_keys

[Wanlong Gao]

It's better to set the device's driver data to NULL
when remove it.

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 drivers/input/misc/pcap_keys.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/input/misc/pcap_keys.c b/drivers/input/misc/pcap_keys.c
index 99335c2..6c670f5 100644
--- a/drivers/input/misc/pcap_keys.c
+++ b/drivers/input/misc/pcap_keys.c
@@ -114,6 +114,8 @@ static int __devexit pcap_keys_remove(struct platform_device *pdev)
 	input_unregister_device(pcap_keys->input);
 	kfree(pcap_keys);
 
+	platform_set_drvdata(pdev, NULL);
+
 	return 0;
 }
 
-- 
1.7.4.1

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Dmitry Torokhov]

On Wed, Jul 20, 2011 at 11:34:28PM +0800, Wanlong Gao wrote:
> It's better to set the device's driver data to NULL
> when remove it.
> 

I'd rather have platform devices core clean up this pointer, then we
could stop caring about it in all drivers...

Thanks.

-- 
Dmitry

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Wanlong Gao]

On 07/25/2011 04:30 PM, Dmitry Torokhov wrote:
> On Wed, Jul 20, 2011 at 11:34:28PM +0800, Wanlong Gao wrote:
>> It's better to set the device's driver data to NULL
>> when remove it.
>>
>
> I'd rather have platform devices core clean up this pointer, then we
> could stop caring about it in all drivers...
But the platform devices core just call the method of each own.
And don't care about the details like pdata, etc.

Meanwhile, I think the platform core need not care about these details.

and Greg, what do you think about this?

-- 
Thanks
Best Regards
Wanlong Gao

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Greg KH]

On Mon, Jul 25, 2011 at 05:14:11PM +0800, Wanlong Gao wrote:
> On 07/25/2011 04:30 PM, Dmitry Torokhov wrote:
> >On Wed, Jul 20, 2011 at 11:34:28PM +0800, Wanlong Gao wrote:
> >>It's better to set the device's driver data to NULL
> >>when remove it.
> >>
> >
> >I'd rather have platform devices core clean up this pointer, then we
> >could stop caring about it in all drivers...
> But the platform devices core just call the method of each own.
> And don't care about the details like pdata, etc.
> 
> Meanwhile, I think the platform core need not care about these details.
> 
> and Greg, what do you think about this?

I don't understand what you are asking me.

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Wanlong Gao]

On Mon, 2011-07-25 at 08:21 -0700, Greg KH wrote:
> On Mon, Jul 25, 2011 at 05:14:11PM +0800, Wanlong Gao wrote:
> > On 07/25/2011 04:30 PM, Dmitry Torokhov wrote:
> > >On Wed, Jul 20, 2011 at 11:34:28PM +0800, Wanlong Gao wrote:
> > >>It's better to set the device's driver data to NULL
> > >>when remove it.
> > >>
> > >
> > >I'd rather have platform devices core clean up this pointer, then we
> > >could stop caring about it in all drivers...
> > But the platform devices core just call the method of each own.
> > And don't care about the details like pdata, etc.
> > 
> > Meanwhile, I think the platform core need not care about these details.
> > 
> > and Greg, what do you think about this?
> 
> I don't understand what you are asking me.
Sorry, that's below:
firt for this patch:

> It's better to set the device's driver data to NULL
> when remove it.
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  drivers/input/misc/pcap_keys.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/misc/pcap_keys.c
> b/drivers/input/misc/pcap_keys.c
> index 99335c2..6c670f5 100644
> --- a/drivers/input/misc/pcap_keys.c
> +++ b/drivers/input/misc/pcap_keys.c
> @@ -114,6 +114,8 @@ static int __devexit pcap_keys_remove(struct
> platform_device *pdev)
>         input_unregister_device(pcap_keys->input);
>         kfree(pcap_keys);
>  
> +       platform_set_drvdata(pdev, NULL);
> +
>         return 0;
>  }
> 
then, Dmitry said that:

> I'd rather have platform devices core clean up this pointer, then we
> could stop caring about it in all drivers...
> 

then I said:

> But the platform devices core just call the method of each own.
> And don't care about the details like pdata, etc.
> 
> Meanwhile, I think the platform core need not care about these
> details.
> 
> and Greg, what do you think about this?
> 
this is the thread, Greg, understand now?

For short, it's all about the platform driver data. Since many drivers
set the platform driver data to NULL when it is removed, then Dmitry
think it should be done in the platform driver core instead.
So, I asked you for this.

It's clear now?

Thanks a lot

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Dmitry Torokhov]

On Mon, Jul 25, 2011 at 11:34:29PM +0800, Wanlong Gao wrote:
> > > On 07/25/2011 04:30 PM, Dmitry Torokhov wrote:
> 
> > I'd rather have platform devices core clean up this pointer, then we
> > could stop caring about it in all drivers...
> > 
> 
> then I said:
> 
> > But the platform devices core just call the method of each own.
> > And don't care about the details like pdata, etc.
> > 
> > Meanwhile, I think the platform core need not care about these
> > details.
> > 
> > and Greg, what do you think about this?
> > 
> this is the thread, Greg, understand now?
> 
> For short, it's all about the platform driver data. Since many drivers
> set the platform driver data to NULL when it is removed, then Dmitry
> think it should be done in the platform driver core instead.

Right, like i2c bus we could just have platform core clean up platform
drvdata pointer after calling ->remove() and also if ->probe() errors
out.

Then individual drivers do not have to care about cleaning up this
pointer.

Thanks.

-- 
Dmitry

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Mark Brown]

On Mon, Jul 25, 2011 at 11:19:16AM -0700, Dmitry Torokhov wrote:

> Right, like i2c bus we could just have platform core clean up platform
> drvdata pointer after calling ->remove() and also if ->probe() errors
> out.

I2C doesn't do this (at least not any more).

> Then individual drivers do not have to care about cleaning up this
> pointer.

They don't have to worry about it anyway, the only thing that is allowed
to use the pointer is a currently bound driver and it's only allowed to
rely on is that while it's bound it'll get back the same driver_data
that it put in.

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Dmitry Torokhov]

On Mon, Jul 25, 2011 at 07:29:28PM +0100, Mark Brown wrote:
> On Mon, Jul 25, 2011 at 11:19:16AM -0700, Dmitry Torokhov wrote:
> 
> > Right, like i2c bus we could just have platform core clean up platform
> > drvdata pointer after calling ->remove() and also if ->probe() errors
> > out.
> 
> I2C doesn't do this (at least not any more).
> 

Sure does. See drivers/i2c/i2c-core.c::i2c_device_probe() and
i2c_device_remove().

> > Then individual drivers do not have to care about cleaning up this
> > pointer.
> 
> They don't have to worry about it anyway, the only thing that is allowed
> to use the pointer is a currently bound driver and it's only allowed to
> rely on is that while it's bound it'll get back the same driver_data
> that it put in.

Right, except that some people trying to use this pointers to pass
platform data to the driver... Resetting the pointer to NULL on unbind
will hopefully show them their mistake.

-- 
Dmitry

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Wanlong Gao]

On 07/26/2011 02:37 AM, Dmitry Torokhov wrote:
> On Mon, Jul 25, 2011 at 07:29:28PM +0100, Mark Brown wrote:
>> On Mon, Jul 25, 2011 at 11:19:16AM -0700, Dmitry Torokhov wrote:
>>
>>> Right, like i2c bus we could just have platform core clean up platform
>>> drvdata pointer after calling ->remove() and also if ->probe() errors
>>> out.
>>
>> I2C doesn't do this (at least not any more).
>>
>
> Sure does. See drivers/i2c/i2c-core.c::i2c_device_probe() and
> i2c_device_remove().
Yeah, I see it. Sure i2c does.
Let the Core to do the pointer's clean up is very good idea.

So, Dmitry, do you means this ?

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
  drivers/base/platform.c |   30 ++++++++++++++++++++++++++++--
  1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 6040717..349e71b 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -405,8 +405,21 @@ static int platform_drv_probe(struct device *_dev)
  {
  	struct platform_driver *drv = to_platform_driver(_dev->driver);
  	struct platform_device *dev = to_platform_device(_dev);
+	int status;

-	return drv->probe(dev);
+	if (!dev)
+		return 0;
+
+	if (!drv->probe)
+		return -ENODEV;
+
+	dev_dbg(_dev, "probe\n");
+
+	status = drv->probe(dev);
+	if (status)
+		platform_set_drvdata(dev, NULL);
+
+	return status;
  }

  static int platform_drv_probe_fail(struct device *_dev)
@@ -418,8 +431,21 @@ static int platform_drv_remove(struct device *_dev)
  {
  	struct platform_driver *drv = to_platform_driver(_dev->driver);
  	struct platform_device *dev = to_platform_device(_dev);
+	int status;
+
+	if (!dev)
+		return 0;
+
+	if (drv->remove) {
+		dev_dbg(_dev, "remove\n");
+		status = drv->remove(dev);
+	} else {
+		status = 0;
+	}
+	if (status == 0)
+		platform_set_drvdata(dev, NULL);

-	return drv->remove(dev);
+	return status;
  }

  static void platform_drv_shutdown(struct device *_dev)
-- 
1.7.6



-- 
Thanks
Best Regards
Wanlong Gao

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Greg KH]

On Tue, Jul 26, 2011 at 09:29:59AM +0800, Wanlong Gao wrote:
> On 07/26/2011 02:37 AM, Dmitry Torokhov wrote:
> >On Mon, Jul 25, 2011 at 07:29:28PM +0100, Mark Brown wrote:
> >>On Mon, Jul 25, 2011 at 11:19:16AM -0700, Dmitry Torokhov wrote:
> >>
> >>>Right, like i2c bus we could just have platform core clean up platform
> >>>drvdata pointer after calling ->remove() and also if ->probe() errors
> >>>out.
> >>
> >>I2C doesn't do this (at least not any more).
> >>
> >
> >Sure does. See drivers/i2c/i2c-core.c::i2c_device_probe() and
> >i2c_device_remove().
> Yeah, I see it. Sure i2c does.
> Let the Core to do the pointer's clean up is very good idea.

No it isn't.

> So, Dmitry, do you means this ?
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  drivers/base/platform.c |   30 ++++++++++++++++++++++++++++--
>  1 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 6040717..349e71b 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -405,8 +405,21 @@ static int platform_drv_probe(struct device *_dev)
>  {
>  	struct platform_driver *drv = to_platform_driver(_dev->driver);
>  	struct platform_device *dev = to_platform_device(_dev);
> +	int status;
> 
> -	return drv->probe(dev);
> +	if (!dev)
> +		return 0;

When would dev ever be NULL?  Wouldn't that be an error?

> +
> +	if (!drv->probe)
> +		return -ENODEV;

That's not a valid error for this.  And when would probe ever be NULL?

> +
> +	dev_dbg(_dev, "probe\n");

Why add this noise?

> +
> +	status = drv->probe(dev);
> +	if (status)
> +		platform_set_drvdata(dev, NULL);

No, not ok.

> +
> +	return status;
>  }
> 
>  static int platform_drv_probe_fail(struct device *_dev)
> @@ -418,8 +431,21 @@ static int platform_drv_remove(struct device *_dev)
>  {
>  	struct platform_driver *drv = to_platform_driver(_dev->driver);
>  	struct platform_device *dev = to_platform_device(_dev);
> +	int status;
> +
> +	if (!dev)
> +		return 0;

Again, when would that ever happen?

> +
> +	if (drv->remove) {
> +		dev_dbg(_dev, "remove\n");
> +		status = drv->remove(dev);
> +	} else {
> +		status = 0;
> +	}

Again, why would remove ever be NULL?

This whole thing isn't needed at all.

greg k-h

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Wanlong Gao]

On 07/26/2011 12:39 PM, Greg KH wrote:

>> +
>> +	if (drv->remove) {
>> +		dev_dbg(_dev, "remove\n");
>> +		status = drv->remove(dev);
>> +	} else {
>> +		status = 0;
>> +	}
>
> Again, why would remove ever be NULL?
>
> This whole thing isn't needed at all.
>
> greg k-h
Yeah, I see.

But Greg, why does i2c-core do this?
like:drivers/i2c/i2c-core.c:
static int i2c_device_remove(struct device *dev)
{
	struct i2c_client	*client = i2c_verify_client(dev);
	struct i2c_driver	*driver;
	int			status;

	if (!client || !dev->driver)
		return 0;

	driver = to_i2c_driver(dev->driver);
	if (driver->remove) {
		dev_dbg(dev, "remove\n");
		status = driver->remove(client);
	} else {
		dev->driver = NULL;
		status = 0;
	}
	if (status == 0) {
		client->driver = NULL;
		i2c_set_clientdata(client, NULL);
	}
	return status;
}

And now, I'm in a fog, can you clear me/us ?

-- 
Thanks
Best Regards
Wanlong Gao

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Greg KH]

On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote:
> On 07/26/2011 12:39 PM, Greg KH wrote:
> 
> >>+
> >>+	if (drv->remove) {
> >>+		dev_dbg(_dev, "remove\n");
> >>+		status = drv->remove(dev);
> >>+	} else {
> >>+		status = 0;
> >>+	}
> >
> >Again, why would remove ever be NULL?
> >
> >This whole thing isn't needed at all.
> >
> >greg k-h
> Yeah, I see.
> 
> But Greg, why does i2c-core do this?
> like:drivers/i2c/i2c-core.c:

The i2c core has different requirements than the driver core does,
right?  They are two totally different things, please don't assume that
the rules for one are the same for the other.

greg k-h

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Wanlong Gao]

On 07/26/2011 02:02 PM, Greg KH wrote:
> On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote:
>> On 07/26/2011 12:39 PM, Greg KH wrote:
>>
>>>> +
>>>> +	if (drv->remove) {
>>>> +		dev_dbg(_dev, "remove\n");
>>>> +		status = drv->remove(dev);
>>>> +	} else {
>>>> +		status = 0;
>>>> +	}
>>>
>>> Again, why would remove ever be NULL?
>>>
>>> This whole thing isn't needed at all.
>>>
>>> greg k-h
>> Yeah, I see.
>>
>> But Greg, why does i2c-core do this?
>> like:drivers/i2c/i2c-core.c:
>
> The i2c core has different requirements than the driver core does,
> right?  They are two totally different things, please don't assume that
> the rules for one are the same for the other.
>
> greg k-h
>
Hmm...They are totally different things, maybe I see..


-- 
Thanks
Best Regards
Wanlong Gao

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Dmitry Torokhov]

On Tue, Jul 26, 2011 at 02:25:48PM +0800, Wanlong Gao wrote:
> On 07/26/2011 02:02 PM, Greg KH wrote:
> >On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote:
> >>On 07/26/2011 12:39 PM, Greg KH wrote:
> >>
> >>>>+
> >>>>+	if (drv->remove) {
> >>>>+		dev_dbg(_dev, "remove\n");
> >>>>+		status = drv->remove(dev);
> >>>>+	} else {
> >>>>+		status = 0;
> >>>>+	}
> >>>
> >>>Again, why would remove ever be NULL?
> >>>
> >>>This whole thing isn't needed at all.
> >>>
> >>>greg k-h
> >>Yeah, I see.
> >>
> >>But Greg, why does i2c-core do this?
> >>like:drivers/i2c/i2c-core.c:
> >
> >The i2c core has different requirements than the driver core does,
> >right?  They are two totally different things, please don't assume that
> >the rules for one are the same for the other.
> >
> >greg k-h
> >
> Hmm...They are totally different things, maybe I see..

Still, it would make sense to clean up platform device's drvdata
pointer, so that every platform driver out there does not have to do it
on its own.

-- 
Dmitry

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Greg KH]

On Mon, Jul 25, 2011 at 11:45:20PM -0700, Dmitry Torokhov wrote:
> On Tue, Jul 26, 2011 at 02:25:48PM +0800, Wanlong Gao wrote:
> > On 07/26/2011 02:02 PM, Greg KH wrote:
> > >On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote:
> > >>On 07/26/2011 12:39 PM, Greg KH wrote:
> > >>
> > >>>>+
> > >>>>+	if (drv->remove) {
> > >>>>+		dev_dbg(_dev, "remove\n");
> > >>>>+		status = drv->remove(dev);
> > >>>>+	} else {
> > >>>>+		status = 0;
> > >>>>+	}
> > >>>
> > >>>Again, why would remove ever be NULL?
> > >>>
> > >>>This whole thing isn't needed at all.
> > >>>
> > >>>greg k-h
> > >>Yeah, I see.
> > >>
> > >>But Greg, why does i2c-core do this?
> > >>like:drivers/i2c/i2c-core.c:
> > >
> > >The i2c core has different requirements than the driver core does,
> > >right?  They are two totally different things, please don't assume that
> > >the rules for one are the same for the other.
> > >
> > >greg k-h
> > >
> > Hmm...They are totally different things, maybe I see..
> 
> Still, it would make sense to clean up platform device's drvdata
> pointer, so that every platform driver out there does not have to do it
> on its own.

Again, it shouldn't need to be "cleaned" up, as no one relies on it
being there.

greg k-h

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Dmitry Torokhov]

On Tue, Jul 26, 2011 at 09:41:06AM -0700, Greg KH wrote:
> On Mon, Jul 25, 2011 at 11:45:20PM -0700, Dmitry Torokhov wrote:
> > On Tue, Jul 26, 2011 at 02:25:48PM +0800, Wanlong Gao wrote:
> > > On 07/26/2011 02:02 PM, Greg KH wrote:
> > > >On Tue, Jul 26, 2011 at 01:36:00PM +0800, Wanlong Gao wrote:
> > > >>On 07/26/2011 12:39 PM, Greg KH wrote:
> > > >>
> > > >>>>+
> > > >>>>+	if (drv->remove) {
> > > >>>>+		dev_dbg(_dev, "remove\n");
> > > >>>>+		status = drv->remove(dev);
> > > >>>>+	} else {
> > > >>>>+		status = 0;
> > > >>>>+	}
> > > >>>
> > > >>>Again, why would remove ever be NULL?
> > > >>>
> > > >>>This whole thing isn't needed at all.
> > > >>>
> > > >>>greg k-h
> > > >>Yeah, I see.
> > > >>
> > > >>But Greg, why does i2c-core do this?
> > > >>like:drivers/i2c/i2c-core.c:
> > > >
> > > >The i2c core has different requirements than the driver core does,
> > > >right?  They are two totally different things, please don't assume that
> > > >the rules for one are the same for the other.
> > > >
> > > >greg k-h
> > > >
> > > Hmm...They are totally different things, maybe I see..
> > 
> > Still, it would make sense to clean up platform device's drvdata
> > pointer, so that every platform driver out there does not have to do it
> > on its own.
> 
> Again, it shouldn't need to be "cleaned" up, as no one relies on it
> being there.

No one should rely on it being there however I came across quite a few
MFD patches that tried passing parent's data in that pointer. Hopefully
they are all cleaned now but we getting new drivers all the time...

If we had it in the platform bus code such uses would break even before
we get such drivers and we won't have to deal with them.

Thanks.

-- 
Dmitry

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Mark Brown]

On Mon, Jul 25, 2011 at 08:21:06AM -0700, Greg KH wrote:
> On Mon, Jul 25, 2011 at 05:14:11PM +0800, Wanlong Gao wrote:
> > On 07/25/2011 04:30 PM, Dmitry Torokhov wrote:

> > >I'd rather have platform devices core clean up this pointer, then we
> > >could stop caring about it in all drivers...

> > But the platform devices core just call the method of each own.
> > And don't care about the details like pdata, etc.

> > Meanwhile, I think the platform core need not care about these details.

> > and Greg, what do you think about this?

> I don't understand what you are asking me.

They're asking if drivers should set driver_data to NULL while
unbinding, which always struck me as a waste of time given that
nothing except a currently bound driver should be using driver_data.

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Greg KH]

On Mon, Jul 25, 2011 at 07:26:28PM +0100, Mark Brown wrote:
> On Mon, Jul 25, 2011 at 08:21:06AM -0700, Greg KH wrote:
> > On Mon, Jul 25, 2011 at 05:14:11PM +0800, Wanlong Gao wrote:
> > > On 07/25/2011 04:30 PM, Dmitry Torokhov wrote:
> 
> > > >I'd rather have platform devices core clean up this pointer, then we
> > > >could stop caring about it in all drivers...
> 
> > > But the platform devices core just call the method of each own.
> > > And don't care about the details like pdata, etc.
> 
> > > Meanwhile, I think the platform core need not care about these details.
> 
> > > and Greg, what do you think about this?
> 
> > I don't understand what you are asking me.
> 
> They're asking if drivers should set driver_data to NULL while
> unbinding, which always struck me as a waste of time given that
> nothing except a currently bound driver should be using driver_data.

Yeah, it's not needed, as nothing should rely on that.  However it's
also not hurting anything either.

greg k-h

Re: [PATCH] drivers:input:set driver data to NULL for pcap_keys

[Mark Brown]

On Mon, Jul 25, 2011 at 04:25:33PM -0700, Greg KH wrote:
> On Mon, Jul 25, 2011 at 07:26:28PM +0100, Mark Brown wrote:

> > They're asking if drivers should set driver_data to NULL while
> > unbinding, which always struck me as a waste of time given that
> > nothing except a currently bound driver should be using driver_data.

> Yeah, it's not needed, as nothing should rely on that.  However it's
> also not hurting anything either.

Every time I see it it always sets off a red flag, partly because we
shouldn't need to bother at all and partly because to the extent it's
valuable it's not something we should be doing on a driver by driver or
even subsystem by subsystem basis.