| Message ID | 20220430081607.15078-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: Remove duplicated error reporting in .remove() | expand |
On Sat, 30 Apr 2022 10:15:58 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello, > > this series adapts several i2c drivers that emit two error messages if > something in their remove function fails. The relevant issue is that the > i2c core emits an error message if the remove callback returns a > non-zero value but the drivers already emit a (better) message. So > these patches change the drivers to return 0 even after an error. Note > there is no further error handling in the i2c core, if a remove callback > returns an error code, the device is removed anyhow, so the only effect > of making the return value zero is that the error message is suppressed. > > The motivation for this series is to eventually change the prototype of > the i2c remove callback to return void. As a preparation all remove > functions should return 0 such that changing the prototype doesn't > change behaviour of individual drivers. I think I'd rather have seen these called out as simply moving towards this second change as it feels wrong to deliberately not report an error so as to avoid repeated error messages! Meh, I don't care that strongly and you call out the real reason in each patch. > > Best regards > Uwe > > Uwe Kleine-König (9): > iio:accel:mc3230: Remove duplicated error reporting in .remove() > iio:accel:stk8312: Remove duplicated error reporting in .remove() > iio:accel:stk8ba50: Remove duplicated error reporting in .remove() > iio:light:bh1780: Remove duplicated error reporting in .remove() > iio:light:isl29028: Remove duplicated error reporting in .remove() > iio:light:jsa1212: Remove duplicated error reporting in .remove() > iio:light:opt3001: Remove duplicated error reporting in .remove() > iio:light:stk3310: Remove duplicated error reporting in .remove() > iio:light:tsl2583: Remove duplicated error reporting in .remove() > > drivers/iio/accel/mc3230.c | 4 +++- > drivers/iio/accel/stk8312.c | 4 +++- > drivers/iio/accel/stk8ba50.c | 4 +++- > drivers/iio/light/bh1780.c | 7 +++---- > drivers/iio/light/isl29028.c | 4 +++- > drivers/iio/light/jsa1212.c | 4 +++- > drivers/iio/light/opt3001.c | 3 +-- > drivers/iio/light/stk3310.c | 5 ++++- > drivers/iio/light/tsl2583.c | 4 +++- > 9 files changed, 26 insertions(+), 13 deletions(-) > > > base-commit: 3123109284176b1532874591f7c81f3837bbdc17
On Sun, 1 May 2022 18:41:49 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 30 Apr 2022 10:15:58 +0200 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > Hello, > > > > this series adapts several i2c drivers that emit two error messages if > > something in their remove function fails. The relevant issue is that the > > i2c core emits an error message if the remove callback returns a > > non-zero value but the drivers already emit a (better) message. So > > these patches change the drivers to return 0 even after an error. Note > > there is no further error handling in the i2c core, if a remove callback > > returns an error code, the device is removed anyhow, so the only effect > > of making the return value zero is that the error message is suppressed. > > > > The motivation for this series is to eventually change the prototype of > > the i2c remove callback to return void. As a preparation all remove > > functions should return 0 such that changing the prototype doesn't > > change behaviour of individual drivers. > > I think I'd rather have seen these called out as simply moving towards > this second change as it feels wrong to deliberately not report an error > so as to avoid repeated error messages! > > Meh, I don't care that strongly and you call out the real reason in each > patch. Series looks fine to me, but I'll leave the on list for a few days to let others have time to take a look. Worth noting that some of these are crying out for use of devm_add_action_or_reset() and getting rid of the remove functions entirely now you've dropped the oddity of them returning non 0. Low hanging fruit for any newbies who want to do it, or maybe I will if I get bored :) Thanks, Jonathan > > > > > Best regards > > Uwe > > > > Uwe Kleine-König (9): > > iio:accel:mc3230: Remove duplicated error reporting in .remove() > > iio:accel:stk8312: Remove duplicated error reporting in .remove() > > iio:accel:stk8ba50: Remove duplicated error reporting in .remove() > > iio:light:bh1780: Remove duplicated error reporting in .remove() > > iio:light:isl29028: Remove duplicated error reporting in .remove() > > iio:light:jsa1212: Remove duplicated error reporting in .remove() > > iio:light:opt3001: Remove duplicated error reporting in .remove() > > iio:light:stk3310: Remove duplicated error reporting in .remove() > > iio:light:tsl2583: Remove duplicated error reporting in .remove() > > > > drivers/iio/accel/mc3230.c | 4 +++- > > drivers/iio/accel/stk8312.c | 4 +++- > > drivers/iio/accel/stk8ba50.c | 4 +++- > > drivers/iio/light/bh1780.c | 7 +++---- > > drivers/iio/light/isl29028.c | 4 +++- > > drivers/iio/light/jsa1212.c | 4 +++- > > drivers/iio/light/opt3001.c | 3 +-- > > drivers/iio/light/stk3310.c | 5 ++++- > > drivers/iio/light/tsl2583.c | 4 +++- > > 9 files changed, 26 insertions(+), 13 deletions(-) > > > > > > base-commit: 3123109284176b1532874591f7c81f3837bbdc17 >
Hello Jonathan, On Sun, May 01, 2022 at 06:41:49PM +0100, Jonathan Cameron wrote: > On Sat, 30 Apr 2022 10:15:58 +0200 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > Hello, > > > > this series adapts several i2c drivers that emit two error messages if > > something in their remove function fails. The relevant issue is that the > > i2c core emits an error message if the remove callback returns a > > non-zero value but the drivers already emit a (better) message. So > > these patches change the drivers to return 0 even after an error. Note > > there is no further error handling in the i2c core, if a remove callback > > returns an error code, the device is removed anyhow, so the only effect > > of making the return value zero is that the error message is suppressed. > > > > The motivation for this series is to eventually change the prototype of > > the i2c remove callback to return void. As a preparation all remove > > functions should return 0 such that changing the prototype doesn't > > change behaviour of individual drivers. > > I think I'd rather have seen these called out as simply moving towards > this second change as it feels wrong to deliberately not report an error > so as to avoid repeated error messages! I admit this is a bit strange. Once the i2c remove callback is changed accordingly (and the platform remove callback, and the ac97_codec_driver remove callback ...) this goes away. Working on it. The reason it's that way is that for similar patches the maintainer feedback was to include the preparatory patch in the series that actually changes the remove callback. As I like to have as much of the preparatory patches already applied, I thought it a good idea to find a motivation to apply already today :-) Thanks for your cooperation, Uwe
On Sun, 1 May 2022 18:51:23 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sun, 1 May 2022 18:41:49 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sat, 30 Apr 2022 10:15:58 +0200 > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > > > Hello, > > > > > > this series adapts several i2c drivers that emit two error messages if > > > something in their remove function fails. The relevant issue is that the > > > i2c core emits an error message if the remove callback returns a > > > non-zero value but the drivers already emit a (better) message. So > > > these patches change the drivers to return 0 even after an error. Note > > > there is no further error handling in the i2c core, if a remove callback > > > returns an error code, the device is removed anyhow, so the only effect > > > of making the return value zero is that the error message is suppressed. > > > > > > The motivation for this series is to eventually change the prototype of > > > the i2c remove callback to return void. As a preparation all remove > > > functions should return 0 such that changing the prototype doesn't > > > change behaviour of individual drivers. > > > > I think I'd rather have seen these called out as simply moving towards > > this second change as it feels wrong to deliberately not report an error > > so as to avoid repeated error messages! > > > > Meh, I don't care that strongly and you call out the real reason in each > > patch. > > Series looks fine to me, but I'll leave the on list for a few days to let > others have time to take a look. > > Worth noting that some of these are crying out for use > of devm_add_action_or_reset() and getting rid of the remove functions > entirely now you've dropped the oddity of them returning non 0. > > Low hanging fruit for any newbies who want to do it, or maybe I will > if I get bored :) Series applied to the togreg branch of iio.git and pushed out as testing for 0-day to see if it can find anything we missed. Thanks, Jonathan > > Thanks, > > Jonathan > > > > > > > > > Best regards > > > Uwe > > > > > > Uwe Kleine-König (9): > > > iio:accel:mc3230: Remove duplicated error reporting in .remove() > > > iio:accel:stk8312: Remove duplicated error reporting in .remove() > > > iio:accel:stk8ba50: Remove duplicated error reporting in .remove() > > > iio:light:bh1780: Remove duplicated error reporting in .remove() > > > iio:light:isl29028: Remove duplicated error reporting in .remove() > > > iio:light:jsa1212: Remove duplicated error reporting in .remove() > > > iio:light:opt3001: Remove duplicated error reporting in .remove() > > > iio:light:stk3310: Remove duplicated error reporting in .remove() > > > iio:light:tsl2583: Remove duplicated error reporting in .remove() > > > > > > drivers/iio/accel/mc3230.c | 4 +++- > > > drivers/iio/accel/stk8312.c | 4 +++- > > > drivers/iio/accel/stk8ba50.c | 4 +++- > > > drivers/iio/light/bh1780.c | 7 +++---- > > > drivers/iio/light/isl29028.c | 4 +++- > > > drivers/iio/light/jsa1212.c | 4 +++- > > > drivers/iio/light/opt3001.c | 3 +-- > > > drivers/iio/light/stk3310.c | 5 ++++- > > > drivers/iio/light/tsl2583.c | 4 +++- > > > 9 files changed, 26 insertions(+), 13 deletions(-) > > > > > > > > > base-commit: 3123109284176b1532874591f7c81f3837bbdc17 > > >
Hello Jonathan, On Sat, May 07, 2022 at 03:38:55PM +0100, Jonathan Cameron wrote: > On Sun, 1 May 2022 18:51:23 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Sun, 1 May 2022 18:41:49 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > On Sat, 30 Apr 2022 10:15:58 +0200 > > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > Hello, > > > > > > > > this series adapts several i2c drivers that emit two error messages if > > > > something in their remove function fails. The relevant issue is that the > > > > i2c core emits an error message if the remove callback returns a > > > > non-zero value but the drivers already emit a (better) message. So > > > > these patches change the drivers to return 0 even after an error. Note > > > > there is no further error handling in the i2c core, if a remove callback > > > > returns an error code, the device is removed anyhow, so the only effect > > > > of making the return value zero is that the error message is suppressed. > > > > > > > > The motivation for this series is to eventually change the prototype of > > > > the i2c remove callback to return void. As a preparation all remove > > > > functions should return 0 such that changing the prototype doesn't > > > > change behaviour of individual drivers. > > > > > > I think I'd rather have seen these called out as simply moving towards > > > this second change as it feels wrong to deliberately not report an error > > > so as to avoid repeated error messages! > > > > > > Meh, I don't care that strongly and you call out the real reason in each > > > patch. > > > > Series looks fine to me, but I'll leave the on list for a few days to let > > others have time to take a look. > > > > Worth noting that some of these are crying out for use > > of devm_add_action_or_reset() and getting rid of the remove functions > > entirely now you've dropped the oddity of them returning non 0. > > > > Low hanging fruit for any newbies who want to do it, or maybe I will > > if I get bored :) > > Series applied to the togreg branch of iio.git and pushed out as testing for > 0-day to see if it can find anything we missed. They are in testing (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing) but not in togreg (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=togreg). Not sure if that is how it's supposed to be? Only togreg seems to be in next. Best regards Uwe
On Fri, 13 May 2022 09:24:24 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello Jonathan, > > On Sat, May 07, 2022 at 03:38:55PM +0100, Jonathan Cameron wrote: > > On Sun, 1 May 2022 18:51:23 +0100 > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > On Sun, 1 May 2022 18:41:49 +0100 > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > On Sat, 30 Apr 2022 10:15:58 +0200 > > > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > Hello, > > > > > > > > > > this series adapts several i2c drivers that emit two error messages if > > > > > something in their remove function fails. The relevant issue is that the > > > > > i2c core emits an error message if the remove callback returns a > > > > > non-zero value but the drivers already emit a (better) message. So > > > > > these patches change the drivers to return 0 even after an error. Note > > > > > there is no further error handling in the i2c core, if a remove callback > > > > > returns an error code, the device is removed anyhow, so the only effect > > > > > of making the return value zero is that the error message is suppressed. > > > > > > > > > > The motivation for this series is to eventually change the prototype of > > > > > the i2c remove callback to return void. As a preparation all remove > > > > > functions should return 0 such that changing the prototype doesn't > > > > > change behaviour of individual drivers. > > > > > > > > I think I'd rather have seen these called out as simply moving towards > > > > this second change as it feels wrong to deliberately not report an error > > > > so as to avoid repeated error messages! > > > > > > > > Meh, I don't care that strongly and you call out the real reason in each > > > > patch. > > > > > > Series looks fine to me, but I'll leave the on list for a few days to let > > > others have time to take a look. > > > > > > Worth noting that some of these are crying out for use > > > of devm_add_action_or_reset() and getting rid of the remove functions > > > entirely now you've dropped the oddity of them returning non 0. > > > > > > Low hanging fruit for any newbies who want to do it, or maybe I will > > > if I get bored :) > > > > Series applied to the togreg branch of iio.git and pushed out as testing for > > 0-day to see if it can find anything we missed. > > They are in testing > (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing) > but not in togreg > (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=togreg). > > Not sure if that is how it's supposed to be? Only togreg seems to be in > next. Yup. That's intentional because I don't rebase the togreg branch unless something goes wrong when it hits next. The intent being it's a stable base to build upon. Normally there is a delay of up to a week to let 0-day take a look at testing and for me to happen to get time sat at the right computer, but sometimes it's longer :( Right now I'm waiting on a pull request being picked up by Greg KH, after which I'll fast forward the togreg branch as I have some patches waiting to be queued up that are dependent on things that have reached char-misc-next via other paths. Unfortunately I'm doubtful about whether I can squeeze in a second pull request this cycle, so they may get queued up for next cycle. A bit of bad timing :( Jonathan > > Best regards > Uwe >
Hello Jonathan, On Sat, May 14, 2022 at 02:31:51PM +0100, Jonathan Cameron wrote: > On Fri, 13 May 2022 09:24:24 +0200 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Sat, May 07, 2022 at 03:38:55PM +0100, Jonathan Cameron wrote: > > > On Sun, 1 May 2022 18:51:23 +0100 > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > On Sun, 1 May 2022 18:41:49 +0100 > > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > > On Sat, 30 Apr 2022 10:15:58 +0200 > > > > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > > > Hello, > > > > > > > > > > > > this series adapts several i2c drivers that emit two error messages if > > > > > > something in their remove function fails. The relevant issue is that the > > > > > > i2c core emits an error message if the remove callback returns a > > > > > > non-zero value but the drivers already emit a (better) message. So > > > > > > these patches change the drivers to return 0 even after an error. Note > > > > > > there is no further error handling in the i2c core, if a remove callback > > > > > > returns an error code, the device is removed anyhow, so the only effect > > > > > > of making the return value zero is that the error message is suppressed. > > > > > > > > > > > > The motivation for this series is to eventually change the prototype of > > > > > > the i2c remove callback to return void. As a preparation all remove > > > > > > functions should return 0 such that changing the prototype doesn't > > > > > > change behaviour of individual drivers. > > > > > > > > > > I think I'd rather have seen these called out as simply moving towards > > > > > this second change as it feels wrong to deliberately not report an error > > > > > so as to avoid repeated error messages! > > > > > > > > > > Meh, I don't care that strongly and you call out the real reason in each > > > > > patch. > > > > > > > > Series looks fine to me, but I'll leave the on list for a few days to let > > > > others have time to take a look. > > > > > > > > Worth noting that some of these are crying out for use > > > > of devm_add_action_or_reset() and getting rid of the remove functions > > > > entirely now you've dropped the oddity of them returning non 0. > > > > > > > > Low hanging fruit for any newbies who want to do it, or maybe I will > > > > if I get bored :) > > > > > > Series applied to the togreg branch of iio.git and pushed out as testing for > > > 0-day to see if it can find anything we missed. > > > > They are in testing > > (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing) > > but not in togreg > > (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=togreg). > > > > Not sure if that is how it's supposed to be? Only togreg seems to be in > > next. > Yup. That's intentional because I don't rebase the togreg branch unless > something goes wrong when it hits next. The intent being it's a stable > base to build upon. Normally there is a delay of up to a week to let > 0-day take a look at testing and for me to happen to get time sat at > the right computer, but sometimes it's longer :( > > Right now I'm waiting on a pull request being picked up by Greg KH, > after which I'll fast forward the togreg branch as I have some patches > waiting to be queued up that are dependent on things that have reached > char-misc-next via other paths. Not sure I understood that correctly. Do you let Greg pull the togreg branch? If you instead let him pull tags, you don't have to wait in such a situation to apply new patches to a for-next branch. (Or just don't use "togreg" for both, sending PRs to Greg and put patches into next.) > Unfortunately I'm doubtful about whether I can squeeze in a second > pull request this cycle, so they may get queued up for next cycle. > A bit of bad timing :( It's not a big problem for me. There is still much to do (also a bit in drivers/iio) before I tackle the final bits of my quest and actually change struct i2c_driver (and so depend on these patches having hit mainline). The only downside for you is, that you might have to endure me asking again for the state of these patches ;-) Thanks for your feedback. Compared to pinging repeatedly and getting no maintainer reply, knowing about your problems is much appreciated. Best regards and have a nice week-end, Uwe
On Sat, 14 May 2022 15:41:59 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello Jonathan, > > On Sat, May 14, 2022 at 02:31:51PM +0100, Jonathan Cameron wrote: > > On Fri, 13 May 2022 09:24:24 +0200 > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > On Sat, May 07, 2022 at 03:38:55PM +0100, Jonathan Cameron wrote: > > > > On Sun, 1 May 2022 18:51:23 +0100 > > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > > On Sun, 1 May 2022 18:41:49 +0100 > > > > > Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > > > > > > On Sat, 30 Apr 2022 10:15:58 +0200 > > > > > > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > this series adapts several i2c drivers that emit two error messages if > > > > > > > something in their remove function fails. The relevant issue is that the > > > > > > > i2c core emits an error message if the remove callback returns a > > > > > > > non-zero value but the drivers already emit a (better) message. So > > > > > > > these patches change the drivers to return 0 even after an error. Note > > > > > > > there is no further error handling in the i2c core, if a remove callback > > > > > > > returns an error code, the device is removed anyhow, so the only effect > > > > > > > of making the return value zero is that the error message is suppressed. > > > > > > > > > > > > > > The motivation for this series is to eventually change the prototype of > > > > > > > the i2c remove callback to return void. As a preparation all remove > > > > > > > functions should return 0 such that changing the prototype doesn't > > > > > > > change behaviour of individual drivers. > > > > > > > > > > > > I think I'd rather have seen these called out as simply moving towards > > > > > > this second change as it feels wrong to deliberately not report an error > > > > > > so as to avoid repeated error messages! > > > > > > > > > > > > Meh, I don't care that strongly and you call out the real reason in each > > > > > > patch. > > > > > > > > > > Series looks fine to me, but I'll leave the on list for a few days to let > > > > > others have time to take a look. > > > > > > > > > > Worth noting that some of these are crying out for use > > > > > of devm_add_action_or_reset() and getting rid of the remove functions > > > > > entirely now you've dropped the oddity of them returning non 0. > > > > > > > > > > Low hanging fruit for any newbies who want to do it, or maybe I will > > > > > if I get bored :) > > > > > > > > Series applied to the togreg branch of iio.git and pushed out as testing for > > > > 0-day to see if it can find anything we missed. > > > > > > They are in testing > > > (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=testing) > > > but not in togreg > > > (https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=togreg). > > > > > > Not sure if that is how it's supposed to be? Only togreg seems to be in > > > next. > > Yup. That's intentional because I don't rebase the togreg branch unless > > something goes wrong when it hits next. The intent being it's a stable > > base to build upon. Normally there is a delay of up to a week to let > > 0-day take a look at testing and for me to happen to get time sat at > > the right computer, but sometimes it's longer :( > > > > Right now I'm waiting on a pull request being picked up by Greg KH, > > after which I'll fast forward the togreg branch as I have some patches > > waiting to be queued up that are dependent on things that have reached > > char-misc-next via other paths. > > Not sure I understood that correctly. Do you let Greg pull the togreg > branch? If you instead let him pull tags, you don't have to wait in such > a situation to apply new patches to a for-next branch. (Or just don't > use "togreg" for both, sending PRs to Greg and put patches into next.) The pull request is indeed for a tag. I could expose what is currently only out as testing to next, but that means pushing it out as togreg. As a general rule (the vast majority of the time) I do not rebase togreg once I've pushed that out to kernel.org. (Testing gets rebased all the time but hopefully the name makes it clear it's not stable!) I need to rebase my local togreg currently to get some fixes that are in Greg's tree, but not mine. If I do that before he's take my pull things will look unecessarily messy in the history. It's one of those silly corner cases where by waiting a little while I can hide the mess under the covers :) > > > Unfortunately I'm doubtful about whether I can squeeze in a second > > pull request this cycle, so they may get queued up for next cycle. > > A bit of bad timing :( > > It's not a big problem for me. There is still much to do (also a bit in > drivers/iio) before I tackle the final bits of my quest and actually > change struct i2c_driver (and so depend on these patches having hit > mainline). The only downside for you is, that you might have to endure > me asking again for the state of these patches ;-) > > Thanks for your feedback. Compared to pinging repeatedly and getting no > maintainer reply, knowing about your problems is much appreciated. > > Best regards and have a nice week-end, Likewise! Jonathan > Uwe >