| Message ID | 20210518163304.3702015-1-gregkh@linuxfoundation.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] b43: don't save dentries for debugfs | expand |
On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: > On 2021-05-18 12:29, Jeff Johnson wrote: > > On 2021-05-18 09:33, Greg Kroah-Hartman wrote: > > > There is no need to keep around the dentry pointers for the debugfs > > > files as they will all be automatically removed when the subdir is > > > removed. So save the space and logic involved in keeping them > > > around by > > > just getting rid of them entirely. > > > > > > By doing this change, we remove one of the last in-kernel user that > > > was > > > storing the result of debugfs_create_bool(), so that api can be > > > cleaned > > > up. > > > > Question not about this specific change, but the general concept > > of keeping (or not keeping) dentry pointers. In the ath drivers, > > as well as in an out-of-tree driver for Android, we keep a > > debugfs dentry pointer to use as a param to relay_open(). > > > > Will we still be able to have a dentry pointer for this purpose? > > Or better, is there a recommended way to get a dentry pointer > > NOT associated with debugfs at all (which would be ideal for > > Android where debugfs is disabled). > > Answering one of my questions: The dentry passed to relay_open() comes > from debugfs_create_dir() which is expected to return a dentry. > > Would still like guidance on if there is a recommended way to get a > dentry not associated with debugfs. What do you exactly mean by "not associated with debugfs"? And why are you passing a debugfs dentry to relay_open()? That feels really wrong and fragile. Ideally I want to get rid of the "raw" dentry that debugfs returns to callers, as it has caused odd problems in the past, but that's a very long-term project... thanks, greg k-h
On 2021-05-18 22:05, Greg Kroah-Hartman wrote: > On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: >> On 2021-05-18 12:29, Jeff Johnson wrote: >> Would still like guidance on if there is a recommended way to get a >> dentry not associated with debugfs. > > What do you exactly mean by "not associated with debugfs"? > > And why are you passing a debugfs dentry to relay_open()? That feels > really wrong and fragile. I don't know the history but the relay documentation tells us: "If you want a directory structure to contain your relay files, you should create it using the host filesystem’s directory creation function, e.g. debugfs_create_dir()..." So my guess is that the original implementation followed that advice. I see 5 clients of this functionality, and all 5 pass a dentry returned from debugfs_create_dir(): drivers/gpu/drm/i915/gt/uc/intel_guc_log.c, line 384 drivers/net/wireless/ath/ath10k/spectral.c, line 534 drivers/net/wireless/ath/ath11k/spectral.c, line 902 drivers/net/wireless/ath/ath9k/common-spectral.c, line 1077 kernel/trace/blktrace.c, line 549 Jeff -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Wed, May 19, 2021 at 08:04:59AM -0700, Jeff Johnson wrote: > On 2021-05-18 22:05, Greg Kroah-Hartman wrote: > > On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: > > > On 2021-05-18 12:29, Jeff Johnson wrote: > > > Would still like guidance on if there is a recommended way to get a > > > dentry not associated with debugfs. > > > > What do you exactly mean by "not associated with debugfs"? > > > > And why are you passing a debugfs dentry to relay_open()? That feels > > really wrong and fragile. > > I don't know the history but the relay documentation tells us: > "If you want a directory structure to contain your relay files, > you should create it using the host filesystem’s directory > creation function, e.g. debugfs_create_dir()..." > > So my guess is that the original implementation followed that > advice. I see 5 clients of this functionality, and all 5 pass a > dentry returned from debugfs_create_dir(): > > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c, line 384 > drivers/net/wireless/ath/ath10k/spectral.c, line 534 > drivers/net/wireless/ath/ath11k/spectral.c, line 902 > drivers/net/wireless/ath/ath9k/common-spectral.c, line 1077 > kernel/trace/blktrace.c, line 549 Ah, that's just the "parent" dentry for the relayfs file. That's fine, not a big deal, debugfs will always provide a way for you to get that if needed. thanks, greg k-h
On 2021-05-19 08:42, Greg Kroah-Hartman wrote: > On Wed, May 19, 2021 at 08:04:59AM -0700, Jeff Johnson wrote: >> On 2021-05-18 22:05, Greg Kroah-Hartman wrote: >> > On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: >> > > On 2021-05-18 12:29, Jeff Johnson wrote: >> > > Would still like guidance on if there is a recommended way to get a >> > > dentry not associated with debugfs. >> > >> > What do you exactly mean by "not associated with debugfs"? >> > >> > And why are you passing a debugfs dentry to relay_open()? That feels >> > really wrong and fragile. >> >> I don't know the history but the relay documentation tells us: >> "If you want a directory structure to contain your relay files, >> you should create it using the host filesystem’s directory >> creation function, e.g. debugfs_create_dir()..." >> >> So my guess is that the original implementation followed that >> advice. I see 5 clients of this functionality, and all 5 pass a >> dentry returned from debugfs_create_dir(): >> >> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c, line 384 >> drivers/net/wireless/ath/ath10k/spectral.c, line 534 >> drivers/net/wireless/ath/ath11k/spectral.c, line 902 >> drivers/net/wireless/ath/ath9k/common-spectral.c, line 1077 >> kernel/trace/blktrace.c, line 549 > > Ah, that's just the "parent" dentry for the relayfs file. That's fine, > not a big deal, debugfs will always provide a way for you to get that > if > needed. Unless debugfs is disabled, like on Android, which is the real problem I'm trying to solve. Jeff
On Wed, May 19, 2021 at 08:57:00AM -0700, Jeff Johnson wrote: > On 2021-05-19 08:42, Greg Kroah-Hartman wrote: > > On Wed, May 19, 2021 at 08:04:59AM -0700, Jeff Johnson wrote: > > > On 2021-05-18 22:05, Greg Kroah-Hartman wrote: > > > > On Tue, May 18, 2021 at 03:00:44PM -0700, Jeff Johnson wrote: > > > > > On 2021-05-18 12:29, Jeff Johnson wrote: > > > > > Would still like guidance on if there is a recommended way to get a > > > > > dentry not associated with debugfs. > > > > > > > > What do you exactly mean by "not associated with debugfs"? > > > > > > > > And why are you passing a debugfs dentry to relay_open()? That feels > > > > really wrong and fragile. > > > > > > I don't know the history but the relay documentation tells us: > > > "If you want a directory structure to contain your relay files, > > > you should create it using the host filesystem’s directory > > > creation function, e.g. debugfs_create_dir()..." > > > > > > So my guess is that the original implementation followed that > > > advice. I see 5 clients of this functionality, and all 5 pass a > > > dentry returned from debugfs_create_dir(): > > > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c, line 384 > > > drivers/net/wireless/ath/ath10k/spectral.c, line 534 > > > drivers/net/wireless/ath/ath11k/spectral.c, line 902 > > > drivers/net/wireless/ath/ath9k/common-spectral.c, line 1077 > > > kernel/trace/blktrace.c, line 549 > > > > Ah, that's just the "parent" dentry for the relayfs file. That's fine, > > not a big deal, debugfs will always provide a way for you to get that if > > needed. > > Unless debugfs is disabled, like on Android, which is the real problem I'm > trying to solve. Then use some other filesystem to place your relay file in. A relay file is not a file that userspace should rely on for normal operation, so why do you need it at all? What tools/operation requires access to this file that systems without debugfs support is causing problems on? thanks, greg k-h
On Tue, May 18, 2021 at 08:47:58PM +0300, Kalle Valo wrote: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > > There is no need to keep around the dentry pointers for the debugfs > > files as they will all be automatically removed when the subdir is > > removed. So save the space and logic involved in keeping them around by > > just getting rid of them entirely. > > > > By doing this change, we remove one of the last in-kernel user that was > > storing the result of debugfs_create_bool(), so that api can be cleaned > > up. > > > > Cc: Kalle Valo <kvalo@codeaurora.org> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Jason Gunthorpe <jgg@ziepe.ca> > > Cc: Chao Yu <chao@kernel.org> > > Cc: Leon Romanovsky <leon@kernel.org> > > Cc: linux-wireless@vger.kernel.org > > Cc: b43-dev@lists.infradead.org > > Cc: netdev@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > drivers/net/wireless/broadcom/b43/debugfs.c | 34 +++------------------ > > drivers/net/wireless/broadcom/b43/debugfs.h | 3 -- > > 2 files changed, 5 insertions(+), 32 deletions(-) > > > > Note, I can take this through my debugfs tree if wanted, that way I can > > clean up the debugfs_create_bool() api at the same time. Otherwise it's > > fine, I can wait until next -rc1 for that to happen. > > Yeah, it's best that you take this via your tree. > > Acked-by: Kalle Valo <kvalo@codeaurora.org> Thanks, will do! greg k-h
diff --git a/drivers/net/wireless/broadcom/b43/debugfs.c b/drivers/net/wireless/broadcom/b43/debugfs.c index 89a25aefb327..efa98444e3fb 100644 --- a/drivers/net/wireless/broadcom/b43/debugfs.c +++ b/drivers/net/wireless/broadcom/b43/debugfs.c @@ -643,24 +643,14 @@ bool b43_debug(struct b43_wldev *dev, enum b43_dyndbg feature) return enabled; } -static void b43_remove_dynamic_debug(struct b43_wldev *dev) -{ - struct b43_dfsentry *e = dev->dfsentry; - int i; - - for (i = 0; i < __B43_NR_DYNDBG; i++) - debugfs_remove(e->dyn_debug_dentries[i]); -} - static void b43_add_dynamic_debug(struct b43_wldev *dev) { struct b43_dfsentry *e = dev->dfsentry; #define add_dyn_dbg(name, id, initstate) do { \ e->dyn_debug[id] = (initstate); \ - e->dyn_debug_dentries[id] = \ - debugfs_create_bool(name, 0600, e->subdir, \ - &(e->dyn_debug[id])); \ + debugfs_create_bool(name, 0600, e->subdir, \ + &(e->dyn_debug[id])); \ } while (0) add_dyn_dbg("debug_xmitpower", B43_DBG_XMITPOWER, false); @@ -713,10 +703,9 @@ void b43_debugfs_add_device(struct b43_wldev *dev) #define ADD_FILE(name, mode) \ do { \ - e->file_##name.dentry = \ - debugfs_create_file(__stringify(name), \ - mode, e->subdir, dev, \ - &fops_##name.fops); \ + debugfs_create_file(__stringify(name), \ + mode, e->subdir, dev, \ + &fops_##name.fops); \ } while (0) @@ -746,19 +735,6 @@ void b43_debugfs_remove_device(struct b43_wldev *dev) e = dev->dfsentry; if (!e) return; - b43_remove_dynamic_debug(dev); - - debugfs_remove(e->file_shm16read.dentry); - debugfs_remove(e->file_shm16write.dentry); - debugfs_remove(e->file_shm32read.dentry); - debugfs_remove(e->file_shm32write.dentry); - debugfs_remove(e->file_mmio16read.dentry); - debugfs_remove(e->file_mmio16write.dentry); - debugfs_remove(e->file_mmio32read.dentry); - debugfs_remove(e->file_mmio32write.dentry); - debugfs_remove(e->file_txstat.dentry); - debugfs_remove(e->file_restart.dentry); - debugfs_remove(e->file_loctls.dentry); debugfs_remove(e->subdir); kfree(e->txstatlog.log); diff --git a/drivers/net/wireless/broadcom/b43/debugfs.h b/drivers/net/wireless/broadcom/b43/debugfs.h index 0bf437c86c67..6f6b500b8881 100644 --- a/drivers/net/wireless/broadcom/b43/debugfs.h +++ b/drivers/net/wireless/broadcom/b43/debugfs.h @@ -32,7 +32,6 @@ struct b43_txstatus_log { }; struct b43_dfs_file { - struct dentry *dentry; char *buffer; size_t data_len; }; @@ -70,8 +69,6 @@ struct b43_dfsentry { /* Enabled/Disabled list for the dynamic debugging features. */ bool dyn_debug[__B43_NR_DYNDBG]; - /* Dentries for the dynamic debugging entries. */ - struct dentry *dyn_debug_dentries[__B43_NR_DYNDBG]; }; bool b43_debug(struct b43_wldev *dev, enum b43_dyndbg feature);
There is no need to keep around the dentry pointers for the debugfs files as they will all be automatically removed when the subdir is removed. So save the space and logic involved in keeping them around by just getting rid of them entirely. By doing this change, we remove one of the last in-kernel user that was storing the result of debugfs_create_bool(), so that api can be cleaned up. Cc: Kalle Valo <kvalo@codeaurora.org> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Chao Yu <chao@kernel.org> Cc: Leon Romanovsky <leon@kernel.org> Cc: linux-wireless@vger.kernel.org Cc: b43-dev@lists.infradead.org Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- drivers/net/wireless/broadcom/b43/debugfs.c | 34 +++------------------ drivers/net/wireless/broadcom/b43/debugfs.h | 3 -- 2 files changed, 5 insertions(+), 32 deletions(-) Note, I can take this through my debugfs tree if wanted, that way I can clean up the debugfs_create_bool() api at the same time. Otherwise it's fine, I can wait until next -rc1 for that to happen. v2: remove X/N from subject, it is a stand-alone patch.