removing set_clientdata(NULL)
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:
Quoted 3 lines
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
Signature · 2 lines
Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ |
A message that is not in the archive (sent off-list or not yet received): [email protected]
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...
Quoted 9 lines · Wolfram Sang
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 akfree(). 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 toset_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.
Quoted 9 lines
Regarding this comment:
Quoted 3 lines
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_clientdataand 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.
Signature · 1 line
Jean Delvare
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Jean,
Quoted 2 lines
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 :)
Quoted 7 lines
Yes, my intent was to add a call to
i2c_set_clientdata(x, NULL)ini2c_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.
Quoted 4 lines
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?
Quoted 6 lines
Quoted 3 lines
I could check if there is any probe-function calling
get_clientdataand 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.
Signature · 2 lines
Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ |
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 18 lines · Wolfram Sang
Hi Jean,
Quoted 2 lines
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 :)
Quoted 7 lines
Yes, my intent was to add a call to
i2c_set_clientdata(x, NULL)ini2c_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.
Quoted 8 lines
Quoted 4 lines
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?
Quoted 1 line
- also for 2.6.34?
No, 2.6.35. This touches too many drivers for 2.6.34 at this point in time.
Quoted 1 line
- also patches per subsystem?
Not sure if it's worth the effort.
Quoted 1 line
- 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.
Signature · 1 line
Jean Delvare
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 6 lines · Jean Delvare
Quoted 1 line · Wolfram Sang
- 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)?
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 11 lines · Mark Brown
Quoted 6 lines · Jean Delvare
Quoted 1 line · Wolfram Sang
- 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.)
Signature · 1 line
Jean Delvare
Quoted 24 lines · Jean Delvare
Quoted 23 lines · Wolfram Sang
Hi Jean,
Quoted 2 lines
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 :)
Quoted 7 lines
Yes, my intent was to add a call to
i2c_set_clientdata(x, NULL)ini2c_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.
Quoted 4 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 4 lines
Quoted 1 line
- 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.
Quoted 3 lines
Quoted 1 line
- 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).
Quoted 4 lines
Quoted 1 line
- 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
Signature · 2 lines
Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ |
A message that is not in the archive (sent off-list or not yet received): [email protected]
Hi Wolfram,
Quoted 5 lines · Wolfram Sang
Quoted 1 line · Jean Delvare
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).
Quoted 7 lines
Quoted 2 lines
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_clientdatapopping up until the merge-window, it will be handled by an incremental patch.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
- 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.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
- 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,
Signature · 1 line
Jean Delvare