removing set_clientdata(NULL)

removing set_clientdata(NULL)

[Wolfram Sang]

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

Hi Jean,

do you have already something in mind how to proceed with the
i2c_setclientdata-topic? I could offer the following:

I modify my semantic patch to remove all set_clientdata(NULL) calls connected
to a kfree(). Then, we could have a look if there are still some left and
investigate why. Once all issues are resolved (maybe there are none), we should
know what to add in the core-layer, hopefully just the call to set_clientdata.

Regarding this comment:

> the core should just set the client data to NULL. If there are drivers that
> rely on the current behavior, then those drivers should be reviewed first as
> to the reason why they need it.

I could check if there is any probe-function calling get_clientdata and making
use of that? That is probably the most obvious thing which would need to rely
on the current behaviour or did I miss something?

Have a nice weekend,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: removing set_clientdata(NULL)

[Jean Delvare]

Hi Wolfram,

Sorry for being a little quiet, I took a long, well-deserved week-end
off and didn't have the time to reply before I left. Now I'm back...

On Sat, 27 Mar 2010 13:15:58 +0100, Wolfram Sang wrote:
> Hi Jean,
> 
> do you have already something in mind how to proceed with the
> i2c_setclientdata-topic? I could offer the following:
> 
> I modify my semantic patch to remove all set_clientdata(NULL) calls connected
> to a kfree(). Then, we could have a look if there are still some left and
> investigate why. Once all issues are resolved (maybe there are none), we should
> know what to add in the core-layer, hopefully just the call to set_clientdata.

Yes, my intent was to add a call to i2c_set_clientdata(x, NULL) in
i2c_device_remove(). We would do this immediately, so that drivers can
start removing the call on their end quickly (and new ones are not
added.)

Can you please send the patch for i2c-core and Documentation/i2c? It's
not difficult but I don't want to steal your credits.

It would also be fair to warn all the developers you already contacted
with your first attempt and let them know that it is being cancelled,
and let them know the new plan. Hopefully this will avoid useless
commits.

> Regarding this comment:
> 
> > the core should just set the client data to NULL. If there are drivers that
> > rely on the current behavior, then those drivers should be reviewed first as
> > to the reason why they need it.
> 
> I could check if there is any probe-function calling get_clientdata and making
> use of that? That is probably the most obvious thing which would need to rely
> on the current behaviour or did I miss something?

You are right, but I really hope you won't catch anybody doing this.
This would go against the device driver model.

-- 
Jean Delvare

Re: removing set_clientdata(NULL)

[Wolfram Sang]

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

Hi Jean,

> Sorry for being a little quiet, I took a long, well-deserved week-end
> off and didn't have the time to reply before I left. Now I'm back...

No worries, no rush :)

> Yes, my intent was to add a call to i2c_set_clientdata(x, NULL) in
> i2c_device_remove(). We would do this immediately, so that drivers can
> start removing the call on their end quickly (and new ones are not
> added.)
> 
> Can you please send the patch for i2c-core and Documentation/i2c? It's
> not difficult but I don't want to steal your credits.

Yes, can do this in a few hours (already midnight here), this is for 2.6.34
then.

> It would also be fair to warn all the developers you already contacted
> with your first attempt and let them know that it is being cancelled,
> and let them know the new plan. Hopefully this will avoid useless
> commits.

Can do this, too. About the removal of the i2c_set_clientdata-calls:

- shall I prepare a series for that, too?
- also for 2.6.34?
- also patches per subsystem?
- shall this better go via the i2c-tree?

> > I could check if there is any probe-function calling get_clientdata and making
> > use of that? That is probably the most obvious thing which would need to rely
> > on the current behaviour or did I miss something?
> 
> You are right, but I really hope you won't catch anybody doing this.
> This would go against the device driver model.

I'd also be surprised if that would find something.

Regards,

   Wolfram

PS: I will also have a look at the pca-issue after that.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: removing set_clientdata(NULL)

[Jean Delvare]

On Mon, 29 Mar 2010 17:09:56 +0200, Wolfram Sang wrote:
> Hi Jean,
> 
> > Sorry for being a little quiet, I took a long, well-deserved week-end
> > off and didn't have the time to reply before I left. Now I'm back...
> 
> No worries, no rush :)
> 
> > Yes, my intent was to add a call to i2c_set_clientdata(x, NULL) in
> > i2c_device_remove(). We would do this immediately, so that drivers can
> > start removing the call on their end quickly (and new ones are not
> > added.)
> > 
> > Can you please send the patch for i2c-core and Documentation/i2c? It's
> > not difficult but I don't want to steal your credits.
> 
> Yes, can do this in a few hours (already midnight here), this is for 2.6.34
> then.
> 
> > It would also be fair to warn all the developers you already contacted
> > with your first attempt and let them know that it is being cancelled,
> > and let them know the new plan. Hopefully this will avoid useless
> > commits.
> 
> Can do this, too. About the removal of the i2c_set_clientdata-calls:
> 
> - shall I prepare a series for that, too?

Not sure what you mean there... What's the alternative?

> - also for 2.6.34?

No, 2.6.35. This touches too many drivers for 2.6.34 at this point in
time.

> - also patches per subsystem?

Not sure if it's worth the effort.

