gpio: sysfs: add missing mutex_destroy()
Subsystems (2)
PATCH 1/1 gpio: sysfs: add missing mutex_destroy()
From: Bartosz Golaszewski <[email protected]>
We initialize the data->mutex in gpiod_export() but lack the corresponding mutex_destroy() in gpiod_unexport() causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.
--- drivers/gpio/gpiolib-sysfs.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpio/gpiolib-sysfs.c +1 -0
@@ -713,6 +713,7 @@ void gpiod_unexport(struct gpio_desc *desc) }put_device (dev); +mutex_destroy (&data->mutex);kfree (data); }EXPORT_SYMBOL_GPL (gpiod_unexport);
Signature · 1 line
2.45.2
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We initialize the
data->mutexingpiod_export()but lack the correspondingmutex_destroy()ingpiod_unexport()causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.
No, there's no resource leak and it's perfectly fine not to call mutex_destroy().
You can't just make shit up and then pretend to fix it...
Quoted 3 lines
Fixes 6ffcb7971486 ("gpio: sysfs: use per-gpio locking")Signed-off-by Bartosz Golaszewski <[email protected]>
Johan
Quoted 4 lines · Johan Hovold
Quoted 2 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
Quoted 8 lines
Quoted 4 lines
We initialize the
data->mutexingpiod_export()but lack the correspondingmutex_destroy()ingpiod_unexport()causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.No, there's no resource leak and it's perfectly fine not to call
mutex_destroy().
No, there's no leak but with lock debugging it still warns if the mutex is locked when it's being destroyed so the change still makes sense with a modified commit message.
Quoted 2 lines
You can't just make shit up and then pretend to fix it...
There's no need for this kind of comment. You made your point clear in the first sentence.
Bartosz
Quoted 13 lines · Bartosz Golaszewski
Quoted 11 lines · Johan Hovold
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We initialize the
data->mutexingpiod_export()but lack the correspondingmutex_destroy()ingpiod_unexport()causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.No, there's no resource leak and it's perfectly fine not to call
mutex_destroy().
Quoted 8 lines
No, there's no leak but with lock debugging it still warns if the mutex is locked when it's being destroyed so the change still makes sense with a modified commit message.
Quoted 1 line
You can't just make shit up and then pretend to fix it...
There's no need for this kind of comment. You made your point clear in the first sentence.
Your claim that there's "a resource leak with mutex debugging enabled" is is quite specific. Now I had to go check that no one had changed something in ways they shouldn't have recently. But mutex_destroy() still works as it always has, which you should have verified yourself before sending a "fix" tagged for stable backport based on a hunch.
Johan
Quoted 21 lines · Johan Hovold
Quoted 19 lines · Bartosz Golaszewski
Quoted 11 lines · Johan Hovold
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We initialize the
data->mutexingpiod_export()but lack the correspondingmutex_destroy()ingpiod_unexport()causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.No, there's no resource leak and it's perfectly fine not to call
mutex_destroy().No, there's no leak but with lock debugging it still warns if the mutex is locked when it's being destroyed so the change still makes sense with a modified commit message.
Quoted 1 line
You can't just make shit up and then pretend to fix it...
Quoted 9 lines
Quoted 2 lines
There's no need for this kind of comment. You made your point clear in the first sentence.
Your claim that there's "a resource leak with mutex debugging enabled" is is quite specific. Now I had to go check that no one had changed something in ways they shouldn't have recently. But
mutex_destroy()still works as it always has, which you should have verified yourself before sending a "fix" tagged for stable backport based on a hunch.
Yes, I admitted that the commit message was wrong. And yes, it sometimes happens that we get copied on crappy patches. However, unlike what your comment suggests, I don't go around the kernel, "making sh*t up" just to add a "Fixes: Johan's commit". I had this as part of a bigger rework I have in progress[1] (discussed previously here[2]) and figured that with the series growing in size, I'll at least get the fix upstream before v6.16-rc1.
I should have given the patch more than 10 seconds of thought for sure but your immediate hostility is uncalled for. Please try to assume good faith a bit more.
Bartosz
[1] https://github.com/brgl/linux/tree/b4/gpio-sysfs-chip-export
[2] https://lore.kernel.org/all/CAMRc=McUCeZcU6co1aN54rTudo+JfPjjForu4iKQ5npwXk6GXA@mail.gmail.com/
Quoted 29 lines · Bartosz Golaszewski
Quoted 19 lines · Johan Hovold
Quoted 18 lines · Bartosz Golaszewski
Quoted 10 lines · Johan Hovold
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We initialize the
data->mutexingpiod_export()but lack the correspondingmutex_destroy()ingpiod_unexport()causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.No, there's no resource leak and it's perfectly fine not to call
mutex_destroy().No, there's no leak but with lock debugging it still warns if the mutex is locked when it's being destroyed so the change still makes sense with a modified commit message.
Quoted 1 line
You can't just make shit up and then pretend to fix it...
Quoted 8 lines
Quoted 2 lines
There's no need for this kind of comment. You made your point clear in the first sentence.
Your claim that there's "a resource leak with mutex debugging enabled" is is quite specific. Now I had to go check that no one had changed something in ways they shouldn't have recently. But
mutex_destroy()still works as it always has, which you should have verified yourself before sending a "fix" tagged for stable backport based on a hunch.
Quoted 7 lines
Yes, I admitted that the commit message was wrong. And yes, it sometimes happens that we get copied on crappy patches. However, unlike what your comment suggests, I don't go around the kernel, "making sh*t up" just to add a "Fixes: Johan's commit". I had this as part of a bigger rework I have in progress[1] (discussed previously here[2]) and figured that with the series growing in size, I'll at least get the fix upstream before v6.16-rc1.
But it is not a fix. It is based on a misunderstanding that you should have caught yourself by just looking at the code before posting.
Sure, mutex_destroy() is an odd bird, but you still need to verify your guesses before posting patches based on them. It's that lazy attitude (and violation of the stable kernel policy) that I'm criticising.
Johan
Quoted 39 lines · Johan Hovold
Quoted 29 lines · Bartosz Golaszewski
Quoted 19 lines · Johan Hovold
Quoted 18 lines · Bartosz Golaszewski
Quoted 10 lines · Johan Hovold
Quoted 6 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
We initialize the
data->mutexingpiod_export()but lack the correspondingmutex_destroy()ingpiod_unexport()causing a resource leak with mutex debugging enabled. Add the call right before kfreeing the GPIO data.No, there's no resource leak and it's perfectly fine not to call
mutex_destroy().No, there's no leak but with lock debugging it still warns if the mutex is locked when it's being destroyed so the change still makes sense with a modified commit message.
Quoted 1 line
You can't just make shit up and then pretend to fix it...
Quoted 8 lines
Quoted 2 lines
There's no need for this kind of comment. You made your point clear in the first sentence.
Your claim that there's "a resource leak with mutex debugging enabled" is is quite specific. Now I had to go check that no one had changed something in ways they shouldn't have recently. But
mutex_destroy()still works as it always has, which you should have verified yourself before sending a "fix" tagged for stable backport based on a hunch.Quoted 7 lines
Yes, I admitted that the commit message was wrong. And yes, it sometimes happens that we get copied on crappy patches. However, unlike what your comment suggests, I don't go around the kernel, "making sh*t up" just to add a "Fixes: Johan's commit". I had this as part of a bigger rework I have in progress[1] (discussed previously here[2]) and figured that with the series growing in size, I'll at least get the fix upstream before v6.16-rc1.
Quoted 3 lines
But it is not a fix. It is based on a misunderstanding that you should have caught yourself by just looking at the code before posting.
Noted, I'll drop the Fixes: tag and submit it as part of the rest of the rework.
Quoted 4 lines
Sure,
mutex_destroy()is an odd bird, but you still need to verify your guesses before posting patches based on them. It's that lazy attitude (and violation of the stable kernel policy) that I'm criticising.
IMO what you just wrote here is a way better way of expressing your criticism than what you led with.
Bartosz