Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 04/17/2018 12:38 PM, Hook, Gary wrote: On 4/13/2018 8:08 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: + + + mutex_lock(_iommu_debugfs_lock); + if (!amd_iommu_debugfs) { + d_top = iommu_debugfs_setup(); + if (d_top) + amd_iommu_debugfs = debugfs_create_dir("amd", d_top); + } + mutex_unlock(_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. I'm not sure it matters, given the finite number of IOMMUs in a system, and the fact that this work is done exactly once. However, removal of a lock is fine thing, so I'll move this around. After thinking about this, and looking at the code, I've decided to leave this alone. v4 is on its way.
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 04/17/2018 12:38 PM, Hook, Gary wrote: On 4/13/2018 8:08 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: + + + mutex_lock(_iommu_debugfs_lock); + if (!amd_iommu_debugfs) { + d_top = iommu_debugfs_setup(); + if (d_top) + amd_iommu_debugfs = debugfs_create_dir("amd", d_top); + } + mutex_unlock(_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. I'm not sure it matters, given the finite number of IOMMUs in a system, and the fact that this work is done exactly once. However, removal of a lock is fine thing, so I'll move this around. After thinking about this, and looking at the code, I've decided to leave this alone. v4 is on its way.
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Hi, Gary, On Wed, 2018-04-18 at 16:51 -0400, Hook, Gary wrote: > On 4/18/2018 4:16 PM, Mehta, Sohil wrote: > > > > On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote: > > > > > > Maybe the original design is to call debugfs_initialized() before > > > calling debugfs_create_xxx()? > > I am unaware of the original design. Someone else would probably > > have > > more context. However, looking at other places in the kernel where > > debugfs_create_xx() is used, the common convention seems to be to > > avoid > > calling debugfs_initialized(). > > > > Sohil > > > debugfs_initialized() was introduced in commit c0f92ba99 back in > 2.6.30-rc1. It was intended as a helper, not as a gatekeeper, which > is > why one doesn't see it used. Given that my use in this proposed patch > is > straightforward, I'm not seeing the need here. I had just seen some > other code that used it, and copied the model. > > Unless someone comes along to say, yes, use it, I'll not. > I agree with you and Sohil on removing the unnecessary function calling. Thanks. Shunyong. > Gary
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Hi, Gary, On Wed, 2018-04-18 at 16:51 -0400, Hook, Gary wrote: > On 4/18/2018 4:16 PM, Mehta, Sohil wrote: > > > > On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote: > > > > > > Maybe the original design is to call debugfs_initialized() before > > > calling debugfs_create_xxx()? > > I am unaware of the original design. Someone else would probably > > have > > more context. However, looking at other places in the kernel where > > debugfs_create_xx() is used, the common convention seems to be to > > avoid > > calling debugfs_initialized(). > > > > Sohil > > > debugfs_initialized() was introduced in commit c0f92ba99 back in > 2.6.30-rc1. It was intended as a helper, not as a gatekeeper, which > is > why one doesn't see it used. Given that my use in this proposed patch > is > straightforward, I'm not seeing the need here. I had just seen some > other code that used it, and copied the model. > > Unless someone comes along to say, yes, use it, I'll not. > I agree with you and Sohil on removing the unnecessary function calling. Thanks. Shunyong. > Gary
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 4/18/2018 4:16 PM, Mehta, Sohil wrote: On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote: Maybe the original design is to call debugfs_initialized() before calling debugfs_create_xxx()? I am unaware of the original design. Someone else would probably have more context. However, looking at other places in the kernel where debugfs_create_xx() is used, the common convention seems to be to avoid calling debugfs_initialized(). Sohil debugfs_initialized() was introduced in commit c0f92ba99 back in 2.6.30-rc1. It was intended as a helper, not as a gatekeeper, which is why one doesn't see it used. Given that my use in this proposed patch is straightforward, I'm not seeing the need here. I had just seen some other code that used it, and copied the model. Unless someone comes along to say, yes, use it, I'll not. Gary
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 4/18/2018 4:16 PM, Mehta, Sohil wrote: On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote: Maybe the original design is to call debugfs_initialized() before calling debugfs_create_xxx()? I am unaware of the original design. Someone else would probably have more context. However, looking at other places in the kernel where debugfs_create_xx() is used, the common convention seems to be to avoid calling debugfs_initialized(). Sohil debugfs_initialized() was introduced in commit c0f92ba99 back in 2.6.30-rc1. It was intended as a helper, not as a gatekeeper, which is why one doesn't see it used. Given that my use in this proposed patch is straightforward, I'm not seeing the need here. I had just seen some other code that used it, and copied the model. Unless someone comes along to say, yes, use it, I'll not. Gary
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote: > Maybe the original design is to call debugfs_initialized() before > calling debugfs_create_xxx()? I am unaware of the original design. Someone else would probably have more context. However, looking at other places in the kernel where debugfs_create_xx() is used, the common convention seems to be to avoid calling debugfs_initialized(). Sohil
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Wed, 2018-04-18 at 08:31 +, Yang, Shunyong wrote: > Maybe the original design is to call debugfs_initialized() before > calling debugfs_create_xxx()? I am unaware of the original design. Someone else would probably have more context. However, looking at other places in the kernel where debugfs_create_xx() is used, the common convention seems to be to avoid calling debugfs_initialized(). Sohil
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Hi, Sohil On Wed, 2018-04-18 at 07:27 +, Mehta, Sohil wrote: > On Wed, 2018-04-18 at 05:58 +, Yang, Shunyong wrote: > > > > Hi, Gary and Sohil, > > > > On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote: > > > > > > On 4/13/2018 8:08 PM, Mehta, Sohil wrote: > > > > > > > > > > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > > > > > > > > > > > > > > > > > > + > > > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > > > > > +{ > > > > > + char name[MAX_NAME_LEN + 1]; > > > > > + struct dentry *d_top; > > > > > + > > > > > + if (!debugfs_initialized()) > > > > Probably not needed. > > > Right. > > When will this check is needed? > > IMO, this function is to check debugfs ready status before we want > > to > > use debugfs. I just want to understand when we should use > > debugfs_initialized(); > > > You are right debugfs_initialized() can be used to check if debugfs > is > ready. However in this case we can also rely on debugfs_create_dir() > which is called in iommu_debufs_setup(). > > debugfs_create_dir() says: > > * If debugfs is not enabled in the kernel, the value -%ENODEV will > be > * returned. It seems "If debugfs is not enabled in the kernel" means CONFIG_DEBUG_FS is not configured. Following is the code of no such config. static inline struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) { return ERR_PTR(-ENODEV); } Looking into the code, debugfs_initialized() return the value of debugfs_registered. debugfs_registered is set to true after debugfs_init() has been called. However, debugfs_create_dir() doesn't call debugfs_initialized() or check debugfs_registered value. So, there is tiny different of checking status by debugfs_create_dir() and debugfs_initialized(). Although it can achieve functionality here. Maybe the original design is to call debugfs_initialized() before calling debugfs_create_xxx()? Thanks. Shunyong. > > Sohil > > > > > Thanks. > > Shunyong. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + return; > > > > > + > > > > > + mutex_lock(_iommu_debugfs_lock); > > > > > + if (!amd_iommu_debugfs) { > > > > > + d_top = iommu_debugfs_setup(); > > > > > + if (d_top) > > > > > + amd_iommu_debugfs = > > > > > debugfs_create_dir("amd", d_top); > > > > > + } > > > > > + mutex_unlock(_iommu_debugfs_lock);
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Hi, Sohil On Wed, 2018-04-18 at 07:27 +, Mehta, Sohil wrote: > On Wed, 2018-04-18 at 05:58 +, Yang, Shunyong wrote: > > > > Hi, Gary and Sohil, > > > > On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote: > > > > > > On 4/13/2018 8:08 PM, Mehta, Sohil wrote: > > > > > > > > > > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > > > > > > > > > > > > > > > > > > + > > > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > > > > > +{ > > > > > + char name[MAX_NAME_LEN + 1]; > > > > > + struct dentry *d_top; > > > > > + > > > > > + if (!debugfs_initialized()) > > > > Probably not needed. > > > Right. > > When will this check is needed? > > IMO, this function is to check debugfs ready status before we want > > to > > use debugfs. I just want to understand when we should use > > debugfs_initialized(); > > > You are right debugfs_initialized() can be used to check if debugfs > is > ready. However in this case we can also rely on debugfs_create_dir() > which is called in iommu_debufs_setup(). > > debugfs_create_dir() says: > > * If debugfs is not enabled in the kernel, the value -%ENODEV will > be > * returned. It seems "If debugfs is not enabled in the kernel" means CONFIG_DEBUG_FS is not configured. Following is the code of no such config. static inline struct dentry *debugfs_create_dir(const char *name, struct dentry *parent) { return ERR_PTR(-ENODEV); } Looking into the code, debugfs_initialized() return the value of debugfs_registered. debugfs_registered is set to true after debugfs_init() has been called. However, debugfs_create_dir() doesn't call debugfs_initialized() or check debugfs_registered value. So, there is tiny different of checking status by debugfs_create_dir() and debugfs_initialized(). Although it can achieve functionality here. Maybe the original design is to call debugfs_initialized() before calling debugfs_create_xxx()? Thanks. Shunyong. > > Sohil > > > > > Thanks. > > Shunyong. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + return; > > > > > + > > > > > + mutex_lock(_iommu_debugfs_lock); > > > > > + if (!amd_iommu_debugfs) { > > > > > + d_top = iommu_debugfs_setup(); > > > > > + if (d_top) > > > > > + amd_iommu_debugfs = > > > > > debugfs_create_dir("amd", d_top); > > > > > + } > > > > > + mutex_unlock(_iommu_debugfs_lock);
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Wed, 2018-04-18 at 05:58 +, Yang, Shunyong wrote: > Hi, Gary and Sohil, > > On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote: > > On 4/13/2018 8:08 PM, Mehta, Sohil wrote: > > > > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > > > > > > > > > > + > > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > > > > +{ > > > > + char name[MAX_NAME_LEN + 1]; > > > > + struct dentry *d_top; > > > > + > > > > + if (!debugfs_initialized()) > > > Probably not needed. > > Right. > > When will this check is needed? > IMO, this function is to check debugfs ready status before we want to > use debugfs. I just want to understand when we should use > debugfs_initialized(); > You are right debugfs_initialized() can be used to check if debugfs is ready. However in this case we can also rely on debugfs_create_dir() which is called in iommu_debufs_setup(). debugfs_create_dir() says: * If debugfs is not enabled in the kernel, the value -%ENODEV will be * returned. Sohil > Thanks. > Shunyong. > > > > > > > > > > > > > > > > > + return; > > > > + > > > > + mutex_lock(_iommu_debugfs_lock); > > > > + if (!amd_iommu_debugfs) { > > > > + d_top = iommu_debugfs_setup(); > > > > + if (d_top) > > > > + amd_iommu_debugfs = > > > > debugfs_create_dir("amd", d_top); > > > > + } > > > > + mutex_unlock(_iommu_debugfs_lock);
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Wed, 2018-04-18 at 05:58 +, Yang, Shunyong wrote: > Hi, Gary and Sohil, > > On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote: > > On 4/13/2018 8:08 PM, Mehta, Sohil wrote: > > > > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > > > > > > > > > > + > > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > > > > +{ > > > > + char name[MAX_NAME_LEN + 1]; > > > > + struct dentry *d_top; > > > > + > > > > + if (!debugfs_initialized()) > > > Probably not needed. > > Right. > > When will this check is needed? > IMO, this function is to check debugfs ready status before we want to > use debugfs. I just want to understand when we should use > debugfs_initialized(); > You are right debugfs_initialized() can be used to check if debugfs is ready. However in this case we can also rely on debugfs_create_dir() which is called in iommu_debufs_setup(). debugfs_create_dir() says: * If debugfs is not enabled in the kernel, the value -%ENODEV will be * returned. Sohil > Thanks. > Shunyong. > > > > > > > > > > > > > > > > > + return; > > > > + > > > > + mutex_lock(_iommu_debugfs_lock); > > > > + if (!amd_iommu_debugfs) { > > > > + d_top = iommu_debugfs_setup(); > > > > + if (d_top) > > > > + amd_iommu_debugfs = > > > > debugfs_create_dir("amd", d_top); > > > > + } > > > > + mutex_unlock(_iommu_debugfs_lock);
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Hi, Gary and Sohil, On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote: > On 4/13/2018 8:08 PM, Mehta, Sohil wrote: > > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > > > > > > > + > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > > > +{ > > > + char name[MAX_NAME_LEN + 1]; > > > + struct dentry *d_top; > > > + > > > + if (!debugfs_initialized()) > > Probably not needed. > Right. When will this check is needed? IMO, this function is to check debugfs ready status before we want to use debugfs. I just want to understand when we should use debugfs_initialized(); Thanks. Shunyong. > > > > > > > > > > > + return; > > > + > > > + mutex_lock(_iommu_debugfs_lock); > > > + if (!amd_iommu_debugfs) { > > > + d_top = iommu_debugfs_setup(); > > > + if (d_top) > > > + amd_iommu_debugfs = > > > debugfs_create_dir("amd", d_top); > > > + } > > > + mutex_unlock(_iommu_debugfs_lock); > > > > You can do the above only once if you iterate over the IOMMUs here > > instead of doing it in amd_iommu_init. > I'm not sure it matters, given the finite number of IOMMUs in a > system, > and the fact that this work is done exactly once. However, removal of > a > lock is fine thing, so I'll move this around. > > > > > > > > > > > + if (amd_iommu_debugfs) { > > > + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- > > > > > > > > index); > > > + iommu->debugfs = debugfs_create_dir(name, > > > + amd_iommu_de > > > bugf > > > s); > > > + if (!iommu->debugfs) { > > > + debugfs_remove_recursive(amd_iommu_debug > > > fs); > > > + amd_iommu_debugfs = NULL; > > > + } > > > + } > > > +} > > -Sohil > > > ___ > iommu mailing list > io...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Hi, Gary and Sohil, On Tue, 2018-04-17 at 13:38 -0400, Hook, Gary wrote: > On 4/13/2018 8:08 PM, Mehta, Sohil wrote: > > > > On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > > > > > > > > + > > > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > > > +{ > > > + char name[MAX_NAME_LEN + 1]; > > > + struct dentry *d_top; > > > + > > > + if (!debugfs_initialized()) > > Probably not needed. > Right. When will this check is needed? IMO, this function is to check debugfs ready status before we want to use debugfs. I just want to understand when we should use debugfs_initialized(); Thanks. Shunyong. > > > > > > > > > > > + return; > > > + > > > + mutex_lock(_iommu_debugfs_lock); > > > + if (!amd_iommu_debugfs) { > > > + d_top = iommu_debugfs_setup(); > > > + if (d_top) > > > + amd_iommu_debugfs = > > > debugfs_create_dir("amd", d_top); > > > + } > > > + mutex_unlock(_iommu_debugfs_lock); > > > > You can do the above only once if you iterate over the IOMMUs here > > instead of doing it in amd_iommu_init. > I'm not sure it matters, given the finite number of IOMMUs in a > system, > and the fact that this work is done exactly once. However, removal of > a > lock is fine thing, so I'll move this around. > > > > > > > > > > > + if (amd_iommu_debugfs) { > > > + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- > > > > > > > > index); > > > + iommu->debugfs = debugfs_create_dir(name, > > > + amd_iommu_de > > > bugf > > > s); > > > + if (!iommu->debugfs) { > > > + debugfs_remove_recursive(amd_iommu_debug > > > fs); > > > + amd_iommu_debugfs = NULL; > > > + } > > > + } > > > +} > > -Sohil > > > ___ > iommu mailing list > io...@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 17/04/18 18:42, Hook, Gary wrote: On 4/16/2018 8:52 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 5eb1121d54b9..0ca250f626d9 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o Compiling amd_iommu_debugfs.c seems to depend only on CONFIG_IOMMU_DEBUG. Can we prevent the code within amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU or CONFIG_IOMMU_DEBUG is disabled? That's a good point. My intention was that only one switch was required to incorporate any DebugFS support, but I see now that I didn't consider all of the cases. It appears that a per-device switch is also necessary. Unless someone has a better idea. Well, you could do a makefile-level dependency i.e.: ifeq ($(CONFIG_IOMMU_DEBUG), y) obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o obj-$(CONFIG_BLAH_IOMMU) += blah_iommu_debugfs.o ... endif Or alternatively have an intermediate silent Kconfig option: config AMD_IOMMU_DEBUG def_bool y depends on AMD_IOMMU && IOMMU_DEBUG The makefile option is arguably ugly, but does at least scale better ;) Robin. obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm-smmu.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o ___ iommu mailing list io...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 17/04/18 18:42, Hook, Gary wrote: On 4/16/2018 8:52 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 5eb1121d54b9..0ca250f626d9 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o Compiling amd_iommu_debugfs.c seems to depend only on CONFIG_IOMMU_DEBUG. Can we prevent the code within amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU or CONFIG_IOMMU_DEBUG is disabled? That's a good point. My intention was that only one switch was required to incorporate any DebugFS support, but I see now that I didn't consider all of the cases. It appears that a per-device switch is also necessary. Unless someone has a better idea. Well, you could do a makefile-level dependency i.e.: ifeq ($(CONFIG_IOMMU_DEBUG), y) obj-$(CONFIG_AMD_IOMMU) += amd_iommu_debugfs.o obj-$(CONFIG_BLAH_IOMMU) += blah_iommu_debugfs.o ... endif Or alternatively have an intermediate silent Kconfig option: config AMD_IOMMU_DEBUG def_bool y depends on AMD_IOMMU && IOMMU_DEBUG The makefile option is arguably ugly, but does at least scale better ;) Robin. obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm-smmu.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o ___ iommu mailing list io...@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 4/16/2018 8:52 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 5eb1121d54b9..0ca250f626d9 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o Compiling amd_iommu_debugfs.c seems to depend only on CONFIG_IOMMU_DEBUG. Can we prevent the code within amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU or CONFIG_IOMMU_DEBUG is disabled? That's a good point. My intention was that only one switch was required to incorporate any DebugFS support, but I see now that I didn't consider all of the cases. It appears that a per-device switch is also necessary. Unless someone has a better idea. obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm-smmu.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 4/16/2018 8:52 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 5eb1121d54b9..0ca250f626d9 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o Compiling amd_iommu_debugfs.c seems to depend only on CONFIG_IOMMU_DEBUG. Can we prevent the code within amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU or CONFIG_IOMMU_DEBUG is disabled? That's a good point. My intention was that only one switch was required to incorporate any DebugFS support, but I see now that I didn't consider all of the cases. It appears that a per-device switch is also necessary. Unless someone has a better idea. obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm-smmu.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 4/13/2018 8:08 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: + +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) +{ + char name[MAX_NAME_LEN + 1]; + struct dentry *d_top; + + if (!debugfs_initialized()) Probably not needed. Right. + return; + + mutex_lock(_iommu_debugfs_lock); + if (!amd_iommu_debugfs) { + d_top = iommu_debugfs_setup(); + if (d_top) + amd_iommu_debugfs = debugfs_create_dir("amd", d_top); + } + mutex_unlock(_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. I'm not sure it matters, given the finite number of IOMMUs in a system, and the fact that this work is done exactly once. However, removal of a lock is fine thing, so I'll move this around. + if (amd_iommu_debugfs) { + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- index); + iommu->debugfs = debugfs_create_dir(name, + amd_iommu_debugf s); + if (!iommu->debugfs) { + debugfs_remove_recursive(amd_iommu_debugfs); + amd_iommu_debugfs = NULL; + } + } +} -Sohil
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On 4/13/2018 8:08 PM, Mehta, Sohil wrote: On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: + +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) +{ + char name[MAX_NAME_LEN + 1]; + struct dentry *d_top; + + if (!debugfs_initialized()) Probably not needed. Right. + return; + + mutex_lock(_iommu_debugfs_lock); + if (!amd_iommu_debugfs) { + d_top = iommu_debugfs_setup(); + if (d_top) + amd_iommu_debugfs = debugfs_create_dir("amd", d_top); + } + mutex_unlock(_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. I'm not sure it matters, given the finite number of IOMMUs in a system, and the fact that this work is done exactly once. However, removal of a lock is fine thing, so I'll move this around. + if (amd_iommu_debugfs) { + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- index); + iommu->debugfs = debugfs_create_dir(name, + amd_iommu_debugf s); + if (!iommu->debugfs) { + debugfs_remove_recursive(amd_iommu_debugfs); + amd_iommu_debugfs = NULL; + } + } +} -Sohil
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > index 5eb1121d54b9..0ca250f626d9 100644 > --- a/drivers/iommu/Makefile > +++ b/drivers/iommu/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o > obj-$(CONFIG_OF_IOMMU) += of_iommu.o > obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o > obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o > +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o Compiling amd_iommu_debugfs.c seems to depend only on CONFIG_IOMMU_DEBUG. Can we prevent the code within amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU or CONFIG_IOMMU_DEBUG is disabled? > obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o > obj-$(CONFIG_ARM_SMMU) += arm-smmu.o > obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile > index 5eb1121d54b9..0ca250f626d9 100644 > --- a/drivers/iommu/Makefile > +++ b/drivers/iommu/Makefile > @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o > obj-$(CONFIG_OF_IOMMU) += of_iommu.o > obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o > obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o > +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o Compiling amd_iommu_debugfs.c seems to depend only on CONFIG_IOMMU_DEBUG. Can we prevent the code within amd_iommu_debugfs.c from getting compiled when either CONFIG_AMD_IOMMU or CONFIG_IOMMU_DEBUG is disabled? > obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o > obj-$(CONFIG_ARM_SMMU) += arm-smmu.o > obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > + > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > +{ > + char name[MAX_NAME_LEN + 1]; > + struct dentry *d_top; > + > + if (!debugfs_initialized()) Probably not needed. > + return; > + > + mutex_lock(_iommu_debugfs_lock); > + if (!amd_iommu_debugfs) { > + d_top = iommu_debugfs_setup(); > + if (d_top) > + amd_iommu_debugfs = > debugfs_create_dir("amd", d_top); > + } > + mutex_unlock(_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. > + if (amd_iommu_debugfs) { > + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- > >index); > + iommu->debugfs = debugfs_create_dir(name, > + amd_iommu_debugf > s); > + if (!iommu->debugfs) { > + debugfs_remove_recursive(amd_iommu_debugfs); > + amd_iommu_debugfs = NULL; > + } > + } > +} -Sohil
Re: [PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
On Fri, 2018-04-06 at 08:17 -0500, Gary R Hook wrote: > > + > +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) > +{ > + char name[MAX_NAME_LEN + 1]; > + struct dentry *d_top; > + > + if (!debugfs_initialized()) Probably not needed. > + return; > + > + mutex_lock(_iommu_debugfs_lock); > + if (!amd_iommu_debugfs) { > + d_top = iommu_debugfs_setup(); > + if (d_top) > + amd_iommu_debugfs = > debugfs_create_dir("amd", d_top); > + } > + mutex_unlock(_iommu_debugfs_lock); You can do the above only once if you iterate over the IOMMUs here instead of doing it in amd_iommu_init. > + if (amd_iommu_debugfs) { > + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu- > >index); > + iommu->debugfs = debugfs_create_dir(name, > + amd_iommu_debugf > s); > + if (!iommu->debugfs) { > + debugfs_remove_recursive(amd_iommu_debugfs); > + amd_iommu_debugfs = NULL; > + } > + } > +} -Sohil
[PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Implement a skeleton framework for debugfs support in the AMD IOMMU. Signed-off-by: Gary R Hook--- drivers/iommu/Makefile|1 + drivers/iommu/amd_iommu_debugfs.c | 45 + drivers/iommu/amd_iommu_init.c|6 +++-- drivers/iommu/amd_iommu_proto.h |6 + drivers/iommu/amd_iommu_types.h |3 ++ 5 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 drivers/iommu/amd_iommu_debugfs.c diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 5eb1121d54b9..0ca250f626d9 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm-smmu.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c new file mode 100644 index ..282100a655b3 --- /dev/null +++ b/drivers/iommu/amd_iommu_debugfs.c @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * AMD IOMMU driver + * + * Copyright (C) 2018 Advanced Micro Devices, Inc. + * + * Author: Gary R Hook + */ + +#include +#include +#include +#include "amd_iommu_proto.h" +#include "amd_iommu_types.h" + +static struct dentry *amd_iommu_debugfs; +static DEFINE_MUTEX(amd_iommu_debugfs_lock); + +#defineMAX_NAME_LEN20 + +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) +{ + char name[MAX_NAME_LEN + 1]; + struct dentry *d_top; + + if (!debugfs_initialized()) + return; + + mutex_lock(_iommu_debugfs_lock); + if (!amd_iommu_debugfs) { + d_top = iommu_debugfs_setup(); + if (d_top) + amd_iommu_debugfs = debugfs_create_dir("amd", d_top); + } + mutex_unlock(_iommu_debugfs_lock); + if (amd_iommu_debugfs) { + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index); + iommu->debugfs = debugfs_create_dir(name, + amd_iommu_debugfs); + if (!iommu->debugfs) { + debugfs_remove_recursive(amd_iommu_debugfs); + amd_iommu_debugfs = NULL; + } + } +} diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c index 904c575d1677..031e6dbb8345 100644 --- a/drivers/iommu/amd_iommu_init.c +++ b/drivers/iommu/amd_iommu_init.c @@ -2721,6 +2721,7 @@ int __init amd_iommu_enable_faulting(void) */ static int __init amd_iommu_init(void) { + struct amd_iommu *iommu; int ret; ret = iommu_go_to_state(IOMMU_INITIALIZED); @@ -2730,14 +2731,15 @@ static int __init amd_iommu_init(void) disable_iommus(); free_iommu_resources(); } else { - struct amd_iommu *iommu; - uninit_device_table_dma(); for_each_iommu(iommu) iommu_flush_all_caches(iommu); } } + for_each_iommu(iommu) + amd_iommu_debugfs_setup(iommu); + return ret; } diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h index 640c286a0ab9..daf7f38531f9 100644 --- a/drivers/iommu/amd_iommu_proto.h +++ b/drivers/iommu/amd_iommu_proto.h @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void); extern void amd_iommu_init_notifier(void); extern int amd_iommu_init_api(void); +#ifdef CONFIG_IOMMU_DEBUG +void amd_iommu_debugfs_setup(struct amd_iommu *iommu); +#else +static inline void amd_iommu_debugfs_setup(struct amd_iommu *iommu) {} +#endif + /* Needed for interrupt remapping */ extern int amd_iommu_prepare(void); extern int amd_iommu_enable(void); diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h index 1c9b080276c9..2ca0959ae9e6 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -593,6 +593,9 @@ struct amd_iommu { u32 flags; volatile u64 __aligned(8) cmd_sem; + + /* DebugFS Info */ + struct dentry *debugfs; }; static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev)
[PATCH v3 2/2] iommu/amd: Add basic debugfs infrastructure for AMD IOMMU
Implement a skeleton framework for debugfs support in the AMD IOMMU. Signed-off-by: Gary R Hook --- drivers/iommu/Makefile|1 + drivers/iommu/amd_iommu_debugfs.c | 45 + drivers/iommu/amd_iommu_init.c|6 +++-- drivers/iommu/amd_iommu_proto.h |6 + drivers/iommu/amd_iommu_types.h |3 ++ 5 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 drivers/iommu/amd_iommu_debugfs.c diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile index 5eb1121d54b9..0ca250f626d9 100644 --- a/drivers/iommu/Makefile +++ b/drivers/iommu/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_IOMMU_IOVA) += iova.o obj-$(CONFIG_OF_IOMMU) += of_iommu.o obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o +obj-$(CONFIG_IOMMU_DEBUG) += amd_iommu_debugfs.o obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o obj-$(CONFIG_ARM_SMMU) += arm-smmu.o obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o diff --git a/drivers/iommu/amd_iommu_debugfs.c b/drivers/iommu/amd_iommu_debugfs.c new file mode 100644 index ..282100a655b3 --- /dev/null +++ b/drivers/iommu/amd_iommu_debugfs.c @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * AMD IOMMU driver + * + * Copyright (C) 2018 Advanced Micro Devices, Inc. + * + * Author: Gary R Hook + */ + +#include +#include +#include +#include "amd_iommu_proto.h" +#include "amd_iommu_types.h" + +static struct dentry *amd_iommu_debugfs; +static DEFINE_MUTEX(amd_iommu_debugfs_lock); + +#defineMAX_NAME_LEN20 + +void amd_iommu_debugfs_setup(struct amd_iommu *iommu) +{ + char name[MAX_NAME_LEN + 1]; + struct dentry *d_top; + + if (!debugfs_initialized()) + return; + + mutex_lock(_iommu_debugfs_lock); + if (!amd_iommu_debugfs) { + d_top = iommu_debugfs_setup(); + if (d_top) + amd_iommu_debugfs = debugfs_create_dir("amd", d_top); + } + mutex_unlock(_iommu_debugfs_lock); + if (amd_iommu_debugfs) { + snprintf(name, MAX_NAME_LEN, "iommu%02d", iommu->index); + iommu->debugfs = debugfs_create_dir(name, + amd_iommu_debugfs); + if (!iommu->debugfs) { + debugfs_remove_recursive(amd_iommu_debugfs); + amd_iommu_debugfs = NULL; + } + } +} diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c index 904c575d1677..031e6dbb8345 100644 --- a/drivers/iommu/amd_iommu_init.c +++ b/drivers/iommu/amd_iommu_init.c @@ -2721,6 +2721,7 @@ int __init amd_iommu_enable_faulting(void) */ static int __init amd_iommu_init(void) { + struct amd_iommu *iommu; int ret; ret = iommu_go_to_state(IOMMU_INITIALIZED); @@ -2730,14 +2731,15 @@ static int __init amd_iommu_init(void) disable_iommus(); free_iommu_resources(); } else { - struct amd_iommu *iommu; - uninit_device_table_dma(); for_each_iommu(iommu) iommu_flush_all_caches(iommu); } } + for_each_iommu(iommu) + amd_iommu_debugfs_setup(iommu); + return ret; } diff --git a/drivers/iommu/amd_iommu_proto.h b/drivers/iommu/amd_iommu_proto.h index 640c286a0ab9..daf7f38531f9 100644 --- a/drivers/iommu/amd_iommu_proto.h +++ b/drivers/iommu/amd_iommu_proto.h @@ -33,6 +33,12 @@ extern void amd_iommu_uninit_devices(void); extern void amd_iommu_init_notifier(void); extern int amd_iommu_init_api(void); +#ifdef CONFIG_IOMMU_DEBUG +void amd_iommu_debugfs_setup(struct amd_iommu *iommu); +#else +static inline void amd_iommu_debugfs_setup(struct amd_iommu *iommu) {} +#endif + /* Needed for interrupt remapping */ extern int amd_iommu_prepare(void); extern int amd_iommu_enable(void); diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h index 1c9b080276c9..2ca0959ae9e6 100644 --- a/drivers/iommu/amd_iommu_types.h +++ b/drivers/iommu/amd_iommu_types.h @@ -593,6 +593,9 @@ struct amd_iommu { u32 flags; volatile u64 __aligned(8) cmd_sem; + + /* DebugFS Info */ + struct dentry *debugfs; }; static inline struct amd_iommu *dev_to_amd_iommu(struct device *dev)