> - shall this better go via the i2c-tree?

This seems simpler, yes. I don't think subsystem or driver maintainers
need to be bothered with what is really only a cleanup.

-- 
Jean Delvare

Re: removing set_clientdata(NULL)

[Mark Brown]

On Mon, Mar 29, 2010 at 05:27:34PM +0200, Jean Delvare wrote:
> On Mon, 29 Mar 2010 17:09:56 +0200, Wolfram Sang wrote:

> > - shall this better go via the i2c-tree?

> This seems simpler, yes. I don't think subsystem or driver maintainers
> need to be bothered with what is really only a cleanup.

What about those subsystems where the maintainers applied the patch from
the first round (adding the explicit set to NULL where it had been
missing)?

Re: removing set_clientdata(NULL)

[Jean Delvare]

On Mon, 29 Mar 2010 16:49:10 +0100, Mark Brown wrote:
> On Mon, Mar 29, 2010 at 05:27:34PM +0200, Jean Delvare wrote:
> > On Mon, 29 Mar 2010 17:09:56 +0200, Wolfram Sang wrote:
> 
> > > - shall this better go via the i2c-tree?
> 
> > This seems simpler, yes. I don't think subsystem or driver maintainers
> > need to be bothered with what is really only a cleanup.
> 
> What about those subsystems where the maintainers applied the patch from
> the first round (adding the explicit set to NULL where it had been
> missing)?

I don't think we can ask them to revert, as they won't like it (except
for the few of working with quilt). So the best way is to let them push
the change upstream and amend it right after. Not ideal, but I can't
propose anything better (short of a time travelling machine, that is.)

-- 
Jean Delvare

Re: removing set_clientdata(NULL)

[Wolfram Sang]

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

On Mon, Mar 29, 2010 at 05:27:34PM +0200, Jean Delvare wrote:
> On Mon, 29 Mar 2010 17:09:56 +0200, Wolfram Sang wrote:
> > Hi Jean,
> > 
> > > Sorry for being a little quiet, I took a long, well-deserved week-end
> > > off and didn't have the time to reply before I left. Now I'm back...
> > 
> > No worries, no rush :)
> > 
> > > Yes, my intent was to add a call to i2c_set_clientdata(x, NULL) in
> > > i2c_device_remove(). We would do this immediately, so that drivers can
> > > start removing the call on their end quickly (and new ones are not
> > > added.)
> > > 
> > > Can you please send the patch for i2c-core and Documentation/i2c? It's
> > > not difficult but I don't want to steal your credits.
> > 
> > Yes, can do this in a few hours (already midnight here), this is for 2.6.34
> > then.
> > 
> > > It would also be fair to warn all the developers you already contacted
> > > with your first attempt and let them know that it is being cancelled,
> > > and let them know the new plan. Hopefully this will avoid useless
> > > commits.
> > 
> > Can do this, too. About the removal of the i2c_set_clientdata-calls:
> > 
> > - shall I prepare a series for that, too?
> 
> Not sure what you mean there... What's the alternative?

I was confused by your "so that drivers can start removing the call on their
end quickly". It sounded a bit like they should care themselves.

> > - also for 2.6.34?
> 
> No, 2.6.35. This touches too many drivers for 2.6.34 at this point in
> time.

I agree. So I base the removal-patch on rc3, so it can go to linux-next. If
there are other users of i2c_set_clientdata popping up until the merge-window,
it will be handled by an incremental patch.

> > - also patches per subsystem?
> 
> Not sure if it's worth the effort.

Not much effort, I made scripts for that :) Still, I think it should go in just
using one commit. Let's hope it won't get too intrusive (= creating conflicts).

> > - shall this better go via the i2c-tree?
> 
> This seems simpler, yes. I don't think subsystem or driver maintainers
> need to be bothered with what is really only a cleanup.

Yes.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: removing set_clientdata(NULL)

[Jean Delvare]

Hi Wolfram,

On Tue, 30 Mar 2010 04:48:31 +0200, Wolfram Sang wrote:
> On Mon, Mar 29, 2010 at 05:27:34PM +0200, Jean Delvare wrote:
> > Not sure what you mean there... What's the alternative?
> 
> I was confused by your "so that drivers can start removing the call on their
> end quickly". It sounded a bit like they should care themselves.

Ah. I mean that if they care, they can do it themselves. But most
probably they don't care and will leave it up to us (that is, you).

> > No, 2.6.35. This touches too many drivers for 2.6.34 at this point in
> > time.
> 
> I agree. So I base the removal-patch on rc3, so it can go to linux-next. If
> there are other users of i2c_set_clientdata popping up until the merge-window,
> it will be handled by an incremental patch.
> 
> > > - also patches per subsystem?
> > 
> > Not sure if it's worth the effort.
> 
> Not much effort, I made scripts for that :) Still, I think it should go in just
> using one commit. Let's hope it won't get too intrusive (= creating conflicts).

If it turns out to be an issue, I guess we could hold on and generate
the path again at the end of the 2.6.35 merge window.

> > > - shall this better go via the i2c-tree?
> > 
> > This seems simpler, yes. I don't think subsystem or driver maintainers
> > need to be bothered with what is really only a cleanup.
> 
> Yes.

Thanks,
-- 
Jean Delvare