nvmem_register() currently registers the device before adding the nvmem cells, which creates a race window where consumers may find the nvmem device (and not get PROBE_DEFERred), but then fail to find the cells and error out.
Move device registration to the end of nvmem_register(), to close the race.
Observed when the stars line up on Apple Silicon machines with the (not yet upstream, but trivial) spmi nvmem driver and the macsmc-rtc client:
[ 0.487375] macsmc-rtc macsmc-rtc: error -ENOENT: Failed to get rtc_offset NVMEM cell
Fixes: eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers") Cc: stable@vger.kernel.org Reviewed-by: Eric Curtin ecurtin@redhat.com Signed-off-by: Hector Martin marcan@marcan.st --- drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) { - ida_free(&nvmem_ida, nvmem->id); - kfree(nvmem); - return ERR_PTR(rval); - } + if (rval) + goto err_gpiod_put;
nvmem->read_only = device_property_present(config->dev, "read-only") || config->read_only || !nvmem->reg_write; @@ -837,20 +834,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
- rval = device_register(&nvmem->dev); - if (rval) - goto err_put_device; - if (nvmem->nkeepout) { rval = nvmem_validate_keepouts(nvmem); if (rval) - goto err_device_del; + goto err_gpiod_put; }
if (config->compat) { rval = nvmem_sysfs_setup_compat(nvmem, config); if (rval) - goto err_device_del; + goto err_gpiod_put; }
if (config->cells) { @@ -867,6 +860,15 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) if (rval) goto err_remove_cells;
+ rval = device_register(&nvmem->dev); + if (rval) { + nvmem_device_remove_all_cells(nvmem); + if (config->compat) + nvmem_sysfs_remove_compat(nvmem, config); + put_device(&nvmem->dev); + return ERR_PTR(rval); + } + blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
return nvmem; @@ -876,10 +878,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) err_teardown_compat: if (config->compat) nvmem_sysfs_remove_compat(nvmem, config); -err_device_del: - device_del(&nvmem->dev); -err_put_device: - put_device(&nvmem->dev); +err_gpiod_put: + gpiod_put(nvmem->wp_gpio); + ida_free(&nvmem_ida, nvmem->id); + kfree(nvmem);
return ERR_PTR(rval); } -- 2.35.1
On 03/01/2023 11:44, Hector Martin wrote:
nvmem_register() currently registers the device before adding the nvmem cells, which creates a race window where consumers may find the nvmem device (and not get PROBE_DEFERred), but then fail to find the cells and error out.
Move device registration to the end of nvmem_register(), to close the race.
Observed when the stars line up on Apple Silicon machines with the (not yet upstream, but trivial) spmi nvmem driver and the macsmc-rtc client:
[ 0.487375] macsmc-rtc macsmc-rtc: error -ENOENT: Failed to get rtc_offset NVMEM cell
Fixes: eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers") Cc: stable@vger.kernel.org Reviewed-by: Eric Curtin ecurtin@redhat.com Signed-off-by: Hector Martin marcan@marcan.st
What has changed since v1?
drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;
Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
nvmem->read_only = device_property_present(config->dev, "read-only") || config->read_only || !nvmem->reg_write; @@ -837,20 +834,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
- rval = device_register(&nvmem->dev);
- if (rval)
goto err_put_device;- if (nvmem->nkeepout) { rval = nvmem_validate_keepouts(nvmem); if (rval)
goto err_device_del;
goto err_gpiod_put;}
if (config->compat) { rval = nvmem_sysfs_setup_compat(nvmem, config); if (rval)
goto err_device_del;
goto err_gpiod_put;}
if (config->cells) {
@@ -867,6 +860,15 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) if (rval) goto err_remove_cells;
rval = device_register(&nvmem->dev);
if (rval) {
nvmem_device_remove_all_cells(nvmem);if (config->compat)nvmem_sysfs_remove_compat(nvmem, config);put_device(&nvmem->dev);return ERR_PTR(rval);}
blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
return nvmem;
@@ -876,10 +878,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) err_teardown_compat: if (config->compat) nvmem_sysfs_remove_compat(nvmem, config); -err_device_del:
- device_del(&nvmem->dev);
-err_put_device:
- put_device(&nvmem->dev);
+err_gpiod_put:
gpiod_put(nvmem->wp_gpio);
ida_free(&nvmem_ida, nvmem->id);
kfree(nvmem);
return ERR_PTR(rval); }
-- 2.35.1
On 03/01/2023 21.41, Srinivas Kandagatla wrote:
On 03/01/2023 11:44, Hector Martin wrote:
nvmem_register() currently registers the device before adding the nvmem cells, which creates a race window where consumers may find the nvmem device (and not get PROBE_DEFERred), but then fail to find the cells and error out.
Move device registration to the end of nvmem_register(), to close the race.
Observed when the stars line up on Apple Silicon machines with the (not yet upstream, but trivial) spmi nvmem driver and the macsmc-rtc client:
[ 0.487375] macsmc-rtc macsmc-rtc: error -ENOENT: Failed to get rtc_offset NVMEM cell
Fixes: eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers") Cc: stable@vger.kernel.org Reviewed-by: Eric Curtin ecurtin@redhat.com Signed-off-by: Hector Martin marcan@marcan.st
What has changed since v1?
What you told me to. I'm trying to get a silly bug fixed after you ignored my original patch until Russell independently discovered and submitted a fix for the same thing. I think we've wasted enough developer time here already.
drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so doing this is correct in every other instance below and maintains existing behavior, and it just so happens that this instance converges into the same codepath so it is correct to merge it, and it just so happens that the gpiod put was missing in this path to begin with so this becomes a drive-by bugfix.
If you don't like it I can remove it (i.e. reintroduce the bug for no good reason) and you can submit this fix yourself, because I have no incentive to waste time submitting a separate patch to fix a GPIO leak in an error path corner case in a subsystem I don't own and I have much bigger things to spend my (increasingly lower and lower) willingness to fight for upstream submissions than this.
Seriously, what is wrong with y'all kernel people. No other open source project wastes contributors' time with stupid nitpicks like this. I found a bug, I fixed it, I then fixed the issues you pointed out, and I don't have the time nor energy to fight over this kind of nonsense next. Do you want bugs fixed or not?
nvmem->read_only = device_property_present(config->dev, "read-only") || config->read_only || !nvmem->reg_write; @@ -837,20 +834,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
- rval = device_register(&nvmem->dev);
- if (rval)
goto err_put_device;- if (nvmem->nkeepout) { rval = nvmem_validate_keepouts(nvmem); if (rval)
goto err_device_del;
goto err_gpiod_put;}
if (config->compat) { rval = nvmem_sysfs_setup_compat(nvmem, config); if (rval)
goto err_device_del;
goto err_gpiod_put;}
if (config->cells) {
@@ -867,6 +860,15 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) if (rval) goto err_remove_cells;
rval = device_register(&nvmem->dev);
if (rval) {
nvmem_device_remove_all_cells(nvmem);if (config->compat)nvmem_sysfs_remove_compat(nvmem, config);put_device(&nvmem->dev);return ERR_PTR(rval);}
blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
return nvmem;
@@ -876,10 +878,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) err_teardown_compat: if (config->compat) nvmem_sysfs_remove_compat(nvmem, config); -err_device_del:
- device_del(&nvmem->dev);
-err_put_device:
- put_device(&nvmem->dev);
+err_gpiod_put:
gpiod_put(nvmem->wp_gpio);
ida_free(&nvmem_ida, nvmem->id);
kfree(nvmem);
return ERR_PTR(rval); }
-- 2.35.1
- Hector
On 03/01/2023 13:48, Hector Martin wrote:
On 03/01/2023 21.41, Srinivas Kandagatla wrote:
On 03/01/2023 11:44, Hector Martin wrote:
nvmem_register() currently registers the device before adding the nvmem cells, which creates a race window where consumers may find the nvmem device (and not get PROBE_DEFERred), but then fail to find the cells and error out.
Move device registration to the end of nvmem_register(), to close the race.
Observed when the stars line up on Apple Silicon machines with the (not yet upstream, but trivial) spmi nvmem driver and the macsmc-rtc client:
[ 0.487375] macsmc-rtc macsmc-rtc: error -ENOENT: Failed to get rtc_offset NVMEM cell
Fixes: eace75cfdcf7 ("nvmem: Add a simple NVMEM framework for nvmem providers") Cc: stable@vger.kernel.org Reviewed-by: Eric Curtin ecurtin@redhat.com Signed-off-by: Hector Martin marcan@marcan.st
What has changed since v1?
What you told me to. I'm trying to get a silly bug fixed after you ignored my original patch until Russell independently discovered and submitted a fix for the same thing. I think we've wasted enough developer time here already.
You should remember that maintainers have other regular job and holidays apart from maintaining. When you last sent the patch we are already in near/middle of merge window. If I had applied your original patch without proper review, It would have introduced more regressions. Be patient and we/I understand your concern.
Its always possible that multiple developers hit same bug and endup in multiple patches, there is no way to avoid this unless every developer checks for similar patches on the list.
drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so
This are clearly untested, And I dont want this to be in the middle to fix to the issue you are hitting. We should always be careful about untested changes, in this case gpiod has some conditions to check before doing a put. So the patch is incorrect as it is.
doing this is correct in every other instance below and maintains existing behavior, and it just so happens that this instance converges into the same codepath so it is correct to merge it, and it just so happens that the gpiod put was missing in this path to begin with so this becomes a drive-by bugfix.
If you don't like it I can remove it (i.e. reintroduce the bug for no good reason) and you can submit this fix yourself, because I have no incentive to waste time submitting a separate patch to fix a GPIO leak in an error path corner case in a subsystem I don't own and I have much bigger things to spend my (increasingly lower and lower) willingness to fight for upstream submissions than this.
Seriously, what is wrong with y'all kernel people. No other open source project wastes contributors' time with stupid nitpicks like this. I
These are not stupid nit picks your v1/v2 patches introduced memory leaks and regressions so i will not be picking up any patches that fall in that area.
found a bug, I fixed it, I then fixed the issues you pointed out, and I don't have the time nor energy to fight over this kind of nonsense next.
I think its worth reading ./Documentation/process/submitting-patches.rst
thanks, Srini
Do you want bugs fixed or not?
nvmem->read_only = device_property_present(config->dev, "read-only") || config->read_only || !nvmem->reg_write;@@ -837,20 +834,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
- rval = device_register(&nvmem->dev);
- if (rval)
goto err_put_device;- if (nvmem->nkeepout) { rval = nvmem_validate_keepouts(nvmem); if (rval)
goto err_device_del;
goto err_gpiod_put;}
if (config->compat) { rval = nvmem_sysfs_setup_compat(nvmem, config); if (rval)
goto err_device_del;
goto err_gpiod_put;}
if (config->cells) {
@@ -867,6 +860,15 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) if (rval) goto err_remove_cells;
rval = device_register(&nvmem->dev);
if (rval) {
nvmem_device_remove_all_cells(nvmem);if (config->compat)nvmem_sysfs_remove_compat(nvmem, config);put_device(&nvmem->dev);return ERR_PTR(rval);}
blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
return nvmem;
@@ -876,10 +878,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) err_teardown_compat: if (config->compat) nvmem_sysfs_remove_compat(nvmem, config); -err_device_del:
- device_del(&nvmem->dev);
-err_put_device:
- put_device(&nvmem->dev);
+err_gpiod_put:
gpiod_put(nvmem->wp_gpio);
ida_free(&nvmem_ida, nvmem->id);
kfree(nvmem);
return ERR_PTR(rval); }
-- 2.35.1
- Hector
On 03/01/2023 23.22, Srinivas Kandagatla wrote:
drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so
This are clearly untested, And I dont want this to be in the middle to fix to the issue you are hitting.
I somehow doubt you tested any of these error paths either. Nobody tests initialization error paths. That's why there was a gpio leak here to begin with.
We should always be careful about untested changes, in this case gpiod has some conditions to check before doing a put. So the patch is incorrect as it is.
Then the existing code is also incorrect as it is, because the device release callback is doing the same gpiod_put() already. I just moved it out since we are now registering the device later.
These are not stupid nit picks your v1/v2 patches introduced memory leaks and regressions so i will not be picking up any patches that fall in that area.
v1 certainly had issues which you rightly pointed out. Now with v2 you are nitpicking that I merged two codepaths that belong together, and fixed a corner case bug in the process. If there is an actual problem, please point it out. "Lol why did you fix this other bug that naturally falls into the influence of the changes being made" is not constructive.
found a bug, I fixed it, I then fixed the issues you pointed out, and I don't have the time nor energy to fight over this kind of nonsense next.
I think its worth reading ./Documentation/process/submitting-patches.rst
Oh I've read it alright. You're not the first maintainer to have me lose more faith in the kernel development process, this isn't my first rodeo (hint: check MAINTAINERS, I'm also a maintainer). And I know it doesn't have to be like this because other maintainers that are actually reasonable and nice to work with do exist.
I'm going to note that right now this core subsystem code is broken in the *success path*, while all the arguments here are about the *error path*. In other words, there is a negligible change that any mistakes/regressions I'm making here would actually impact people, while the status quo is that the code is actively breaking people's systems.
So, are you going to actually point out the supposed regressions with v2 so we can actually fix this bug, or should I just drop this submission and keep it in my downstream tree and you can continue to get bug reports about this race condition?
- Hector
On Tue, Jan 03, 2023 at 11:56:21PM +0900, Hector Martin wrote:
On 03/01/2023 23.22, Srinivas Kandagatla wrote:
drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so
This are clearly untested, And I dont want this to be in the middle to fix to the issue you are hitting.
I somehow doubt you tested any of these error paths either. Nobody tests initialization error paths. That's why there was a gpio leak here to begin with.
Sadly, this is one of the biggest problems with error paths, they get very little proper testing - and in most cases we're reliant on reviewers spotting errors. That's why we much prefer the devm_* stuff, but even that can be error-prone.
We should always be careful about untested changes, in this case gpiod has some conditions to check before doing a put. So the patch is incorrect as it is.
Then the existing code is also incorrect as it is, because the device release callback is doing the same gpiod_put() already. I just moved it out since we are now registering the device later.
At the point where this change is being made (checking rval after dev_set_name()) the struct device has not been initialised, so the release callback will not be called. nvmem->wp_gpio will be leaked.
However, there may be bigger problems with wp_gpio - related to where it can come from and what we do with it.
nvmem->wp_gpio has two sources - one of them is gpiod_get_optional(), and we need to call gpiod_put() on that to drop the reference that _this_ code acquired. The other is config->wp_gpio - we don't own that reference, yet we call gpiod_put() on it. I'm not sure whether config->wp_gpio is actually used anywhere - my grepping so far has not found any users, but maybe Srivinas knows better.
Hence, sorting out the leak of wp_gpio needs more discussion, and it would not be right to delay merging the fix for the very real race that people hit today resulting in stuff not working while we try to work out how wp_gpio should be handled.
So... always fix one problem in one patch. Sometimes a fix is not as obvious as one may first think.
On 04/01/2023 00.18, Russell King (Oracle) wrote:
On Tue, Jan 03, 2023 at 11:56:21PM +0900, Hector Martin wrote:
On 03/01/2023 23.22, Srinivas Kandagatla wrote:
drivers/nvmem/core.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 321d7d63e068..606f428d6292 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so
This are clearly untested, And I dont want this to be in the middle to fix to the issue you are hitting.
I somehow doubt you tested any of these error paths either. Nobody tests initialization error paths. That's why there was a gpio leak here to begin with.
Sadly, this is one of the biggest problems with error paths, they get very little proper testing - and in most cases we're reliant on reviewers spotting errors. That's why we much prefer the devm_* stuff, but even that can be error-prone.
We should always be careful about untested changes, in this case gpiod has some conditions to check before doing a put. So the patch is incorrect as it is.
Then the existing code is also incorrect as it is, because the device release callback is doing the same gpiod_put() already. I just moved it out since we are now registering the device later.
At the point where this change is being made (checking rval after dev_set_name()) the struct device has not been initialised, so the release callback will not be called. nvmem->wp_gpio will be leaked.
But later in the code where device_put() was being called would will be, and that callback is calling gpiod_put() unconditionally, which is why I am doing the same after moving the device registration later.
Is this wrong? Well,
However, there may be bigger problems with wp_gpio - related to where it can come from and what we do with it.
nvmem->wp_gpio has two sources - one of them is gpiod_get_optional(), and we need to call gpiod_put() on that to drop the reference that _this_ code acquired. The other is config->wp_gpio - we don't own that reference, yet we call gpiod_put() on it. I'm not sure whether config->wp_gpio is actually used anywhere - my grepping so far has not found any users, but maybe Srivinas knows better.
... then yes, it's wrong, and the original code is already broken too. I merely moved functionality around in the other cases *except* the one case after dev_set_name(), where I swapped one bug out for calling other buggy code derived from existing buggy code. ¯_(ツ)_/¯
Hence, sorting out the leak of wp_gpio needs more discussion, and it would not be right to delay merging the fix for the very real race that people hit today resulting in stuff not working while we try to work out how wp_gpio should be handled.
So... always fix one problem in one patch. Sometimes a fix is not as obvious as one may first think.
Sure, but you do realize the issue here, right? This, coupled with the kernel development model (and *especially* the impolite discourse that often goes along with it from maintainers) incentivizes doing the minimum amount of work to fix the minimum amount of bugs that actually affect people. I couldn't care less about the gpiod leak fix, it doesn't affect me, but I noticed and thought I could fix it. Turns out it's more complicated than that because the existing code is even more broken than I thought, sure - but I have no incentive to start that conversation nor fix all this now. Now I just know next time I touch nvmem code I won't bother pointing out any bugs I notice by accident, because clearly the discussion likely to follow is more mental overhead than I'm willing to spend on something that doesn't affect me. When bugs don't noticeably break people and fixing them is too much effort, nobody will.
While if my change had been merged as-is, we'd have at least fixed one common case bug and reduced two related bugs to one class of bug (the unconditional gpiod_put()) which is still a strict improvement. And then someone can point out the incorrect puts when the GPIO isn't owned and fix that later if they want.
- Hector
On Wed, Jan 04, 2023 at 12:33:33AM +0900, Hector Martin wrote:
On 04/01/2023 00.18, Russell King (Oracle) wrote:
On Tue, Jan 03, 2023 at 11:56:21PM +0900, Hector Martin wrote:
On 03/01/2023 23.22, Srinivas Kandagatla wrote:
> drivers/nvmem/core.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index 321d7d63e068..606f428d6292 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) > break; > } > > - if (rval) { > - ida_free(&nvmem_ida, nvmem->id); > - kfree(nvmem); > - return ERR_PTR(rval); > - } > + if (rval) > + goto err_gpiod_put;
Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so
This are clearly untested, And I dont want this to be in the middle to fix to the issue you are hitting.
I somehow doubt you tested any of these error paths either. Nobody tests initialization error paths. That's why there was a gpio leak here to begin with.
Sadly, this is one of the biggest problems with error paths, they get very little proper testing - and in most cases we're reliant on reviewers spotting errors. That's why we much prefer the devm_* stuff, but even that can be error-prone.
We should always be careful about untested changes, in this case gpiod has some conditions to check before doing a put. So the patch is incorrect as it is.
Then the existing code is also incorrect as it is, because the device release callback is doing the same gpiod_put() already. I just moved it out since we are now registering the device later.
At the point where this change is being made (checking rval after dev_set_name()) the struct device has not been initialised, so the release callback will not be called. nvmem->wp_gpio will be leaked.
But later in the code where device_put() was being called would will be, and that callback is calling gpiod_put() unconditionally, which is why I am doing the same after moving the device registration later.
Is this wrong? Well,
I'm not going to read the rest of your rant, honestly it's really not worth it. Let's just concentrate on trying to work out how best to fix this crud.
Not only is there the issue with wp_gpio, but the whole IDA handling is fscked as well, so there's many problems to be sorted out here, and if we lump them all into one patch, we'll probably be getting to the point of completely rewriting nvmem_register() making backports extremely difficult.
Hi Hector,
On Tue, Jan 03, 2023 at 10:48:52PM +0900, Hector Martin wrote:
@@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so doing this is correct in every other instance below and maintains existing behavior, and it just so happens that this instance converges into the same codepath so it is correct to merge it, and it just so happens that the gpiod put was missing in this path to begin with so this becomes a drive-by bugfix.
If you don't like it I can remove it (i.e. reintroduce the bug for no good reason) and you can submit this fix yourself, because I have no incentive to waste time submitting a separate patch to fix a GPIO leak in an error path corner case in a subsystem I don't own and I have much bigger things to spend my (increasingly lower and lower) willingness to fight for upstream submissions than this.
Seriously, what is wrong with y'all kernel people. No other open source project wastes contributors' time with stupid nitpicks like this. I found a bug, I fixed it, I then fixed the issues you pointed out, and I don't have the time nor energy to fight over this kind of nonsense next. Do you want bugs fixed or not?
This is not nonsense. We have always had a policy of one fix/change per patch, and in this case it makes complete and utter sense. Of course, the interpretation of "one change" is a matter of opinion.
Your patch contains two bug fixes for problems: 1) publication of nvmem_device before it's fully setup (leading to the race) which has been around since the inception of nvmem stuff. 2) fixing a memory leak for gpiod stuff, caused by a recent patch 5544e90c8126 ("nvmem: core: add error handling for dev_set_name") from September 2022.
Hence these two changes need different treatment for stable backporting, with the former needing to be backported to more kernels than the latter. So, it makes complete sense to split the two fixes into their own separate patches.
Why are other projects happy to accept a patch that fixes multiple issues? Maybe they work in different ways - maybe they don't backport changes to older releases? Maybe they don't do multiple stable versions like we do with the kernel.
On 04/01/2023 00.06, Russell King (Oracle) wrote:
Hi Hector,
On Tue, Jan 03, 2023 at 10:48:52PM +0900, Hector Martin wrote:
@@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so doing this is correct in every other instance below and maintains existing behavior, and it just so happens that this instance converges into the same codepath so it is correct to merge it, and it just so happens that the gpiod put was missing in this path to begin with so this becomes a drive-by bugfix.
If you don't like it I can remove it (i.e. reintroduce the bug for no good reason) and you can submit this fix yourself, because I have no incentive to waste time submitting a separate patch to fix a GPIO leak in an error path corner case in a subsystem I don't own and I have much bigger things to spend my (increasingly lower and lower) willingness to fight for upstream submissions than this.
Seriously, what is wrong with y'all kernel people. No other open source project wastes contributors' time with stupid nitpicks like this. I found a bug, I fixed it, I then fixed the issues you pointed out, and I don't have the time nor energy to fight over this kind of nonsense next. Do you want bugs fixed or not?
This is not nonsense. We have always had a policy of one fix/change per patch, and in this case it makes complete and utter sense. Of course, the interpretation of "one change" is a matter of opinion.
The change here is the race condition fix. That change involves adding an error cleanup path that involves a gpio_put(). Therefore it seems logical to actually use it in that one extra case that should've used it anyway, a few lines above.
Now,
Your patch contains two bug fixes for problems:
- publication of nvmem_device before it's fully setup (leading to the race) which has been around since the inception of nvmem stuff.
- fixing a memory leak for gpiod stuff, caused by a recent patch 5544e90c8126 ("nvmem: core: add error handling for dev_set_name") from September 2022.
That's a fair argument for having two patches (I didn't know the gpiod leak was introduced later). However, the backport is nontrivial anyway if you want clean code, because if we merge the codepaths the fix would end up being different in backports and mainline. Which means we now need 3 patches for them to apply properly. Which is more effort than I'm willing to put in for an issue I don't care about.
But the bigger problem is that this isn't what Srini replied with, he's now saying my patch is outright broken, and I'm tired of this nonsense.
- Hector
On Wed, Jan 04, 2023 at 12:14:10AM +0900, Hector Martin wrote:
On 04/01/2023 00.06, Russell King (Oracle) wrote:
Hi Hector,
On Tue, Jan 03, 2023 at 10:48:52PM +0900, Hector Martin wrote:
@@ -822,11 +822,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) break; }
- if (rval) {
ida_free(&nvmem_ida, nvmem->id);kfree(nvmem);return ERR_PTR(rval);- }
- if (rval)
goto err_gpiod_put;Why was gpiod changes added to this patch, that should be a separate patch/discussion, as this is not relevant to the issue that you are reporting.
Because freeing the device also does a gpiod_put in the destructor, so doing this is correct in every other instance below and maintains existing behavior, and it just so happens that this instance converges into the same codepath so it is correct to merge it, and it just so happens that the gpiod put was missing in this path to begin with so this becomes a drive-by bugfix.
If you don't like it I can remove it (i.e. reintroduce the bug for no good reason) and you can submit this fix yourself, because I have no incentive to waste time submitting a separate patch to fix a GPIO leak in an error path corner case in a subsystem I don't own and I have much bigger things to spend my (increasingly lower and lower) willingness to fight for upstream submissions than this.
Seriously, what is wrong with y'all kernel people. No other open source project wastes contributors' time with stupid nitpicks like this. I found a bug, I fixed it, I then fixed the issues you pointed out, and I don't have the time nor energy to fight over this kind of nonsense next. Do you want bugs fixed or not?
This is not nonsense. We have always had a policy of one fix/change per patch, and in this case it makes complete and utter sense. Of course, the interpretation of "one change" is a matter of opinion.
The change here is the race condition fix. That change involves adding an error cleanup path that involves a gpio_put(). Therefore it seems logical to actually use it in that one extra case that should've used it anyway, a few lines above.
The two are entirely unrelated. as I've already explained. The call to device_register() happens _after_ the check for rval from the dev_set_name() that you are changing. Moving device_register() doesn't make the lack of gpiod_put() any better or worse than it was before.
That said, I'm now thinking that my patch is actually wrong, but for a different reason unrelated to the gpiod issue. :(
linux-stable-mirror@lists.linaro.org
-
Hector Martin -
Russell King (Oracle) -
Srinivas Kandagatla