Re: [PATCH 2/2] driver core: Silence device links sphinx warning
On Sun, Dec 04, 2016 at 01:10:04PM +0100, Lukas Wunner wrote: > Silence this warning emitted by sphinx: > include/linux/device.h:938: warning: No description found for parameter > 'links' > > While at it, fix typos in comments of device links code. > > Cc: Rafael J. Wysocki > Cc: Greg Kroah-Hartman > Cc: Jonathan Corbet > Cc: Silvio Fricke > Signed-off-by: Lukas Wunner > --- > drivers/base/core.c| 4 ++-- > include/linux/device.h | 1 + > 2 files changed, 3 insertions(+), 2 deletions(-) I'll take this patch now to keep the warnings from showing up in 4.10-rc1. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/9] Move dma_ops from archdata into struct device
On Tue, Jan 10, 2017 at 04:56:41PM -0800, Bart Van Assche wrote:
> Several RDMA drivers, e.g. drivers/infiniband/hw/qib, use the CPU to
> transfer data between memory and PCIe adapter. Because of performance
> reasons it is important that the CPU cache is not flushed when such
> drivers transfer data. Make this possible by allowing these drivers to
> override the dma_map_ops pointer. Additionally, introduce the function
> set_dma_ops() that will be used by a later patch in this series.
>
> Signed-off-by: Bart Van Assche
> Cc: Greg Kroah-Hartman
> Cc: Aurelien Jacquiot
> Cc: Catalin Marinas
> Cc: Chris Zankel
> Cc: David Howells
> Cc: David S. Miller
> Cc: Fenghua Yu
> Cc: Geert Uytterhoeven
> Cc: Geoff Levand
> Cc: H. Peter Anvin
> Cc: Haavard Skinnemoen
> Cc: Hans-Christian Egtvedt
> Cc: Helge Deller
> Cc: Ingo Molnar
> Cc: James E.J. Bottomley
> Cc: Jesper Nilsson
> Cc: Joerg Roedel
> Cc: Jon Mason
> Cc: Jonas Bonn
> Cc: Ley Foon Tan
> Cc: Mark Salter
> Cc: Max Filippov
> Cc: Mikael Starvik
> Cc: Muli Ben-Yehuda
> Cc: Rich Felker
> Cc: Russell King
> Cc: Stafford Horne
> Cc: Stefan Kristiansson
> Cc: Thomas Gleixner
> Cc: Tony Luck
> Cc: Will Deacon
> Cc: x...@kernel.org
> Cc: Yoshinori Sato
> Cc: adi-buildroot-de...@lists.sourceforge.net
> Cc: iommu@lists.linux-foundation.org
> Cc: linux-al...@vger.kernel.org
> Cc: linux-am33-l...@redhat.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-c6x-...@linux-c6x.org
> Cc: linux-cris-ker...@axis.com
> Cc: linux-hexa...@vger.kernel.org
> Cc: linux-i...@vger.kernel.org
> Cc: linux-m...@lists.linux-m68k.org
> Cc: linux-me...@vger.kernel.org
> Cc: linux-m...@linux-mips.org
> Cc: linux-par...@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Cc: linux-r...@vger.kernel.org
> Cc: linux-s...@vger.kernel.org
> Cc: linux...@vger.kernel.org
> Cc: linux-snps-...@lists.infradead.org
> Cc: linux-xte...@linux-xtensa.org
> Cc: linuxppc-...@lists.ozlabs.org
> Cc: nios2-...@lists.rocketboards.org
> Cc: openr...@lists.librecores.org
> Cc: sparcli...@vger.kernel.org
> Cc: uclinux-h8-de...@lists.sourceforge.jp
That's a crazy cc: list, you should break this up into smaller pieces,
otherwise it's going to bounce...
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 491b4c0ca633..c7cb225d36b0 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -885,6 +885,8 @@ struct dev_links_info {
> * a higher-level representation of the device.
> */
> struct device {
> + const struct dma_map_ops *dma_ops; /* See also get_dma_ops() */
> +
> struct device *parent;
>
> struct device_private *p;
Why not put this new pointer down with the other dma fields in this
structure? Any specific reason it needs to be first?
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/9] Move dma_ops from archdata into struct device
On Tue, Jan 10, 2017 at 04:56:41PM -0800, Bart Van Assche wrote: > Several RDMA drivers, e.g. drivers/infiniband/hw/qib, use the CPU to > transfer data between memory and PCIe adapter. Because of performance > reasons it is important that the CPU cache is not flushed when such > drivers transfer data. Make this possible by allowing these drivers to > override the dma_map_ops pointer. Additionally, introduce the function > set_dma_ops() that will be used by a later patch in this series. When you say things like "additionally", that's a huge flag that this needs to be split up into multiple patches. No need to add set_dma_ops() here in this patch. And I'd argue that it should be dma_ops_set(), and dma_ops_get(), just to keep the namespace sane, but that's probably a different set of patches... thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.4 26/91] iommu/vt-d: Fix some macros that are incorrectly specified in intel-iommu
4.4-stable review patch. If anyone has any objections, please let me know.
--
From: CQ Tang
commit aaa59306b0b7e0ca4ba92cc04c5db101cbb1c096 upstream.
Some of the macros are incorrect with wrong bit-shifts resulting in picking
the incorrect invalidation granularity. Incorrect Source-ID in extended
devtlb invalidation caused device side errors.
To: Joerg Roedel
To: David Woodhouse
Cc: iommu@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: CQ Tang
Cc: Ashok Raj
Fixes: 2f26e0a9 ("iommu/vt-d: Add basic SVM PASID support")
Signed-off-by: CQ Tang
Signed-off-by: Ashok Raj
Tested-by: CQ Tang
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
include/linux/intel-iommu.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -153,8 +153,8 @@ static inline void dmar_writeq(void __io
#define DMA_TLB_GLOBAL_FLUSH (((u64)1) << 60)
#define DMA_TLB_DSI_FLUSH (((u64)2) << 60)
#define DMA_TLB_PSI_FLUSH (((u64)3) << 60)
-#define DMA_TLB_IIRG(type) ((type >> 60) & 7)
-#define DMA_TLB_IAIG(val) (((val) >> 57) & 7)
+#define DMA_TLB_IIRG(type) ((type >> 60) & 3)
+#define DMA_TLB_IAIG(val) (((val) >> 57) & 3)
#define DMA_TLB_READ_DRAIN (((u64)1) << 49)
#define DMA_TLB_WRITE_DRAIN (((u64)1) << 48)
#define DMA_TLB_DID(id)(((u64)((id) & 0x)) << 32)
@@ -164,9 +164,9 @@ static inline void dmar_writeq(void __io
/* INVALID_DESC */
#define DMA_CCMD_INVL_GRANU_OFFSET 61
-#define DMA_ID_TLB_GLOBAL_FLUSH(((u64)1) << 3)
-#define DMA_ID_TLB_DSI_FLUSH (((u64)2) << 3)
-#define DMA_ID_TLB_PSI_FLUSH (((u64)3) << 3)
+#define DMA_ID_TLB_GLOBAL_FLUSH(((u64)1) << 4)
+#define DMA_ID_TLB_DSI_FLUSH (((u64)2) << 4)
+#define DMA_ID_TLB_PSI_FLUSH (((u64)3) << 4)
#define DMA_ID_TLB_READ_DRAIN (((u64)1) << 7)
#define DMA_ID_TLB_WRITE_DRAIN (((u64)1) << 6)
#define DMA_ID_TLB_DID(id) (((u64)((id & 0x) << 16)))
@@ -316,8 +316,8 @@ enum {
#define QI_DEV_EIOTLB_SIZE (((u64)1) << 11)
#define QI_DEV_EIOTLB_GLOB(g) ((u64)g)
#define QI_DEV_EIOTLB_PASID(p) (((u64)p) << 32)
-#define QI_DEV_EIOTLB_SID(sid) ((u64)((sid) & 0x) << 32)
-#define QI_DEV_EIOTLB_QDEP(qd) (((qd) & 0x1f) << 16)
+#define QI_DEV_EIOTLB_SID(sid) ((u64)((sid) & 0x) << 16)
+#define QI_DEV_EIOTLB_QDEP(qd) ((u64)((qd) & 0x1f) << 4)
#define QI_DEV_EIOTLB_MAX_INVS 32
#define QI_PGRP_IDX(idx) (((u64)(idx)) << 55)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.4 27/91] iommu/vt-d: Tylersburg isoch identity map check is done too late.
4.4-stable review patch. If anyone has any objections, please let me know.
--
From: Ashok Raj
commit 21e722c4c8377b5bc82ad058fed12165af739c1b upstream.
The check to set identity map for tylersburg is done too late. It needs
to be done before the check for identity_map domain is done.
To: Joerg Roedel
To: David Woodhouse
Cc: iommu@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: Ashok Raj
Fixes: 86080ccc22 ("iommu/vt-d: Allocate si_domain in init_dmars()")
Signed-off-by: Ashok Raj
Reported-by: Yunhong Jiang
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3238,13 +3238,14 @@ static int __init init_dmars(void)
iommu_identity_mapping |= IDENTMAP_GFX;
#endif
+ check_tylersburg_isoch();
+
if (iommu_identity_mapping) {
ret = si_domain_init(hw_pass_through);
if (ret)
goto free_iommu;
}
- check_tylersburg_isoch();
/*
* If we copied translations from a previous kernel in the kdump
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.9 039/153] iommu/vt-d: Fix some macros that are incorrectly specified in intel-iommu
4.9-stable review patch. If anyone has any objections, please let me know.
--
From: CQ Tang
commit aaa59306b0b7e0ca4ba92cc04c5db101cbb1c096 upstream.
Some of the macros are incorrect with wrong bit-shifts resulting in picking
the incorrect invalidation granularity. Incorrect Source-ID in extended
devtlb invalidation caused device side errors.
To: Joerg Roedel
To: David Woodhouse
Cc: iommu@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: CQ Tang
Cc: Ashok Raj
Fixes: 2f26e0a9 ("iommu/vt-d: Add basic SVM PASID support")
Signed-off-by: CQ Tang
Signed-off-by: Ashok Raj
Tested-by: CQ Tang
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
include/linux/intel-iommu.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -153,8 +153,8 @@ static inline void dmar_writeq(void __io
#define DMA_TLB_GLOBAL_FLUSH (((u64)1) << 60)
#define DMA_TLB_DSI_FLUSH (((u64)2) << 60)
#define DMA_TLB_PSI_FLUSH (((u64)3) << 60)
-#define DMA_TLB_IIRG(type) ((type >> 60) & 7)
-#define DMA_TLB_IAIG(val) (((val) >> 57) & 7)
+#define DMA_TLB_IIRG(type) ((type >> 60) & 3)
+#define DMA_TLB_IAIG(val) (((val) >> 57) & 3)
#define DMA_TLB_READ_DRAIN (((u64)1) << 49)
#define DMA_TLB_WRITE_DRAIN (((u64)1) << 48)
#define DMA_TLB_DID(id)(((u64)((id) & 0x)) << 32)
@@ -164,9 +164,9 @@ static inline void dmar_writeq(void __io
/* INVALID_DESC */
#define DMA_CCMD_INVL_GRANU_OFFSET 61
-#define DMA_ID_TLB_GLOBAL_FLUSH(((u64)1) << 3)
-#define DMA_ID_TLB_DSI_FLUSH (((u64)2) << 3)
-#define DMA_ID_TLB_PSI_FLUSH (((u64)3) << 3)
+#define DMA_ID_TLB_GLOBAL_FLUSH(((u64)1) << 4)
+#define DMA_ID_TLB_DSI_FLUSH (((u64)2) << 4)
+#define DMA_ID_TLB_PSI_FLUSH (((u64)3) << 4)
#define DMA_ID_TLB_READ_DRAIN (((u64)1) << 7)
#define DMA_ID_TLB_WRITE_DRAIN (((u64)1) << 6)
#define DMA_ID_TLB_DID(id) (((u64)((id & 0x) << 16)))
@@ -316,8 +316,8 @@ enum {
#define QI_DEV_EIOTLB_SIZE (((u64)1) << 11)
#define QI_DEV_EIOTLB_GLOB(g) ((u64)g)
#define QI_DEV_EIOTLB_PASID(p) (((u64)p) << 32)
-#define QI_DEV_EIOTLB_SID(sid) ((u64)((sid) & 0x) << 32)
-#define QI_DEV_EIOTLB_QDEP(qd) (((qd) & 0x1f) << 16)
+#define QI_DEV_EIOTLB_SID(sid) ((u64)((sid) & 0x) << 16)
+#define QI_DEV_EIOTLB_QDEP(qd) ((u64)((qd) & 0x1f) << 4)
#define QI_DEV_EIOTLB_MAX_INVS 32
#define QI_PGRP_IDX(idx) (((u64)(idx)) << 55)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.9 040/153] iommu/vt-d: Tylersburg isoch identity map check is done too late.
4.9-stable review patch. If anyone has any objections, please let me know.
--
From: Ashok Raj
commit 21e722c4c8377b5bc82ad058fed12165af739c1b upstream.
The check to set identity map for tylersburg is done too late. It needs
to be done before the check for identity_map domain is done.
To: Joerg Roedel
To: David Woodhouse
Cc: iommu@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: Ashok Raj
Fixes: 86080ccc22 ("iommu/vt-d: Allocate si_domain in init_dmars()")
Signed-off-by: Ashok Raj
Reported-by: Yunhong Jiang
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3325,13 +3325,14 @@ static int __init init_dmars(void)
iommu_identity_mapping |= IDENTMAP_GFX;
#endif
+ check_tylersburg_isoch();
+
if (iommu_identity_mapping) {
ret = si_domain_init(hw_pass_through);
if (ret)
goto free_iommu;
}
- check_tylersburg_isoch();
/*
* If we copied translations from a previous kernel in the kdump
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.10 042/167] iommu/vt-d: Fix some macros that are incorrectly specified in intel-iommu
4.10-stable review patch. If anyone has any objections, please let me know.
--
From: CQ Tang
commit aaa59306b0b7e0ca4ba92cc04c5db101cbb1c096 upstream.
Some of the macros are incorrect with wrong bit-shifts resulting in picking
the incorrect invalidation granularity. Incorrect Source-ID in extended
devtlb invalidation caused device side errors.
To: Joerg Roedel
To: David Woodhouse
Cc: iommu@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: CQ Tang
Cc: Ashok Raj
Fixes: 2f26e0a9 ("iommu/vt-d: Add basic SVM PASID support")
Signed-off-by: CQ Tang
Signed-off-by: Ashok Raj
Tested-by: CQ Tang
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
include/linux/intel-iommu.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -153,8 +153,8 @@ static inline void dmar_writeq(void __io
#define DMA_TLB_GLOBAL_FLUSH (((u64)1) << 60)
#define DMA_TLB_DSI_FLUSH (((u64)2) << 60)
#define DMA_TLB_PSI_FLUSH (((u64)3) << 60)
-#define DMA_TLB_IIRG(type) ((type >> 60) & 7)
-#define DMA_TLB_IAIG(val) (((val) >> 57) & 7)
+#define DMA_TLB_IIRG(type) ((type >> 60) & 3)
+#define DMA_TLB_IAIG(val) (((val) >> 57) & 3)
#define DMA_TLB_READ_DRAIN (((u64)1) << 49)
#define DMA_TLB_WRITE_DRAIN (((u64)1) << 48)
#define DMA_TLB_DID(id)(((u64)((id) & 0x)) << 32)
@@ -164,9 +164,9 @@ static inline void dmar_writeq(void __io
/* INVALID_DESC */
#define DMA_CCMD_INVL_GRANU_OFFSET 61
-#define DMA_ID_TLB_GLOBAL_FLUSH(((u64)1) << 3)
-#define DMA_ID_TLB_DSI_FLUSH (((u64)2) << 3)
-#define DMA_ID_TLB_PSI_FLUSH (((u64)3) << 3)
+#define DMA_ID_TLB_GLOBAL_FLUSH(((u64)1) << 4)
+#define DMA_ID_TLB_DSI_FLUSH (((u64)2) << 4)
+#define DMA_ID_TLB_PSI_FLUSH (((u64)3) << 4)
#define DMA_ID_TLB_READ_DRAIN (((u64)1) << 7)
#define DMA_ID_TLB_WRITE_DRAIN (((u64)1) << 6)
#define DMA_ID_TLB_DID(id) (((u64)((id & 0x) << 16)))
@@ -316,8 +316,8 @@ enum {
#define QI_DEV_EIOTLB_SIZE (((u64)1) << 11)
#define QI_DEV_EIOTLB_GLOB(g) ((u64)g)
#define QI_DEV_EIOTLB_PASID(p) (((u64)p) << 32)
-#define QI_DEV_EIOTLB_SID(sid) ((u64)((sid) & 0x) << 32)
-#define QI_DEV_EIOTLB_QDEP(qd) (((qd) & 0x1f) << 16)
+#define QI_DEV_EIOTLB_SID(sid) ((u64)((sid) & 0x) << 16)
+#define QI_DEV_EIOTLB_QDEP(qd) ((u64)((qd) & 0x1f) << 4)
#define QI_DEV_EIOTLB_MAX_INVS 32
#define QI_PGRP_IDX(idx) (((u64)(idx)) << 55)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.10 043/167] iommu/vt-d: Tylersburg isoch identity map check is done too late.
4.10-stable review patch. If anyone has any objections, please let me know.
--
From: Ashok Raj
commit 21e722c4c8377b5bc82ad058fed12165af739c1b upstream.
The check to set identity map for tylersburg is done too late. It needs
to be done before the check for identity_map domain is done.
To: Joerg Roedel
To: David Woodhouse
Cc: iommu@lists.linux-foundation.org
Cc: linux-ker...@vger.kernel.org
Cc: Ashok Raj
Fixes: 86080ccc22 ("iommu/vt-d: Allocate si_domain in init_dmars()")
Signed-off-by: Ashok Raj
Reported-by: Yunhong Jiang
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3325,13 +3325,14 @@ static int __init init_dmars(void)
iommu_identity_mapping |= IDENTMAP_GFX;
#endif
+ check_tylersburg_isoch();
+
if (iommu_identity_mapping) {
ret = si_domain_init(hw_pass_through);
if (ret)
goto free_iommu;
}
- check_tylersburg_isoch();
/*
* If we copied translations from a previous kernel in the kdump
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3.18 12/39] x86/pci-calgary: Fix iommu_free() comparison of unsigned expression >= 0
3.18-stable review patch. If anyone has any objections, please let me know.
--
From: Nikola Pajkovsky
commit 68dee8e2f2cacc54d038394e70d22411dee89da2 upstream.
commit 8fd524b355da ("x86: Kill bad_dma_address variable") has killed
bad_dma_address variable and used instead of macro DMA_ERROR_CODE
which is always zero. Since dma_addr is unsigned, the statement
dma_addr >= DMA_ERROR_CODE
is always true, and not needed.
arch/x86/kernel/pci-calgary_64.c: In function ‘iommu_free’:
arch/x86/kernel/pci-calgary_64.c:299:2: warning: comparison of unsigned
expression >= 0 is always true [-Wtype-limits]
if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
Fixes: 8fd524b355da ("x86: Kill bad_dma_address variable")
Signed-off-by: Nikola Pajkovsky
Cc: iommu@lists.linux-foundation.org
Cc: Jon Mason
Cc: Muli Ben-Yehuda
Link:
http://lkml.kernel.org/r/7612c0f9dd7c1290407dbf8e809def922006920b.1479161177.git.npajkov...@suse.cz
Signed-off-by: Thomas Gleixner
Signed-off-by: Greg Kroah-Hartman
---
arch/x86/kernel/pci-calgary_64.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -296,7 +296,7 @@ static void iommu_free(struct iommu_tabl
/* were we called with bad_dma_address? */
badend = DMA_ERROR_CODE + (EMERGENCY_PAGES * PAGE_SIZE);
- if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
+ if (unlikely(dma_addr < badend)) {
WARN(1, KERN_ERR "Calgary: driver tried unmapping bad DMA "
"address 0x%Lx\n", dma_addr);
return;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.10 060/129] x86/pci-calgary: Fix iommu_free() comparison of unsigned expression >= 0
4.10-stable review patch. If anyone has any objections, please let me know.
--
From: Nikola Pajkovsky
commit 68dee8e2f2cacc54d038394e70d22411dee89da2 upstream.
commit 8fd524b355da ("x86: Kill bad_dma_address variable") has killed
bad_dma_address variable and used instead of macro DMA_ERROR_CODE
which is always zero. Since dma_addr is unsigned, the statement
dma_addr >= DMA_ERROR_CODE
is always true, and not needed.
arch/x86/kernel/pci-calgary_64.c: In function ‘iommu_free’:
arch/x86/kernel/pci-calgary_64.c:299:2: warning: comparison of unsigned
expression >= 0 is always true [-Wtype-limits]
if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
Fixes: 8fd524b355da ("x86: Kill bad_dma_address variable")
Signed-off-by: Nikola Pajkovsky
Cc: iommu@lists.linux-foundation.org
Cc: Jon Mason
Cc: Muli Ben-Yehuda
Link:
http://lkml.kernel.org/r/7612c0f9dd7c1290407dbf8e809def922006920b.1479161177.git.npajkov...@suse.cz
Signed-off-by: Thomas Gleixner
Signed-off-by: Greg Kroah-Hartman
---
arch/x86/kernel/pci-calgary_64.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -296,7 +296,7 @@ static void iommu_free(struct iommu_tabl
/* were we called with bad_dma_address? */
badend = DMA_ERROR_CODE + (EMERGENCY_PAGES * PAGE_SIZE);
- if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
+ if (unlikely(dma_addr < badend)) {
WARN(1, KERN_ERR "Calgary: driver tried unmapping bad DMA "
"address 0x%Lx\n", dma_addr);
return;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.9 044/103] x86/pci-calgary: Fix iommu_free() comparison of unsigned expression >= 0
4.9-stable review patch. If anyone has any objections, please let me know.
--
From: Nikola Pajkovsky
commit 68dee8e2f2cacc54d038394e70d22411dee89da2 upstream.
commit 8fd524b355da ("x86: Kill bad_dma_address variable") has killed
bad_dma_address variable and used instead of macro DMA_ERROR_CODE
which is always zero. Since dma_addr is unsigned, the statement
dma_addr >= DMA_ERROR_CODE
is always true, and not needed.
arch/x86/kernel/pci-calgary_64.c: In function ‘iommu_free’:
arch/x86/kernel/pci-calgary_64.c:299:2: warning: comparison of unsigned
expression >= 0 is always true [-Wtype-limits]
if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
Fixes: 8fd524b355da ("x86: Kill bad_dma_address variable")
Signed-off-by: Nikola Pajkovsky
Cc: iommu@lists.linux-foundation.org
Cc: Jon Mason
Cc: Muli Ben-Yehuda
Link:
http://lkml.kernel.org/r/7612c0f9dd7c1290407dbf8e809def922006920b.1479161177.git.npajkov...@suse.cz
Signed-off-by: Thomas Gleixner
Signed-off-by: Greg Kroah-Hartman
---
arch/x86/kernel/pci-calgary_64.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -296,7 +296,7 @@ static void iommu_free(struct iommu_tabl
/* were we called with bad_dma_address? */
badend = DMA_ERROR_CODE + (EMERGENCY_PAGES * PAGE_SIZE);
- if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
+ if (unlikely(dma_addr < badend)) {
WARN(1, KERN_ERR "Calgary: driver tried unmapping bad DMA "
"address 0x%Lx\n", dma_addr);
return;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.4 17/60] x86/pci-calgary: Fix iommu_free() comparison of unsigned expression >= 0
4.4-stable review patch. If anyone has any objections, please let me know.
--
From: Nikola Pajkovsky
commit 68dee8e2f2cacc54d038394e70d22411dee89da2 upstream.
commit 8fd524b355da ("x86: Kill bad_dma_address variable") has killed
bad_dma_address variable and used instead of macro DMA_ERROR_CODE
which is always zero. Since dma_addr is unsigned, the statement
dma_addr >= DMA_ERROR_CODE
is always true, and not needed.
arch/x86/kernel/pci-calgary_64.c: In function ‘iommu_free’:
arch/x86/kernel/pci-calgary_64.c:299:2: warning: comparison of unsigned
expression >= 0 is always true [-Wtype-limits]
if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
Fixes: 8fd524b355da ("x86: Kill bad_dma_address variable")
Signed-off-by: Nikola Pajkovsky
Cc: iommu@lists.linux-foundation.org
Cc: Jon Mason
Cc: Muli Ben-Yehuda
Link:
http://lkml.kernel.org/r/7612c0f9dd7c1290407dbf8e809def922006920b.1479161177.git.npajkov...@suse.cz
Signed-off-by: Thomas Gleixner
Signed-off-by: Greg Kroah-Hartman
---
arch/x86/kernel/pci-calgary_64.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/arch/x86/kernel/pci-calgary_64.c
+++ b/arch/x86/kernel/pci-calgary_64.c
@@ -296,7 +296,7 @@ static void iommu_free(struct iommu_tabl
/* were we called with bad_dma_address? */
badend = DMA_ERROR_CODE + (EMERGENCY_PAGES * PAGE_SIZE);
- if (unlikely((dma_addr >= DMA_ERROR_CODE) && (dma_addr < badend))) {
+ if (unlikely(dma_addr < badend)) {
WARN(1, KERN_ERR "Calgary: driver tried unmapping bad DMA "
"address 0x%Lx\n", dma_addr);
return;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 3.14 78/78] [stable PATCH] iommu/vt-d: Fix missing IOTLB flush in intel_iommu_unmap()
3.14-stable review patch. If anyone has any objections, please let me know.
--
From: David Woodhouse
Based on commit ea8ea460c9ace60bbb5ac6e5521d637d5c15293d upstream
This missing IOTLB flush was added as a minor, inconsequential bug-fix
in commit ea8ea460c ("iommu/vt-d: Clean up and fix page table clear/free
behaviour") in 3.15. It wasn't originally intended for -stable but a
couple of users have reported issues which turn out to be fixed by
adding the missing flush.
Signed-off-by: David Woodhouse
---
For 3.14 and earlier. As noted, this fix is in 3.15 already.
drivers/iommu/intel-iommu.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4075,7 +4075,7 @@ static size_t intel_iommu_unmap(struct i
unsigned long iova, size_t size)
{
struct dmar_domain *dmar_domain = domain->priv;
- int order;
+ int order, iommu_id;
order = dma_pte_clear_range(dmar_domain, iova >> VTD_PAGE_SHIFT,
(iova + size - 1) >> VTD_PAGE_SHIFT);
@@ -4083,6 +4083,22 @@ static size_t intel_iommu_unmap(struct i
if (dmar_domain->max_addr == iova + size)
dmar_domain->max_addr = iova;
+ for_each_set_bit(iommu_id, dmar_domain->iommu_bmp, g_num_of_iommus) {
+ struct intel_iommu *iommu = g_iommus[iommu_id];
+ int num, ndomains;
+
+ /*
+* find bit position of dmar_domain
+*/
+ ndomains = cap_ndoms(iommu->cap);
+ for_each_set_bit(num, iommu->domain_ids, ndomains) {
+ if (iommu->domains[num] == dmar_domain)
+ iommu_flush_iotlb_psi(iommu, num,
+ iova >> VTD_PAGE_SHIFT,
+ 1 << order, 0);
+ }
+ }
+
return PAGE_SIZE << order;
}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] iommu: Constify struct iommu_ops
On Fri, Jun 27, 2014 at 09:03:12AM +0200, Thierry Reding wrote: > From: Thierry Reding > > This structure is read-only data and should never be modified. > > Signed-off-by: Thierry Reding > --- > Changes in v2: > - add missing hunk from include/device.h > Signed-off-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: 3.16rc3 multiplatform, Armada 370 and IOMMU: unbootable kernel
On Sat, Jul 05, 2014 at 12:03:08PM -0300, Ezequiel Garcia wrote: > After following Gregory's stacktrace (also reproduced here): > > [] (iommu_bus_notifier) from [] > (notifier_call_chain+0x64/0x9c) > [] (notifier_call_chain) from [] > (__blocking_notifier_call_chain+0x40/0x58) > [] (__blocking_notifier_call_chain) from [] > (blocking_notifier_call_chain+0x14/0x1c) > [] (blocking_notifier_call_chain) from [] > (device_add+0x424/0x524) > [] (device_add) from [] (pci_device_add+0xec/0x110) > [] (pci_device_add) from [] > (pci_scan_single_device+0xa0/0xac) > > I added a few printks and found that the problem is that the > iommu_bus_notifier is > called for the 'pci' bus type, which has a null iommu_ops. > > On 04 Jul 10:47 AM, Laurent Pinchart wrote: > [..] > > > > We need a quick fix for v3.16, ... > > Therefore, a quick fix would be to simply check for that: > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index efc..b712cb2 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -536,6 +536,9 @@ static int iommu_bus_notifier(struct notifier_block *nb, > struct iommu_group *group; > unsigned long group_action = 0; > > + if (!ops) > + return 0; > + > /* > * ADD/DEL call into iommu driver ops if provided, which may > * result in ADD/DEL notifiers to group->notifier > > This (nasty workaround?) patch makes the problem go away. > > [..] > > > So it also boot well in 3.15 and then failed in 3.16-rc3. I hope it will > > > help the developers of the OMAP IOMMU driver to fix it. > > > > Thank you. I've had a look at the OMAP IOMMU driver changes between v3.15 > > and > > v3.16-rc3, and didn't find at first sight any change that could explain the > > crash. > > > > 286f600 iommu/omap: Fix map protection value handling > > 67b779d iommu/omap: Remove comment about supporting single page mappings > > only > > f7129a0 iommu/omap: Fix 'no page for' debug message in flush_iotlb_page() > > 5acc97d iommu/omap: Move to_iommu definition from omap-iopgtable.h > > 2ac6133 iommu/omap: Remove omap_iommu_domain_has_cap() function > > d760e3e iommu/omap: Correct init value of iotlb_entry valid field > > > > Could you try reverting those changes and retest ? If the problem doesn't > > disappear, we'll need to look somewhere else. > > > > I reverted the above commits but nothing changed. I'm far from being an > expert, > but it sounds odd to have this bus notifier (that got registered for the > platform bus type) called by a pci bus type. Why wouldn't the PCI bus set this up for its devices? Are you "assuming" you know the bus type and that's the issue? I see the a number of different places this is being initialized for the pci bus. Ah, look at drivers/iommu/fsl_pamu_domain.c, odds are, it shouldn't be doing that logic in the pamu_domain_init() code, using the same bus ops for different bus types, that's ripe for major problems... thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: 3.16rc3 multiplatform, Armada 370 and IOMMU: unbootable kernel
On Mon, Jul 07, 2014 at 07:58:18AM -0300, Ezequiel Garcia wrote:
> On 05 Jul 01:59 PM, Greg Kroah-Hartman wrote:
> > On Sat, Jul 05, 2014 at 12:03:08PM -0300, Ezequiel Garcia wrote:
> > > After following Gregory's stacktrace (also reproduced here):
> > >
> > > [] (iommu_bus_notifier) from []
> > > (notifier_call_chain+0x64/0x9c)
> > > [] (notifier_call_chain) from []
> > > (__blocking_notifier_call_chain+0x40/0x58)
> > > [] (__blocking_notifier_call_chain) from []
> > > (blocking_notifier_call_chain+0x14/0x1c)
> > > [] (blocking_notifier_call_chain) from []
> > > (device_add+0x424/0x524)
> > > [] (device_add) from [] (pci_device_add+0xec/0x110)
> > > [] (pci_device_add) from []
> > > (pci_scan_single_device+0xa0/0xac)
> > >
> > > I added a few printks and found that the problem is that the
> > > iommu_bus_notifier is
> > > called for the 'pci' bus type, which has a null iommu_ops.
> > >
> > > On 04 Jul 10:47 AM, Laurent Pinchart wrote:
> > > [..]
> > > >
> > > > We need a quick fix for v3.16, ...
> > >
> > > Therefore, a quick fix would be to simply check for that:
> > >
> > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > index efc..b712cb2 100644
> > > --- a/drivers/iommu/iommu.c
> > > +++ b/drivers/iommu/iommu.c
> > > @@ -536,6 +536,9 @@ static int iommu_bus_notifier(struct notifier_block
> > > *nb,
> > > struct iommu_group *group;
> > > unsigned long group_action = 0;
> > >
> > > + if (!ops)
> > > + return 0;
> > > +
> > > /*
> > > * ADD/DEL call into iommu driver ops if provided, which may
> > > * result in ADD/DEL notifiers to group->notifier
> > >
> > > This (nasty workaround?) patch makes the problem go away.
> > >
> > > [..]
> > > > > So it also boot well in 3.15 and then failed in 3.16-rc3. I hope it
> > > > > will
> > > > > help the developers of the OMAP IOMMU driver to fix it.
> > > >
> > > > Thank you. I've had a look at the OMAP IOMMU driver changes between
> > > > v3.15 and
> > > > v3.16-rc3, and didn't find at first sight any change that could explain
> > > > the
> > > > crash.
> > > >
> > > > 286f600 iommu/omap: Fix map protection value handling
> > > > 67b779d iommu/omap: Remove comment about supporting single page
> > > > mappings only
> > > > f7129a0 iommu/omap: Fix 'no page for' debug message in
> > > > flush_iotlb_page()
> > > > 5acc97d iommu/omap: Move to_iommu definition from omap-iopgtable.h
> > > > 2ac6133 iommu/omap: Remove omap_iommu_domain_has_cap() function
> > > > d760e3e iommu/omap: Correct init value of iotlb_entry valid field
> > > >
> > > > Could you try reverting those changes and retest ? If the problem
> > > > doesn't
> > > > disappear, we'll need to look somewhere else.
> > > >
> > >
> > > I reverted the above commits but nothing changed. I'm far from being an
> > > expert,
> > > but it sounds odd to have this bus notifier (that got registered for the
> > > platform bus type) called by a pci bus type.
> >
> > Why wouldn't the PCI bus set this up for its devices? Are you
> > "assuming" you know the bus type and that's the issue?
> >
>
> Thanks for looking at this.
>
> I guess I snipped the thread and lost most of the information about the panic.
> Here's the original bug report:
>
> http://www.spinics.net/lists/arm-kernel/msg344059.html
>
> The problem reported involves enabling OMAP IOMMU driver and not any other
> IOMMU
> driver. Doing some tracing and adding a few prints, we found that
> omap_iommu_init() sets a bus notifier for the platform bus type:
>
> omap_iommu_init -> bus_set_iommu -> iommu_bus_init:
>
> static void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
> {
> bus_register_notifier(bus, &iommu_bus_nb);
> bus_for_each_dev(bus, NULL, ops, add_iommu_group);
> }
>
> But the iommu bus notifier gets called for the 'pci' bus type, which
> has the iommu_ops field NULL (since it hasn't been set for iommu).
So this is what needs to be figured out, how is the notifier being
called with a PCI device? Who else called iommu_bus_init() for the PCI
bus?
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: 3.16rc3 multiplatform, Armada 370 and IOMMU: unbootable kernel
On Mon, Jul 07, 2014 at 08:37:58PM -0300, Ezequiel Garcia wrote:
> On 07 Jul 11:30 AM, Greg Kroah-Hartman wrote:
> > On Mon, Jul 07, 2014 at 07:58:18AM -0300, Ezequiel Garcia wrote:
> [..]
> > >
> > > I guess I snipped the thread and lost most of the information about the
> > > panic.
> > > Here's the original bug report:
> > >
> > > http://www.spinics.net/lists/arm-kernel/msg344059.html
> > >
> > > The problem reported involves enabling OMAP IOMMU driver and not any
> > > other IOMMU
> > > driver. Doing some tracing and adding a few prints, we found that
> > > omap_iommu_init() sets a bus notifier for the platform bus type:
> > >
> > > omap_iommu_init -> bus_set_iommu -> iommu_bus_init:
> > >
> > > static void iommu_bus_init(struct bus_type *bus, struct iommu_ops *ops)
> > > {
> > > bus_register_notifier(bus, &iommu_bus_nb);
> > > bus_for_each_dev(bus, NULL, ops, add_iommu_group);
> > > }
> > >
> > > But the iommu bus notifier gets called for the 'pci' bus type, which
> > > has the iommu_ops field NULL (since it hasn't been set for iommu).
> >
> > So this is what needs to be figured out, how is the notifier being
> > called with a PCI device? Who else called iommu_bus_init() for the PCI
> > bus?
> >
>
> Nobody. The only caller of iommu_bus_init() the above OMAP IOMMU.
>
> However, I found something interesting. Tried to bisect this without much
> luck; I did two bisects and ended up in the same merge commit:
>
> # good: [0f16aa3c24a216d14d7f0587e1cbd2c1b51a38f3] Merge tag 'samsung-dt-3'
> of git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung into
> next/dt
> # first bad commit: [755a9ba7bf24a45b6dbf8bb15a5a56c8ed12461a] Merge tag
> 'dt-for-3.16' of git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
> into next
>
> So, after doing a few diff's between that good and bad and searching for
> "bus_notifier" changes, saw something strange in
> arch/arm/mach-mvebu/coherency.c.
>
> It seems bus_register_notifier() is been called for platform and pci devices
> with the *same* notifier block. Haven't looked close enough, but you mentioned
> that could cause trouble?
>
> This patch fixes the issue here:
>
> diff --git a/arch/arm/mach-mvebu/coherency.c b/arch/arm/mach-mvebu/coherency.c
> index 477202f..2bdc323 100644
> --- a/arch/arm/mach-mvebu/coherency.c
> +++ b/arch/arm/mach-mvebu/coherency.c
> @@ -292,6 +292,10 @@ static struct notifier_block mvebu_hwcc_nb = {
> .notifier_call = mvebu_hwcc_notifier,
> };
>
> +static struct notifier_block mvebu_hwcc_pci_nb = {
> + .notifier_call = mvebu_hwcc_notifier,
> +};
> +
> static void __init armada_370_coherency_init(struct device_node *np)
> {
> struct resource res;
> @@ -427,7 +431,7 @@ static int __init coherency_pci_init(void)
> {
> if (coherency_available())
> bus_register_notifier(&pci_bus_type,
> -&mvebu_hwcc_nb);
> +&mvebu_hwcc_pci_nb);
> return 0;
> }
>
> Paolo, can you apply it and confirm it fixes the problem?
>
> Greg, can you confirm using the same notifier block pointer
> for two different bus types makes the bus notifier go nuts?
The bus notifier doesn't "go nuts", but the call back functions must be
able to handle the devices for that specific bus being passed to the
function for it.
Again, you shouldn't ever mix bus types with the same notifier unless
you _really_ know what you are doing, and even then, I wouldn't
recommend it.
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.18 33/34] x86/swiotlb: Enable swiotlb for > 4GiG RAM on 32-bit kernels
4.18-stable review patch. If anyone has any objections, please let me know.
--
From: Christoph Hellwig
commit 485734f3fc77c1eb77ffe138c027b9a4bf0178f3 upstream.
We already build the swiotlb code for 32-bit kernels with PAE support,
but the code to actually use swiotlb has only been enabled for 64-bit
kernels for an unknown reason.
Before Linux v4.18 we paper over this fact because the networking code,
the SCSI layer and some random block drivers implemented their own
bounce buffering scheme.
[ mingo: Changelog fixes. ]
Fixes: 21e07dba9fb1 ("scsi: reduce use of block bounce buffers")
Fixes: ab74cfebafa3 ("net: remove the PCI_DMA_BUS_IS_PHYS check in
illegal_highdma")
Reported-by: Matthew Whitehead
Signed-off-by: Christoph Hellwig
Signed-off-by: Thomas Gleixner
Tested-by: Matthew Whitehead
Cc: konrad.w...@oracle.com
Cc: iommu@lists.linux-foundation.org
Cc: sta...@vger.kernel.org
Link: https://lkml.kernel.org/r/20181014075208.2715-1-...@lst.de
Signed-off-by: Ingo Molnar
Signed-off-by: Greg Kroah-Hartman
---
arch/x86/kernel/pci-swiotlb.c |2 --
1 file changed, 2 deletions(-)
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -42,10 +42,8 @@ IOMMU_INIT_FINISH(pci_swiotlb_detect_ove
int __init pci_swiotlb_detect_4gb(void)
{
/* don't initialize swiotlb if iommu=off (no_iommu=1) */
-#ifdef CONFIG_X86_64
if (!no_iommu && max_possible_pfn > MAX_DMA32_PFN)
swiotlb = 1;
-#endif
/*
* If SME is active then swiotlb will be set to 1 so that bounce
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/5] driver core: Introduce device_iommu_mapped() function
On Tue, Dec 04, 2018 at 06:25:00PM +0100, Joerg Roedel wrote: > From: Joerg Roedel > > Some places in the kernel check the iommu_group pointer in > 'struct device' in order to find ot whether a device is > mapped by an IOMMU. > > This is not good way to make this check, as the pointer will > be moved to 'struct dev_iommu_data'. This way to make the > check is also not very readable. > > Introduce an explicit function to perform this check. > > Cc: Greg Kroah-Hartman > Signed-off-by: Joerg Roedel > --- > include/linux/device.h | 10 ++++++ > 1 file changed, 10 insertions(+) Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/6] driver core: Introduce device_iommu_mapped() function
On Tue, Dec 11, 2018 at 02:43:38PM +0100, Joerg Roedel wrote: > From: Joerg Roedel > > Some places in the kernel check the iommu_group pointer in > 'struct device' in order to find ot whether a device is > mapped by an IOMMU. > > This is not good way to make this check, as the pointer will > be moved to 'struct dev_iommu_data'. This way to make the > check is also not very readable. > > Introduce an explicit function to perform this check. > > Cc: Greg Kroah-Hartman > Acked-by: Greg Kroah-Hartman No need to have a cc: line if I have already acked it :) thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] dma: debug: no need to check return value of debugfs_create functions
When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Also delete the variables for the file dentries for the debugfs entries
as they are never used at all once they are created.
Cc: Christoph Hellwig
Cc: Marek Szyprowski
Cc: Robin Murphy
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Greg Kroah-Hartman
---
kernel/dma/debug.c | 81 ++
1 file changed, 10 insertions(+), 71 deletions(-)
diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
index 23cf5361bcf1..2f5fc8b9d39f 100644
--- a/kernel/dma/debug.c
+++ b/kernel/dma/debug.c
@@ -136,14 +136,6 @@ static u32 nr_prealloc_entries =
PREALLOC_DMA_DEBUG_ENTRIES;
/* debugfs dentry's for the stuff above */
static struct dentry *dma_debug_dent__read_mostly;
-static struct dentry *global_disable_dent __read_mostly;
-static struct dentry *error_count_dent __read_mostly;
-static struct dentry *show_all_errors_dent __read_mostly;
-static struct dentry *show_num_errors_dent __read_mostly;
-static struct dentry *num_free_entries_dent __read_mostly;
-static struct dentry *min_free_entries_dent __read_mostly;
-static struct dentry *nr_total_entries_dent __read_mostly;
-static struct dentry *filter_dent __read_mostly;
/* per-driver filter related state */
@@ -840,66 +832,18 @@ static const struct file_operations filter_fops = {
.llseek = default_llseek,
};
-static int dma_debug_fs_init(void)
+static void dma_debug_fs_init(void)
{
dma_debug_dent = debugfs_create_dir("dma-api", NULL);
- if (!dma_debug_dent) {
- pr_err("can not create debugfs directory\n");
- return -ENOMEM;
- }
-
- global_disable_dent = debugfs_create_bool("disabled", 0444,
- dma_debug_dent,
- &global_disable);
- if (!global_disable_dent)
- goto out_err;
-
- error_count_dent = debugfs_create_u32("error_count", 0444,
- dma_debug_dent, &error_count);
- if (!error_count_dent)
- goto out_err;
-
- show_all_errors_dent = debugfs_create_u32("all_errors", 0644,
- dma_debug_dent,
- &show_all_errors);
- if (!show_all_errors_dent)
- goto out_err;
-
- show_num_errors_dent = debugfs_create_u32("num_errors", 0644,
- dma_debug_dent,
- &show_num_errors);
- if (!show_num_errors_dent)
- goto out_err;
-
- num_free_entries_dent = debugfs_create_u32("num_free_entries", 0444,
- dma_debug_dent,
- &num_free_entries);
- if (!num_free_entries_dent)
- goto out_err;
-
- min_free_entries_dent = debugfs_create_u32("min_free_entries", 0444,
- dma_debug_dent,
- &min_free_entries);
- if (!min_free_entries_dent)
- goto out_err;
-
- nr_total_entries_dent = debugfs_create_u32("nr_total_entries", 0444,
- dma_debug_dent,
- &nr_total_entries);
- if (!nr_total_entries_dent)
- goto out_err;
-
- filter_dent = debugfs_create_file("driver_filter", 0644,
- dma_debug_dent, NULL, &filter_fops);
- if (!filter_dent)
- goto out_err;
-
- return 0;
-out_err:
- debugfs_remove_recursive(dma_debug_dent);
-
- return -ENOMEM;
+ debugfs_create_bool("disabled", 0444, dma_debug_dent, &global_disable);
+ debugfs_create_u32("error_count", 0444, dma_debug_dent, &error_count);
+ debugfs_create_u32("all_errors", 0644, dma_debug_dent,
&show_all_errors);
+ debugfs_create_u32("num_errors", 0644, dma_debug_dent,
&show_num_errors);
+ debugfs_create_u32("num_free_entries", 0444, dma_debug_dent,
&num_free_entries);
+ debugfs_create_u32("min_free_entries", 0444, dma_debug_dent,
&min_free_entries);
+ debugfs_create_u32("nr_total_entries", 0444, dma_debug_dent,
&nr_total_entries);
+ debugfs_create_file("driver_filter", 0644, dma_debug_dent, NULL,
&filter_fops);
}
static int device_dma_allocations(struct device *dev, struct dma_debug_entry
**out_entry)
@@ -985,12 +929,7 @@ static int dma_debug_init(void)
spin_lock_init(&dma_entry_hash[i].lock);
}
- if (dma_debug_fs_init() != 0) {
- pr_err("error creating debugfs entries - disabling\n");
- global_disable = true;
-
- retu
Re: [PATCH] dma: debug: no need to check return value of debugfs_create functions
On Tue, Jan 22, 2019 at 06:44:38PM +, Robin Murphy wrote: > Hi Greg, > > On 22/01/2019 15:21, Greg Kroah-Hartman wrote: > > When calling debugfs functions, there is no need to ever check the > > return value. The function can work or not, but the code logic should > > never do something different based on this. > > > > Also delete the variables for the file dentries for the debugfs entries > > as they are never used at all once they are created. > > Modulo one nit below, I certainly approve of the cleanup :) > > Reviewed-by: Robin Murphy > > > Cc: Christoph Hellwig > > Cc: Marek Szyprowski > > Cc: Robin Murphy > > Cc: iommu@lists.linux-foundation.org > > Signed-off-by: Greg Kroah-Hartman > > --- > > kernel/dma/debug.c | 81 ++ > > 1 file changed, 10 insertions(+), 71 deletions(-) > > > > diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c > > index 23cf5361bcf1..2f5fc8b9d39f 100644 > > --- a/kernel/dma/debug.c > > +++ b/kernel/dma/debug.c > > @@ -136,14 +136,6 @@ static u32 nr_prealloc_entries = > > PREALLOC_DMA_DEBUG_ENTRIES; > > /* debugfs dentry's for the stuff above */ > > static struct dentry *dma_debug_dent__read_mostly; > > Does this actually need to be at file scope, or could it be punted to a > local in dma_debug_fs_init() while we're here? It can be moved to the function scope if you want me to, I was trying to do the least needed here :) thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 1/5] pci/p2p: add a function to test peer to peer capability
On Tue, Jan 29, 2019 at 11:24:09AM -0700, Logan Gunthorpe wrote:
>
>
> On 2019-01-29 10:47 a.m., jgli...@redhat.com wrote:
> > +bool pci_test_p2p(struct device *devA, struct device *devB)
> > +{
> > + struct pci_dev *pciA, *pciB;
> > + bool ret;
> > + int tmp;
> > +
> > + /*
> > +* For now we only support PCIE peer to peer but other inter-connect
> > +* can be added.
> > +*/
> > + pciA = find_parent_pci_dev(devA);
> > + pciB = find_parent_pci_dev(devB);
> > + if (pciA == NULL || pciB == NULL) {
> > + ret = false;
> > + goto out;
> > + }
> > +
> > + tmp = upstream_bridge_distance(pciA, pciB, NULL);
> > + ret = tmp < 0 ? false : true;
> > +
> > +out:
> > + pci_dev_put(pciB);
> > + pci_dev_put(pciA);
> > + return false;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_test_p2p);
>
> This function only ever returns false
I guess it was nevr actually tested :(
I feel really worried about passing random 'struct device' pointers into
the PCI layer. Are we _sure_ it can handle this properly?
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 2/5] drivers/base: add a function to test peer to peer capability
On Tue, Jan 29, 2019 at 12:47:25PM -0500, jgli...@redhat.com wrote: > From: Jérôme Glisse > > device_test_p2p() return true if two devices can peer to peer to > each other. We add a generic function as different inter-connect > can support peer to peer and we want to genericaly test this no > matter what the inter-connect might be. However this version only > support PCIE for now. There is no defintion of "peer to peer" in the driver/device model, so why should this be in the driver core at all? Especially as you only do this for PCI, why not just keep it in the PCI layer, that way you _know_ you are dealing with the right pointer types and there is no need to mess around with the driver core at all. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] dma: debug: no need to check return value of debugfs_create functions
On Fri, Feb 01, 2019 at 10:04:02AM +0100, Christoph Hellwig wrote: > On Tue, Jan 22, 2019 at 07:48:47PM +0100, Greg Kroah-Hartman wrote: > > > Does this actually need to be at file scope, or could it be punted to a > > > local in dma_debug_fs_init() while we're here? > > > > It can be moved to the function scope if you want me to, I was trying to > > do the least needed here :) > > I've folded the scope move in when applying the patch to the > dma-mapping for-next tree. Wonderful, thanks! greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 06/12] dma-mapping: improve selection of dma_declare_coherent availability
On Mon, Feb 11, 2019 at 02:35:48PM +0100, Christoph Hellwig wrote: > This API is primarily used through DT entries, but two architectures > and two drivers call it directly. So instead of selecting the config > symbol for random architectures pull it in implicitly for the actual > users. Also rename the Kconfig option to describe the feature better. > > Signed-off-by: Christoph Hellwig Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 02/12] device.h: dma_mem is only needed for HAVE_GENERIC_DMA_COHERENT
On Mon, Feb 11, 2019 at 02:35:44PM +0100, Christoph Hellwig wrote: > No need to carry an unused field around. > > Signed-off-by: Christoph Hellwig > --- > include/linux/device.h | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 09/12] dma-mapping: remove the DMA_MEMORY_EXCLUSIVE flag
On Mon, Feb 11, 2019 at 02:35:51PM +0100, Christoph Hellwig wrote: > All users of dma_declare_coherent want their allocations to be > exclusive, so default to exclusive allocations. > > Signed-off-by: Christoph Hellwig > --- > Documentation/DMA-API.txt | 9 +-- > arch/arm/mach-imx/mach-imx27_visstrim_m10.c | 12 +++-- > arch/arm/mach-imx/mach-mx31moboard.c | 3 +-- > arch/sh/boards/mach-ap325rxa/setup.c | 5 ++-- > arch/sh/boards/mach-ecovec24/setup.c | 6 ++--- > arch/sh/boards/mach-kfr2r09/setup.c | 5 ++-- > arch/sh/boards/mach-migor/setup.c | 5 ++-- > arch/sh/boards/mach-se/7724/setup.c | 6 ++--- > arch/sh/drivers/pci/fixups-dreamcast.c| 3 +-- > .../soc_camera/sh_mobile_ceu_camera.c | 3 +-- > drivers/usb/host/ohci-sm501.c | 3 +-- > drivers/usb/host/ohci-tmio.c | 2 +- > include/linux/dma-mapping.h | 7 ++ > kernel/dma/coherent.c | 25 ++- > 14 files changed, 29 insertions(+), 65 deletions(-) Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 07/12] dma-mapping: move CONFIG_DMA_CMA to kernel/dma/Kconfig
On Mon, Feb 11, 2019 at 02:35:49PM +0100, Christoph Hellwig wrote: > This is where all the related code already lives. > > Signed-off-by: Christoph Hellwig > --- > drivers/base/Kconfig | 77 > kernel/dma/Kconfig | 77 > 2 files changed, 77 insertions(+), 77 deletions(-) Much nicer, thanks! Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
On Thu, Mar 07, 2019 at 02:52:55PM +, Robin Murphy wrote: > Hi John, > > On 07/03/2019 14:45, John Garry wrote: > [...] > > Hi guys, > > > > Any idea what happened to this fix? > > It's been in -next for a while (commit 376991db4b64) - I assume it will land > shortly and hit stable thereafter, at which point somebody gets to sort out > the manual backport past 4.20. It's already in Linus's tree, I'll go try to do the stable backporting now... greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: drivers/iommu/qcom_iommu.c:348 qcom_iommu_domain_free+0x5c/0x70
On Tue, Feb 18, 2020 at 04:13:57PM +0530, Naresh Kamboju wrote: > We have noticed these kernel warnings on APQ 8016 SBC (dragonboard > 410c ) running stable rc 5.5, 5.4 branches and linux mainline and > linux-next master branches. > > This warning started happening from linux mainline 5.3 onwards (2019-08-29) > > [5.488128] [ cut here ] > [5.488161] WARNING: CPU: 3 PID: 221 at > drivers/iommu/qcom_iommu.c:348 qcom_iommu_domain_free+0x5c/0x70 > [5.491817] Modules linked in: crct10dif_ce(+) adv7511(+) cec > msm(+) mdt_loader drm_kms_helper drm fuse > [5.491842] CPU: 3 PID: 221 Comm: systemd-udevd Not tainted > 5.4.21-rc1-00037-g6f8f5c79416e #1 > [5.491844] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT) > [5.491849] pstate: 0005 (nzcv daif -PAN -UAO) > [5.491858] pc : qcom_iommu_domain_free+0x5c/0x70 > [5.491865] lr : iommu_group_release+0x44/0x60 > [5.491867] sp : 800011e83600 > Starting Network Manager...[5.491870] x29: > 800011e83600 x28: 0100 > [5.491893] x19: 36a9e500 x18: 0001 > [5.491898] x17: x16: > [5.491902] x15: 0004 x14: > [5.491907] x13: x12: 3a137aa0 > [5.491911] x11: 3a137908 x10: > [5.491916] x9 : 0eb2c098 x8 : 0eb2c090 > [5.491920] x7 : 400c x6 : 0004 > [5.491924] x5 : 0001 x4 : > [5.491929] x3 : x2 : > [5.491933] x1 : 3bdb9680 x0 : 36aa0ab0 > [5.491938] Call trace: > [5.491946] qcom_iommu_domain_free+0x5c/0x70 > [5.491951] iommu_group_release+0x44/0x60 > [5.491957] kobject_put+0x6c/0xf0 > [5.491962] kobject_del+0x50/0x68 > [5.491966] kobject_put+0xd0/0xf0 > [5.491972] iommu_group_remove_device+0xd8/0xec > [5.491977] qcom_iommu_remove_device+0x44/0x60 > [5.491982] iommu_release_device+0x28/0x40 > [5.491987] iommu_bus_notifier+0xac/0xc0 > [5.491993] notifier_call_chain+0x58/0x98 > [5.491998] blocking_notifier_call_chain+0x54/0x78 > [5.492003] device_del+0x224/0x348 > [5.492008] platform_device_del.part.0+0x18/0x88 > [5.492012] platform_device_unregister+0x20/0x38 > [5.492019] of_platform_device_destroy+0xdc/0xf0 > [5.492023] device_for_each_child+0x58/0xa0 > [5.492028] of_platform_depopulate+0x38/0x78 > [5.492155] msm_pdev_probe+0x1c0/0x308 [msm] > [5.492166] platform_drv_probe+0x50/0xa0 > [5.682404] really_probe+0xd4/0x308 > [5.686388] driver_probe_device+0x54/0xe8 > [5.690035] device_driver_attach+0x6c/0x78 > [5.693940] __driver_attach+0x54/0xd0 > [5.698020] bus_for_each_dev+0x6c/0xc0 > [5.701838] driver_attach+0x20/0x28 > [5.705572] bus_add_driver+0x140/0x1e8 > [5.709390] driver_register+0x60/0x110 > [5.712950] __platform_driver_register+0x44/0x50 > [5.716886] msm_drm_register+0x54/0x68 [msm] > [5.721633] do_one_initcall+0x50/0x190 > [5.725972] do_init_module+0x50/0x208 > [5.729615] load_module+0x1d1c/0x2298 > [5.733435] __do_sys_finit_module+0xd0/0xe8 > [5.737169] __arm64_sys_finit_module+0x1c/0x28 > [5.741598] el0_svc_common.constprop.0+0x68/0x160 > [5.745851] el0_svc_handler+0x20/0x80 > [5.750709] el0_svc+0x8/0xc > [5.754443] ---[ end trace 0ebe200932dd18fc ]--- > > > Full test log link, > https://lkft.validation.linaro.org/scheduler/job/1226062#L3841 > https://lkft.validation.linaro.org/scheduler/job/1227242#L3877 > > Ref: > https://bugs.linaro.org/show_bug.cgi?id=5460 Can you bisect to find the offending commit? thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: drivers/iommu/qcom_iommu.c:348 qcom_iommu_domain_free+0x5c/0x70
On Tue, Feb 18, 2020 at 06:31:51PM +, Robin Murphy wrote: > On 18/02/2020 6:23 pm, Greg Kroah-Hartman wrote: > [...] > > Can you bisect to find the offending commit? > > This particular presentation appears to be down to the DRM driver starting > to call of_platform_depopulate(), but it's merely exposing badness in the > IOMMU driver that's been there from day 1. I just sent a fix for that[1]. > > Robin. > > [1] > https://lore.kernel.org/linux-iommu/be92829c6e5467634b109add002351e6cf9e18d2.1582049382.git.robin.mur...@arm.com/ Can you also add a cc: stable tag to it so that I know to pick it up? thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] dma-mapping: add a dma_ops_bypass flag to struct device
On Fri, Mar 20, 2020 at 03:16:39PM +0100, Christoph Hellwig wrote: > Several IOMMU drivers have a bypass mode where they can use a direct > mapping if the devices DMA mask is large enough. Add generic support > to the core dma-mapping code to do that to switch those drivers to > a common solution. > > Signed-off-by: Christoph Hellwig > --- > include/linux/device.h | 6 ++ > include/linux/dma-mapping.h | 30 ++ > kernel/dma/mapping.c| 36 +++- > 3 files changed, 51 insertions(+), 21 deletions(-) Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 06/16] iommu: Move iommu_fwspec to struct dev_iommu
On Thu, Mar 26, 2020 at 04:08:31PM +0100, Joerg Roedel wrote: > From: Joerg Roedel > > Move the iommu_fwspec pointer in struct device into struct dev_iommu. > This is a step in the effort to reduce the iommu related pointers in > struct device to one. > > Cc: Greg Kroah-Hartman > Tested-by: Will Deacon # arm-smmu > Reviewed-by: Jean-Philippe Brucker > Signed-off-by: Joerg Roedel > --- > drivers/iommu/iommu.c | 3 +++ > include/linux/device.h | 3 --- > include/linux/iommu.h | 12 > 3 files changed, 11 insertions(+), 7 deletions(-) Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/16] iommu: Rename struct iommu_param to dev_iommu
On Thu, Mar 26, 2020 at 04:08:30PM +0100, Joerg Roedel wrote: > From: Joerg Roedel > > The term dev_iommu aligns better with other existing structures and > their accessor functions. > > Cc: Greg Kroah-Hartman > Tested-by: Will Deacon # arm-smmu > Reviewed-by: Jean-Philippe Brucker > Signed-off-by: Joerg Roedel > --- > drivers/iommu/iommu.c | 28 ++-- > include/linux/device.h | 6 +++--- > include/linux/iommu.h | 4 ++-- > 3 files changed, 19 insertions(+), 19 deletions(-) Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 3/4] dma-mapping: add a dma_ops_bypass flag to struct device
On Tue, Apr 14, 2020 at 02:25:05PM +0200, Christoph Hellwig wrote: > Several IOMMU drivers have a bypass mode where they can use a direct > mapping if the devices DMA mask is large enough. Add generic support > to the core dma-mapping code to do that to switch those drivers to > a common solution. > > Signed-off-by: Christoph Hellwig > --- > include/linux/device.h | 6 > kernel/dma/mapping.c | 70 +- > 2 files changed, 55 insertions(+), 21 deletions(-) Reviewed-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] drivers/iommu: properly export iommu_group_get_for_dev
In commit a7ba5c3d008d ("drivers/iommu: Export core IOMMU API symbols to
permit modular drivers") a bunch of iommu symbols were exported, all
with _GPL markings except iommu_group_get_for_dev(). That export should
also be _GPL like the others.
Cc: Will Deacon
Cc: Joerg Roedel
Cc: John Garry
Fixes: a7ba5c3d008d ("drivers/iommu: Export core IOMMU API symbols to permit
modular drivers")
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2b471419e26c..1ecbc8788fe7 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1428,7 +1428,7 @@ struct iommu_group *iommu_group_get_for_dev(struct device
*dev)
return group;
}
-EXPORT_SYMBOL(iommu_group_get_for_dev);
+EXPORT_SYMBOL_GPL(iommu_group_get_for_dev);
struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
{
--
2.26.2
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 09/15] device core: Add ability to handle multiple dma offsets
On Tue, May 19, 2020 at 04:34:07PM -0400, Jim Quinlan wrote:
> diff --git a/include/linux/device.h b/include/linux/device.h
> index ac8e37cd716a..6cd916860b5f 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -493,6 +493,8 @@ struct dev_links_info {
> * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a smaller
> * DMA limit than the device itself supports.
> * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> + * @dma_map: Like dma_pfn_offset but used when there are multiple
> + * pfn offsets for multiple dma-ranges.
> * @dma_parms: A low level driver may set these to teach IOMMU code
> about
> * segment limitations.
> * @dma_pools: Dma pools (if dma'ble device).
> @@ -578,7 +580,12 @@ struct device {
>allocations such descriptors. */
> u64 bus_dma_limit; /* upstream dma constraint */
> unsigned long dma_pfn_offset;
> -
> +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> + const void *dma_offset_map; /* Like dma_pfn_offset, but for
> + * the unlikely case of multiple
> + * offsets. If non-null, dma_pfn_offset
> + * will be 0. */
> +#endif
> struct device_dma_parameters *dma_parms;
>
> struct list_headdma_pools; /* dma pools (if dma'ble) */
I'll defer to Christoph here, but I thought we were trying to get rid of
stuff like this from struct device, not add new things to it for dma
apis. And why is it a void *?
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 09/15] device core: Add ability to handle multiple dma offsets
On Wed, May 20, 2020 at 09:50:36AM -0400, Jim Quinlan wrote:
> On Wed, May 20, 2020 at 1:43 AM Greg Kroah-Hartman
> wrote:
> >
> > On Tue, May 19, 2020 at 04:34:07PM -0400, Jim Quinlan wrote:
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index ac8e37cd716a..6cd916860b5f 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -493,6 +493,8 @@ struct dev_links_info {
> > > * @bus_dma_limit: Limit of an upstream bridge or bus which imposes a
> > > smaller
> > > * DMA limit than the device itself supports.
> > > * @dma_pfn_offset: offset of DMA memory range relatively of RAM
> > > + * @dma_map: Like dma_pfn_offset but used when there are multiple
> > > + * pfn offsets for multiple dma-ranges.
> > > * @dma_parms: A low level driver may set these to teach IOMMU
> > > code about
> > > * segment limitations.
> > > * @dma_pools: Dma pools (if dma'ble device).
> > > @@ -578,7 +580,12 @@ struct device {
> > >allocations such descriptors.
> > > */
> > > u64 bus_dma_limit; /* upstream dma constraint */
> > > unsigned long dma_pfn_offset;
> > > -
> > > +#ifdef CONFIG_DMA_PFN_OFFSET_MAP
> > > + const void *dma_offset_map; /* Like dma_pfn_offset, but for
> > > + * the unlikely case of multiple
> > > + * offsets. If non-null,
> > > dma_pfn_offset
> > > + * will be 0. */
> > > +#endif
> > > struct device_dma_parameters *dma_parms;
> > >
> > > struct list_headdma_pools; /* dma pools (if dma'ble) */
> >
> > I'll defer to Christoph here, but I thought we were trying to get rid of
> > stuff like this from struct device, not add new things to it for dma
> Hi Greg,
>
> I wasn't aware of this policy. I put it in 'struct device' because
> just like the existing dma_pfn_offset; it seemed to be the only way to
> pull this off. I'll certainly follow any ideas on alternative
> strategies from Christoph et al.
>
> > apis. And why is it a void *?
> Just wanted to minimize the number of lines I've added to device.h, no
> other reason.
How would using a real type make this more lines? Never use a void *
unless you have to, we want the compiler to check our errors for us :)
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.9 09/64] iommu/amd: Fix over-read of ACPI UID from IVRS table
From: Alexander Monakov
[ Upstream commit e461b8c991b9202b007ea2059d953e264240b0c9 ]
IVRS parsing code always tries to read 255 bytes from memory when
retrieving ACPI device path, and makes an assumption that firmware
provides a zero-terminated string. Both of those are bugs: the entry
is likely to be shorter than 255 bytes, and zero-termination is not
guaranteed.
With Acer SF314-42 firmware these issues manifest visibly in dmesg:
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1...
The first three lines show how the code over-reads adjacent table
entries into the UID, and in the last line it even reads garbage data
beyond the end of the IVRS table itself.
Since each entry has the length of the UID (uidl member of ivhd_entry
struct), use that for memcpy, and manually add a zero terminator.
Avoid zero-filling hid and uid arrays up front, and instead ensure
the uid array is always zero-terminated. No change needed for the hid
array, as it was already properly zero-terminated.
Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type
HID")
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
Link: https://lore.kernel.org/r/20200511102352.1831-1-amona...@ispras.ru
Signed-off-by: Joerg Roedel
Signed-off-by: Sasha Levin
---
drivers/iommu/amd_iommu_init.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index e6ae8d123984..a3279f303b49 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1171,8 +1171,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
}
case IVHD_DEV_ACPI_HID: {
u16 devid;
- u8 hid[ACPIHID_HID_LEN] = {0};
- u8 uid[ACPIHID_UID_LEN] = {0};
+ u8 hid[ACPIHID_HID_LEN];
+ u8 uid[ACPIHID_UID_LEN];
int ret;
if (h->type != 0x40) {
@@ -1189,6 +1189,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
+ uid[0] = '\0';
switch (e->uidf) {
case UID_NOT_PRESENT:
@@ -1203,8 +1204,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
case UID_IS_CHARACTER:
- memcpy(uid, (u8 *)(&e->uid), ACPIHID_UID_LEN -
1);
- uid[ACPIHID_UID_LEN - 1] = '\0';
+ memcpy(uid, &e->uid, e->uidl);
+ uid[e->uidl] = '\0';
break;
default:
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.14 10/59] iommu/amd: Fix over-read of ACPI UID from IVRS table
From: Alexander Monakov
[ Upstream commit e461b8c991b9202b007ea2059d953e264240b0c9 ]
IVRS parsing code always tries to read 255 bytes from memory when
retrieving ACPI device path, and makes an assumption that firmware
provides a zero-terminated string. Both of those are bugs: the entry
is likely to be shorter than 255 bytes, and zero-termination is not
guaranteed.
With Acer SF314-42 firmware these issues manifest visibly in dmesg:
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1...
The first three lines show how the code over-reads adjacent table
entries into the UID, and in the last line it even reads garbage data
beyond the end of the IVRS table itself.
Since each entry has the length of the UID (uidl member of ivhd_entry
struct), use that for memcpy, and manually add a zero terminator.
Avoid zero-filling hid and uid arrays up front, and instead ensure
the uid array is always zero-terminated. No change needed for the hid
array, as it was already properly zero-terminated.
Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type
HID")
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
Link: https://lore.kernel.org/r/20200511102352.1831-1-amona...@ispras.ru
Signed-off-by: Joerg Roedel
Signed-off-by: Sasha Levin
---
drivers/iommu/amd_iommu_init.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 6c228144b3da..ec9a20e06941 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1317,8 +1317,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
}
case IVHD_DEV_ACPI_HID: {
u16 devid;
- u8 hid[ACPIHID_HID_LEN] = {0};
- u8 uid[ACPIHID_UID_LEN] = {0};
+ u8 hid[ACPIHID_HID_LEN];
+ u8 uid[ACPIHID_UID_LEN];
int ret;
if (h->type != 0x40) {
@@ -1335,6 +1335,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
+ uid[0] = '\0';
switch (e->uidf) {
case UID_NOT_PRESENT:
@@ -1349,8 +1350,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
case UID_IS_CHARACTER:
- memcpy(uid, (u8 *)(&e->uid), ACPIHID_UID_LEN -
1);
- uid[ACPIHID_UID_LEN - 1] = '\0';
+ memcpy(uid, &e->uid, e->uidl);
+ uid[e->uidl] = '\0';
break;
default:
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.19 12/81] iommu/amd: Fix over-read of ACPI UID from IVRS table
From: Alexander Monakov
[ Upstream commit e461b8c991b9202b007ea2059d953e264240b0c9 ]
IVRS parsing code always tries to read 255 bytes from memory when
retrieving ACPI device path, and makes an assumption that firmware
provides a zero-terminated string. Both of those are bugs: the entry
is likely to be shorter than 255 bytes, and zero-termination is not
guaranteed.
With Acer SF314-42 firmware these issues manifest visibly in dmesg:
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1...
The first three lines show how the code over-reads adjacent table
entries into the UID, and in the last line it even reads garbage data
beyond the end of the IVRS table itself.
Since each entry has the length of the UID (uidl member of ivhd_entry
struct), use that for memcpy, and manually add a zero terminator.
Avoid zero-filling hid and uid arrays up front, and instead ensure
the uid array is always zero-terminated. No change needed for the hid
array, as it was already properly zero-terminated.
Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type
HID")
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
Link: https://lore.kernel.org/r/20200511102352.1831-1-amona...@ispras.ru
Signed-off-by: Joerg Roedel
Signed-off-by: Sasha Levin
---
drivers/iommu/amd_iommu_init.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 2557ed112bc2..c7d0bb3b4a30 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1334,8 +1334,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
}
case IVHD_DEV_ACPI_HID: {
u16 devid;
- u8 hid[ACPIHID_HID_LEN] = {0};
- u8 uid[ACPIHID_UID_LEN] = {0};
+ u8 hid[ACPIHID_HID_LEN];
+ u8 uid[ACPIHID_UID_LEN];
int ret;
if (h->type != 0x40) {
@@ -1352,6 +1352,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
+ uid[0] = '\0';
switch (e->uidf) {
case UID_NOT_PRESENT:
@@ -1366,8 +1367,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
case UID_IS_CHARACTER:
- memcpy(uid, (u8 *)(&e->uid), ACPIHID_UID_LEN -
1);
- uid[ACPIHID_UID_LEN - 1] = '\0';
+ memcpy(uid, &e->uid, e->uidl);
+ uid[e->uidl] = '\0';
break;
default:
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5.4 012/111] iommu/amd: Fix over-read of ACPI UID from IVRS table
From: Alexander Monakov
[ Upstream commit e461b8c991b9202b007ea2059d953e264240b0c9 ]
IVRS parsing code always tries to read 255 bytes from memory when
retrieving ACPI device path, and makes an assumption that firmware
provides a zero-terminated string. Both of those are bugs: the entry
is likely to be shorter than 255 bytes, and zero-termination is not
guaranteed.
With Acer SF314-42 firmware these issues manifest visibly in dmesg:
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1...
The first three lines show how the code over-reads adjacent table
entries into the UID, and in the last line it even reads garbage data
beyond the end of the IVRS table itself.
Since each entry has the length of the UID (uidl member of ivhd_entry
struct), use that for memcpy, and manually add a zero terminator.
Avoid zero-filling hid and uid arrays up front, and instead ensure
the uid array is always zero-terminated. No change needed for the hid
array, as it was already properly zero-terminated.
Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type
HID")
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
Link: https://lore.kernel.org/r/20200511102352.1831-1-amona...@ispras.ru
Signed-off-by: Joerg Roedel
Signed-off-by: Sasha Levin
---
drivers/iommu/amd_iommu_init.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index ef14b00fa94b..135ae5222cf3 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1331,8 +1331,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
}
case IVHD_DEV_ACPI_HID: {
u16 devid;
- u8 hid[ACPIHID_HID_LEN] = {0};
- u8 uid[ACPIHID_UID_LEN] = {0};
+ u8 hid[ACPIHID_HID_LEN];
+ u8 uid[ACPIHID_UID_LEN];
int ret;
if (h->type != 0x40) {
@@ -1349,6 +1349,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
+ uid[0] = '\0';
switch (e->uidf) {
case UID_NOT_PRESENT:
@@ -1363,8 +1364,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
case UID_IS_CHARACTER:
- memcpy(uid, (u8 *)(&e->uid), ACPIHID_UID_LEN -
1);
- uid[ACPIHID_UID_LEN - 1] = '\0';
+ memcpy(uid, &e->uid, e->uidl);
+ uid[e->uidl] = '\0';
break;
default:
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5.6 014/126] iommu/amd: Fix over-read of ACPI UID from IVRS table
From: Alexander Monakov
[ Upstream commit e461b8c991b9202b007ea2059d953e264240b0c9 ]
IVRS parsing code always tries to read 255 bytes from memory when
retrieving ACPI device path, and makes an assumption that firmware
provides a zero-terminated string. Both of those are bugs: the entry
is likely to be shorter than 255 bytes, and zero-termination is not
guaranteed.
With Acer SF314-42 firmware these issues manifest visibly in dmesg:
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR0\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR1\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR2\xf0\xa5, rdevid:160
AMD-Vi: ivrs, add hid:AMDI0020, uid:\_SB.FUR3>\x83e\x8d\x9a\xd1...
The first three lines show how the code over-reads adjacent table
entries into the UID, and in the last line it even reads garbage data
beyond the end of the IVRS table itself.
Since each entry has the length of the UID (uidl member of ivhd_entry
struct), use that for memcpy, and manually add a zero terminator.
Avoid zero-filling hid and uid arrays up front, and instead ensure
the uid array is always zero-terminated. No change needed for the hid
array, as it was already properly zero-terminated.
Fixes: 2a0cb4e2d423c ("iommu/amd: Add new map for storing IVHD dev entry type
HID")
Signed-off-by: Alexander Monakov
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
Link: https://lore.kernel.org/r/20200511102352.1831-1-amona...@ispras.ru
Signed-off-by: Joerg Roedel
Signed-off-by: Sasha Levin
---
drivers/iommu/amd_iommu_init.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 2b9a67ecc6ac..5b81fd16f5fa 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1329,8 +1329,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
}
case IVHD_DEV_ACPI_HID: {
u16 devid;
- u8 hid[ACPIHID_HID_LEN] = {0};
- u8 uid[ACPIHID_UID_LEN] = {0};
+ u8 hid[ACPIHID_HID_LEN];
+ u8 uid[ACPIHID_UID_LEN];
int ret;
if (h->type != 0x40) {
@@ -1347,6 +1347,7 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
}
+ uid[0] = '\0';
switch (e->uidf) {
case UID_NOT_PRESENT:
@@ -1361,8 +1362,8 @@ static int __init init_iommu_from_acpi(struct amd_iommu
*iommu,
break;
case UID_IS_CHARACTER:
- memcpy(uid, (u8 *)(&e->uid), ACPIHID_UID_LEN -
1);
- uid[ACPIHID_UID_LEN - 1] = '\0';
+ memcpy(uid, &e->uid, e->uidl);
+ uid[e->uidl] = '\0';
break;
default:
--
2.25.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/2] Introduce PCI_FIXUP_IOMMU
On Tue, May 26, 2020 at 07:49:07PM +0800, Zhangfei Gao wrote: > Some platform devices appear as PCI but are actually on the AMBA bus, Why would these devices not just show up on the AMBA bus and use all of that logic instead of being a PCI device and having to go through odd fixes like this? thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] iommu: calling pci_fixup_iommu in iommu_fwspec_init
On Tue, May 26, 2020 at 07:49:09PM +0800, Zhangfei Gao wrote: > Calling pci_fixup_iommu in iommu_fwspec_init, which alloc > iommu_fwnode. Some platform devices appear as PCI but are > actually on the AMBA bus, and they need fixup in > drivers/pci/quirks.c handling iommu_fwnode. > So calling pci_fixup_iommu after iommu_fwnode is allocated. > > Signed-off-by: Zhangfei Gao > --- > drivers/iommu/iommu.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 7b37542..fb84c42 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -2418,6 +2418,10 @@ int iommu_fwspec_init(struct device *dev, struct > fwnode_handle *iommu_fwnode, > fwspec->iommu_fwnode = iommu_fwnode; > fwspec->ops = ops; > dev_iommu_fwspec_set(dev, fwspec); > + > + if (dev_is_pci(dev)) > + pci_fixup_device(pci_fixup_iommu, to_pci_dev(dev)); Why can't the caller do this as it "knows" it is a PCI device at that point in time, right? thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] PCI: Introduce PCI_FIXUP_IOMMU
On Tue, May 26, 2020 at 11:09:57PM +0800, Zhangfei Gao wrote: > Hi, Christoph > > On 2020/5/26 下午10:46, Christoph Hellwig wrote: > > On Tue, May 26, 2020 at 07:49:08PM +0800, Zhangfei Gao wrote: > > > Some platform devices appear as PCI but are actually on the AMBA bus, > > > and they need fixup in drivers/pci/quirks.c handling iommu_fwnode. > > > Here introducing PCI_FIXUP_IOMMU, which is called after iommu_fwnode > > > is allocated, instead of reusing PCI_FIXUP_FINAL since it will slow > > > down iommu probing as all devices in fixup final list will be > > > reprocessed. > > Who is going to use this? I don't see a single user in the series. > We will add iommu fixup in drivers/pci/quirks.c, handling > > fwspec->can_stall, which is introduced in > > https://www.spinics.net/lists/linux-pci/msg94559.html > > Unfortunately, the patch does not catch v5.8, so we have to wait. > And we want to check whether this is a right method to solve this issue. We can't take new apis without a real user, so please submit them all at once. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Mon, Jun 15, 2020 at 06:17:42PM -0700, Rajat Jain wrote:
> This is needed to allow the userspace to determine when an untrusted
> device has been added, and thus allowing it to bind the driver manually
> to it, if it so wishes. This is being done as part of the approach
> discussed at https://lkml.org/lkml/2020/6/9/1331
>
> Signed-off-by: Rajat Jain
> ---
> drivers/pci/pci-sysfs.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 6d78df981d41a..574e9c613ba26 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -50,6 +50,7 @@ pci_config_attr(subsystem_device, "0x%04x\n");
> pci_config_attr(revision, "0x%02x\n");
> pci_config_attr(class, "0x%06x\n");
> pci_config_attr(irq, "%u\n");
> +pci_config_attr(untrusted, "%u\n");
>
> static ssize_t broken_parity_status_show(struct device *dev,
>struct device_attribute *attr,
> @@ -608,6 +609,7 @@ static struct attribute *pci_dev_attrs[] = {
> #endif
> &dev_attr_driver_override.attr,
> &dev_attr_ari_enabled.attr,
> + &dev_attr_untrusted.attr,
> NULL,
> };
You also need a Documentation/ABI/ update for this new file.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Wed, Jun 17, 2020 at 12:53:03PM -0700, Rajat Jain wrote: > Hi Greg, Christoph, > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig wrote: > > > > On Tue, Jun 16, 2020 at 12:27:35PM -0700, Rajat Jain wrote: > > > Need clarification. The flag "untrusted" is currently a part of > > > pci_dev struct, and is populated within the PCI subsystem. > > > > Yes, and that is the problem. > > > > > > > > 1) Is your suggestion to move this flag as well as the attribute to > > > device core (in "struct device")? This would allow other buses to > > > populate/use this flag if they want. By default it'll be set to 0 for > > > all devices (PCI subsystem will populate it based on platform info, > > > like it does today). > > > > > > OR > > > > > > 2) Are you suggesting to keep the "untrusted" flag within PCI, but > > > attach the sysfs attribute to the base device? (&pci_dev->dev)? > > > > (1). As for IOMMUs and userspace policy it really should not matter > > what bus a device is on if it is external and not trustworthy. > > Sure. I can move the flag to the "struct device" (and likely call > it "external" instead of "untrusted" so as to make it suitable for > more use cases later). The buses can fill this up if they know which > devices are external and which ones are not (otherwise it will be 0 by > default). The PCI can fill this up like it does today, from platform > info (ACPI / Device tree). Greg, how does this sound? That's fine, convert USB over to use it at the same time if you get the chance :) thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain wrote: > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig > > wrote: > > ... > > > (and likely call it "external" instead of "untrusted". > > Which is not okay. 'External' to what? 'untrusted' has been carefully > chosen by the meaning of it. > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > tables, but I can replace it. Then your ACPI tables should show this, there is an attribute for it, right? > This is only one example. Or if firmware of some device is altered, > and it's internal (whatever it means) is it trusted or not? That is what people are using policy for today, if you object to this, please bring it up to those developers :) > So, please leave it as is (I mean name). firmware today exports this attribute, why do you not want userspace to also know it? Trust is different, yes, don't get the two mixed up please. That should be a different sysfs attribute for obvious reasons. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Thu, Jun 18, 2020 at 12:14:41PM +0300, Andy Shevchenko wrote: > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman > wrote: > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote: > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain wrote: > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig > > > > wrote: > > > > > > ... > > > > > > > (and likely call it "external" instead of "untrusted". > > > > > > Which is not okay. 'External' to what? 'untrusted' has been carefully > > > chosen by the meaning of it. > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI > > > tables, but I can replace it. > > > > Then your ACPI tables should show this, there is an attribute for it, > > right? > > There is a _PLD() method, but it's for the USB devices (or optional > for others, I don't remember by heart). So, most of the ACPI tables, > alas, don't show this. There is something like this for PCI as well, otherwise they wouldn't be getting this info from "the ether" :) thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data
On Wed, Aug 08, 2012 at 09:24:32AM +0300, Hiroshi Doyu wrote:
> Add __debugfs_create_dir(), which takes data passed from caller.
Why?
> Signed-off-by: Hiroshi Doyu
> ---
> fs/debugfs/inode.c |7 ---
> include/linux/debugfs.h |9 -
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 4733eab..423df9f 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -387,7 +387,7 @@ struct dentry *debugfs_create_file(const char *name,
> umode_t mode,
> EXPORT_SYMBOL_GPL(debugfs_create_file);
>
> /**
> - * debugfs_create_dir - create a directory in the debugfs filesystem
> + * __debugfs_create_dir - create a directory in the debugfs filesystem
> * @name: a pointer to a string containing the name of the directory to
> *create.
> * @parent: a pointer to the parent dentry for this file. This should be a
> @@ -404,10 +404,11 @@ EXPORT_SYMBOL_GPL(debugfs_create_file);
> * If debugfs is not enabled in the kernel, the value -%ENODEV will be
> * returned.
> */
> -struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
> +struct dentry *__debugfs_create_dir(const char *name, struct dentry *parent,
> + void *data)
> {
> return __create_file(name, S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO,
> -parent, NULL, NULL);
> +parent, data, NULL);
> }
> EXPORT_SYMBOL_GPL(debugfs_create_dir);
You can't export a symbol that doesn't exist anymore.
What are you trying to do here? This patch doesn't look right at all.
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] debugfs: Allow debugfs_create_dir() to take data
On Wed, Aug 15, 2012 at 08:40:08AM +0300, Hiroshi Doyu wrote: > On Thu, 9 Aug 2012 14:56:24 +0200 > Hiroshi Doyu wrote: > > > Hi Greg, Felipe, > > > > On Wed, 8 Aug 2012 15:34:27 +0200 > > Greg Kroah-Hartman wrote: > > > > > On Wed, Aug 08, 2012 at 09:24:32AM +0300, Hiroshi Doyu wrote: > > > > Add __debugfs_create_dir(), which takes data passed from caller. > > > > > > Why? > > > > > > > Signed-off-by: Hiroshi Doyu > > > > --- > > > > fs/debugfs/inode.c |7 --- > > > > include/linux/debugfs.h |9 - > > > > 2 files changed, 12 insertions(+), 4 deletions(-) > > . > > > What are you trying to do here? This patch doesn't look right at all. > > > > I missed to send the cover letter of this patch series to LKML, which > > explained the background. I attached that cover letter below. Please > > read the following explanation too. > > Any chance to get some feedback on this? I still don't like it, and then there's the basic fact that I'm pretty sure you broke the build with the patch, but who's noticing little things like that :) > I'm also sending another version of patch, which just uses > debgufs_create_dir() as Felipe suggested, in-reply-to this mail. I'd prefer not to change debugfs for this, so if you don't do that, I don't object to your driver-specific patch. greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 04/29] drivers: base: add notifier for failed driver bind
On Tue, Aug 05, 2014 at 12:47:32PM +0200, Marek Szyprowski wrote: > This patch adds support for getting a notify for failed device driver > bind, so all the items done in BUS_NOTIFY_BIND_DRIVER event can be > cleaned if the driver fails to bind. But doesn't the bus know if the driver fails to bind, so why would a notifier be needed here? Can't it unwind any problems then? > Signed-off-by: Marek Szyprowski > --- > drivers/base/dd.c | 10 +++--- > include/linux/device.h | 4 +++- > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index e4ffbcf..541a41f 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -237,10 +237,14 @@ static int driver_sysfs_add(struct device *dev) > return ret; > } > > -static void driver_sysfs_remove(struct device *dev) > +static void driver_sysfs_remove(struct device *dev, int failed) I _hate_ having functions with a flag that does something different depending on it. If you _really_ need this, just make the notifier call before calling this function, which should work just fine, right? thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 0/2] iommu/vt-d: Keep RMRR mappings around on driver unbind
On Tue, Sep 30, 2014 at 01:02:01PM +0200, Joerg Roedel wrote: > Hi, > > here is a patch-set to fix an issue recently discovered when > the Intel IOMMU is in use with devices that need RMRR > mappings. > > The problem is that the RMRR mappings are destroyed when the > device driver is unbound from the device, causing DMAR > faults. > > To solve this problem a device driver core change is > necessary to catch the right point in time for the IOMMU > code to destroy any mappings for a device. > > With this patch-set the RMRR mappings are only destroyed > when the device is actually removed from the system. > > Please review. I have no objection to these, do you want me to take them through my tree? Or if they are going through some other one feel free to add: Acked-by: Greg Kroah-Hartman To the first patch. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: next take at setting up a dma mask by default for platform devices v2
On Fri, Aug 16, 2019 at 08:24:29AM +0200, Christoph Hellwig wrote: > Hi all, > > this is another attempt to make sure the dma_mask pointer is always > initialized for platform devices. Not doing so lead to lots of > boilerplate code, and makes platform devices different from all our > major busses like PCI where we always set up a dma_mask. In the long > run this should also help to eventually make dma_mask a scalar value > instead of a pointer and remove even more cruft. > > The bigger blocker for this last time was the fact that the usb > subsystem uses the presence or lack of a dma_mask to check if the core > should do dma mapping for the driver, which is highly unusual. So we > fix this first. Note that this has some overlap with the pending > desire to use the proper dma_mmap_coherent helper for mapping usb > buffers. The first two patches have already been queued up by Greg > and are only included for completeness. Note to everyone. The first two patches in this series is already in 5.3-rc5. I've applied the rest of the series to my usb-next branch (with the 6th patch landing there later today.) They are scheduled to be merge to Linus in 5.4-rc1. Christoph, thanks so much for these cleanups. greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] dma-mapping: Move vmap address checks into dma_map_single()
On Fri, Oct 04, 2019 at 02:28:16PM -0700, Kees Cook wrote:
> As we've seen from USB and other areas, we need to always do runtime
> checks for DMA operating on memory regions that might be remapped. This
> moves the existing checks from USB into dma_map_single(), but leaves
> the slightly heavier checks as they are.
>
> Suggested-by: Laura Abbott
> Signed-off-by: Kees Cook
> ---
> v2: Only add is_vmalloc_addr()
> v1: https://lore.kernel.org/lkml/201910021341.7819A660@keescook
> ---
> drivers/usb/core/hcd.c | 8 +---
> include/linux/dma-mapping.h | 7 +++
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index f225eaa98ff8..281568d464f9 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1410,10 +1410,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd,
> struct urb *urb,
> if (hcd->self.uses_pio_for_control)
> return ret;
> if (hcd_uses_dma(hcd)) {
> - if (is_vmalloc_addr(urb->setup_packet)) {
> - WARN_ONCE(1, "setup packet is not dma
> capable\n");
> - return -EAGAIN;
> - } else if (object_is_on_stack(urb->setup_packet)) {
> + if (object_is_on_stack(urb->setup_packet)) {
> WARN_ONCE(1, "setup packet is on stack\n");
> return -EAGAIN;
> }
> @@ -1479,9 +1476,6 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct
> urb *urb,
> ret = -EAGAIN;
> else
> urb->transfer_flags |= URB_DMA_MAP_PAGE;
> - } else if (is_vmalloc_addr(urb->transfer_buffer)) {
> - WARN_ONCE(1, "transfer buffer not dma
> capable\n");
> - ret = -EAGAIN;
> } else if (object_is_on_stack(urb->transfer_buffer)) {
> WARN_ONCE(1, "transfer buffer is on stack\n");
> ret = -EAGAIN;
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..12dbd07f74f2 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -583,6 +583,13 @@ static inline unsigned long
> dma_get_merge_boundary(struct device *dev)
> static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> + /* DMA must never operate on areas that might be remapped. */
> + if (WARN_ONCE(is_vmalloc_addr(ptr),
> + "%s %s: driver maps %lu bytes from vmalloc area\n",
> + dev ? dev_driver_string(dev) : "unknown driver",
> + dev ? dev_name(dev) : "unknown device", size))
If you use dev_warn() here you get all of that "unknown driver/device"
checking and handling set properly. And it's in the "standard" format
that userspace tools know how to check.
thanks,
greg k-h
Re: [PATCH v3 1/2] dma-mapping: Add vmap checks to dma_map_single()
On Thu, Oct 10, 2019 at 03:28:28PM -0700, Kees Cook wrote:
> As we've seen from USB and other areas[1], we need to always do runtime
> checks for DMA operating on memory regions that might be remapped. This
> adds vmap checks (similar to those already in USB but missing in other
> places) into dma_map_single() so all callers benefit from the checking.
>
> [1] https://git.kernel.org/linus/3840c5b78803b2b6cc1ff820100a74a092c40cbb
>
> Suggested-by: Laura Abbott
> Signed-off-by: Kees Cook
> ---
> include/linux/dma-mapping.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..ff4e91c66f44 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -583,6 +583,12 @@ static inline unsigned long
> dma_get_merge_boundary(struct device *dev)
> static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> + /* DMA must never operate on areas that might be remapped. */
> + if (unlikely(is_vmalloc_addr(ptr))) {
> + dev_warn_once(dev, "bad map: %zu bytes in vmalloc\n", size);
Can we get a bit better error text here? In USB we were at least giving
people a hint as to what went wrong, "bad map" might not really make
that much sense to a USB developer as to what they needed to do to fix
this issue.
Other than that minor nit, I have no objection to this series, thanks
for fixing this up!
greg k-h
Re: [PATCH v4 1/2] dma-mapping: Add vmap checks to dma_map_single()
On Tue, Oct 29, 2019 at 02:34:22PM -0700, Kees Cook wrote:
> As we've seen from USB and other areas[1], we need to always do runtime
> checks for DMA operating on memory regions that might be remapped. This
> adds vmap checks (similar to those already in USB but missing in other
> places) into dma_map_single() so all callers benefit from the checking.
>
> [1] https://git.kernel.org/linus/3840c5b78803b2b6cc1ff820100a74a092c40cbb
>
> Suggested-by: Laura Abbott
> Signed-off-by: Kees Cook
> ---
> include/linux/dma-mapping.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..54de3c496407 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -583,6 +583,12 @@ static inline unsigned long
> dma_get_merge_boundary(struct device *dev)
> static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> size_t size, enum dma_data_direction dir, unsigned long attrs)
> {
> + /* DMA must never operate on areas that might be remapped. */
> + if (dev_WARN_ONCE(dev, is_vmalloc_addr(ptr),
> + "wanted %zu bytes mapped in vmalloc\n", size)) {
> + return DMA_MAPPING_ERROR;
> + }
That's a very odd error string, I know if I saw it for the first time, I
would have no idea what it meant. The USB message at least gives you a
bit more context as to what went wrong and how to fix it.
How about something like "Memory is not DMA capabable, please fix the
allocation of it to be correct", or "non-dma-able memory was attempted
to be mapped, but this is impossible to to" or something else.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] dma-mapping: Add vmap checks to dma_map_single()
On Wed, Oct 30, 2019 at 07:09:21PM +0100, Christoph Hellwig wrote:
> On Wed, Oct 30, 2019 at 10:18:49AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Oct 29, 2019 at 02:34:22PM -0700, Kees Cook wrote:
> > > As we've seen from USB and other areas[1], we need to always do runtime
> > > checks for DMA operating on memory regions that might be remapped. This
> > > adds vmap checks (similar to those already in USB but missing in other
> > > places) into dma_map_single() so all callers benefit from the checking.
> > >
> > > [1] https://git.kernel.org/linus/3840c5b78803b2b6cc1ff820100a74a092c40cbb
> > >
> > > Suggested-by: Laura Abbott
> > > Signed-off-by: Kees Cook
> > > ---
> > > include/linux/dma-mapping.h | 6 ++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > > index 4a1c4fca475a..54de3c496407 100644
> > > --- a/include/linux/dma-mapping.h
> > > +++ b/include/linux/dma-mapping.h
> > > @@ -583,6 +583,12 @@ static inline unsigned long
> > > dma_get_merge_boundary(struct device *dev)
> > > static inline dma_addr_t dma_map_single_attrs(struct device *dev, void
> > > *ptr,
> > > size_t size, enum dma_data_direction dir, unsigned long attrs)
> > > {
> > > + /* DMA must never operate on areas that might be remapped. */
> > > + if (dev_WARN_ONCE(dev, is_vmalloc_addr(ptr),
> > > + "wanted %zu bytes mapped in vmalloc\n", size)) {
> > > + return DMA_MAPPING_ERROR;
> > > + }
> >
> > That's a very odd error string, I know if I saw it for the first time, I
> > would have no idea what it meant. The USB message at least gives you a
> > bit more context as to what went wrong and how to fix it.
> >
> > How about something like "Memory is not DMA capabable, please fix the
> > allocation of it to be correct", or "non-dma-able memory was attempted
> > to be mapped, but this is impossible to to" or something else.
>
> I've fixed the message to "rejecting DMA map of vmalloc memory" and
> applied the patch.
Looks good! You can apply patch 2/2 as well if you want to take that
through your tree too.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/2] usb: core: Remove redundant vmap checks
On Tue, Oct 29, 2019 at 02:34:23PM -0700, Kees Cook wrote: > Now that the vmap area checks are being performed in the DMA > infrastructure directly, there is no need to repeat them in USB. > > Signed-off-by: Kees Cook > --- > drivers/usb/core/hcd.c | 8 +--- > 1 file changed, 1 insertion(+), 7 deletions(-) Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] dma-mapping: Add vmap checks to dma_map_single()
On Wed, Oct 30, 2019 at 08:45:32PM +0100, Christoph Hellwig wrote: > On Wed, Oct 30, 2019 at 08:26:40PM +0100, Greg Kroah-Hartman wrote: > > Looks good! You can apply patch 2/2 as well if you want to take that > > through your tree too. > > I can do that, I'll just need a formal ACK from you. Now sent, thanks. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/16] drivers/iommu: Take a ref to the IOMMU driver prior to ->add_device()
On Thu, Dec 19, 2019 at 12:03:41PM +, Will Deacon wrote:
> To avoid accidental removal of an active IOMMU driver module, take a
> reference to the driver module in 'iommu_probe_device()' immediately
> prior to invoking the '->add_device()' callback and hold it until the
> after the device has been removed by '->remove_device()'.
>
> Suggested-by: Joerg Roedel
> Signed-off-by: Will Deacon
> ---
> drivers/iommu/iommu.c | 19 +--
> include/linux/iommu.h | 4 +++-
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7c92197d53f3..bc8edf90e729 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> static struct kset *iommu_group_kset;
> @@ -185,10 +186,21 @@ int iommu_probe_device(struct device *dev)
> if (!iommu_get_dev_param(dev))
> return -ENOMEM;
>
> + if (!try_module_get(ops->owner)) {
> + ret = -EINVAL;
> + goto err_free_dev_param;
> + }
> +
> ret = ops->add_device(dev);
> if (ret)
> - iommu_free_dev_param(dev);
> + goto err_module_put;
> +
> + return 0;
>
> +err_module_put:
> + module_put(ops->owner);
> +err_free_dev_param:
> + iommu_free_dev_param(dev);
> return ret;
> }
>
> @@ -199,7 +211,10 @@ void iommu_release_device(struct device *dev)
> if (dev->iommu_group)
> ops->remove_device(dev);
>
> - iommu_free_dev_param(dev);
> + if (dev->iommu_param) {
> + module_put(ops->owner);
> + iommu_free_dev_param(dev);
> + }
> }
>
> static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f2223cbb5fd5..e9f94d3f7a04 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -246,9 +246,10 @@ struct iommu_iotlb_gather {
> * @sva_get_pasid: Get PASID associated to a SVA handle
> * @page_response: handle page request response
> * @cache_invalidate: invalidate translation caches
> - * @pgsize_bitmap: bitmap of all possible supported page sizes
> * @sva_bind_gpasid: bind guest pasid and mm
> * @sva_unbind_gpasid: unbind guest pasid and mm
> + * @pgsize_bitmap: bitmap of all possible supported page sizes
> + * @owner: Driver module providing these ops
> */
> struct iommu_ops {
> bool (*capable)(enum iommu_cap);
> @@ -318,6 +319,7 @@ struct iommu_ops {
> int (*sva_unbind_gpasid)(struct device *dev, int pasid);
>
> unsigned long pgsize_bitmap;
> + struct module *owner;
Everyone is always going to forget to set this field. I don't think you
even set it for all of the different iommu_ops possible in this series,
right?
The "trick" we did to keep people from having to remember this is to do
what we did for the bus registering functions.
Look at pci_register_driver in pci.h:
#define pci_register_driver(driver) \
__pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
Then we set the .owner field in the "real" __pci_register_driver() call.
Same thing for USB and lots, if not all, other driver register
functions.
You can do the same thing here, and I would recommend it.
No need to stop this series from happening now, just an add-on that is
easy to make to ensure that no one ever forgets to set this field
properly.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 00/16] iommu: Permit modular builds of ARM SMMU[v3] drivers
On Thu, Dec 19, 2019 at 12:03:36PM +, Will Deacon wrote:
> Hi all,
>
> This is version four of the patches I previously posted here:
>
> v1: https://lore.kernel.org/lkml/20191030145112.19738-1-w...@kernel.org/
> v2: https://lore.kernel.org/lkml/20191108151608.20932-1-w...@kernel.org
> v3: https://lore.kernel.org/lkml/20191121114918.2293-1-w...@kernel.org
>
> Changes since v3 include:
>
> * Based on v5.5-rc1
> * ACPI/IORT support (thanks to Ard)
> * Export pci_{enable,disable}_ats() (thanks to Greg)
> * Added review tags
>
> I tested this on AMD Seattle by loading arm-smmu-mod.ko from the initrd.
All look good to me!
Reviewed-by: Greg Kroah-Hartman
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5.4 371/434] iommu/vt-d: Allocate reserved region for ISA with correct permission
From: Jerry Snitselaar
commit cde9319e884eb6267a0df446f3c131fe1108defb upstream.
Currently the reserved region for ISA is allocated with no
permissions. If a dma domain is being used, mapping this region will
fail. Set the permissions to DMA_PTE_READ|DMA_PTE_WRITE.
Cc: Joerg Roedel
Cc: Lu Baolu
Cc: iommu@lists.linux-foundation.org
Cc: sta...@vger.kernel.org # v5.3+
Fixes: d850c2ee5fe2 ("iommu/vt-d: Expose ISA direct mapping region via
iommu_get_resv_regions")
Signed-off-by: Jerry Snitselaar
Acked-by: Lu Baolu
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5697,7 +5697,7 @@ static void intel_iommu_get_resv_regions
struct pci_dev *pdev = to_pci_dev(device);
if ((pdev->class >> 8) == PCI_CLASS_BRIDGE_ISA) {
- reg = iommu_alloc_resv_region(0, 1UL << 24, 0,
+ reg = iommu_alloc_resv_region(0, 1UL << 24, prot,
IOMMU_RESV_DIRECT_RELAXABLE);
if (reg)
list_add_tail(®->list, head);
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5.4 368/434] iommu: set group default domain before creating direct mappings
From: Jerry Snitselaar
commit d360211524bece6db9920f32c91808235290b51c upstream.
iommu_group_create_direct_mappings uses group->default_domain, but
right after it is called, request_default_domain_for_dev calls
iommu_domain_free for the default domain, and sets the group default
domain to a different domain. Move the
iommu_group_create_direct_mappings call to after the group default
domain is set, so the direct mappings get associated with that domain.
Cc: Joerg Roedel
Cc: Lu Baolu
Cc: iommu@lists.linux-foundation.org
Cc: sta...@vger.kernel.org
Fixes: 7423e01741dd ("iommu: Add API to request DMA domain for device")
Signed-off-by: Jerry Snitselaar
Reviewed-by: Lu Baolu
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/iommu.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2221,13 +2221,13 @@ request_default_domain_for_dev(struct de
goto out;
}
- iommu_group_create_direct_mappings(group, dev);
-
/* Make the domain the default for this group */
if (group->default_domain)
iommu_domain_free(group->default_domain);
group->default_domain = domain;
+ iommu_group_create_direct_mappings(group, dev);
+
dev_info(dev, "Using iommu %s mapping\n",
type == IOMMU_DOMAIN_DMA ? "dma" : "direct");
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: dma-direct: don't check swiotlb=force in dma_direct_map_resource
On Tue, Jan 07, 2020 at 06:18:28PM +, Robin Murphy wrote:
> On 07/01/2020 5:38 pm, Naresh Kamboju wrote:
> > Following build error on stable-rc 5.4.9-rc1 for arm architecture.
> >
> > dma/direct.c: In function 'dma_direct_possible':
> > dma/direct.c:329:3: error: too many arguments to function 'dma_capable'
> > dma_capable(dev, dma_addr, size, true);
> > ^~~
>
> Not sure that $SUBJECT comes into it at all, but by the look of it I guess
> "dma-direct: exclude dma_direct_map_resource from the min_low_pfn check"
> implicitly depends on 130c1ccbf553 ("dma-direct: unify the dma_capable
> definitions") too.
Ugh, good catch. I'll drop these patches, they don't look ok for stable
at this point in time.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 05/16] drivers/iommu: Take a ref to the IOMMU driver prior to ->add_device()
On Thu, Jan 09, 2020 at 02:16:03PM +, Will Deacon wrote:
> Hi Greg,
>
> On Thu, Dec 19, 2019 at 03:44:37PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Dec 19, 2019 at 12:03:41PM +, Will Deacon wrote:
> > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > index f2223cbb5fd5..e9f94d3f7a04 100644
> > > --- a/include/linux/iommu.h
> > > +++ b/include/linux/iommu.h
> > > @@ -246,9 +246,10 @@ struct iommu_iotlb_gather {
> > > * @sva_get_pasid: Get PASID associated to a SVA handle
> > > * @page_response: handle page request response
> > > * @cache_invalidate: invalidate translation caches
> > > - * @pgsize_bitmap: bitmap of all possible supported page sizes
> > > * @sva_bind_gpasid: bind guest pasid and mm
> > > * @sva_unbind_gpasid: unbind guest pasid and mm
> > > + * @pgsize_bitmap: bitmap of all possible supported page sizes
> > > + * @owner: Driver module providing these ops
> > > */
> > > struct iommu_ops {
> > > bool (*capable)(enum iommu_cap);
> > > @@ -318,6 +319,7 @@ struct iommu_ops {
> > > int (*sva_unbind_gpasid)(struct device *dev, int pasid);
> > >
> > > unsigned long pgsize_bitmap;
> > > + struct module *owner;
> >
> > Everyone is always going to forget to set this field. I don't think you
> > even set it for all of the different iommu_ops possible in this series,
> > right?
>
> I only initialised the field for those drivers which can actually be built
> as a module, but I take your point about this being error-prone.
>
> > The "trick" we did to keep people from having to remember this is to do
> > what we did for the bus registering functions.
> >
> > Look at pci_register_driver in pci.h:
> > #define pci_register_driver(driver) \
> > __pci_register_driver(driver, THIS_MODULE, KBUILD_MODNAME)
> >
> > Then we set the .owner field in the "real" __pci_register_driver() call.
> >
> > Same thing for USB and lots, if not all, other driver register
> > functions.
> >
> > You can do the same thing here, and I would recommend it.
>
> Yes, that makes sense, cheers. Diff below. I'll send it to Joerg along
> with some other SMMU patches that have come in since the holiday.
>
> Will
>
> --->8
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 03dc97842875..e82997a705a8 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -2733,7 +2733,6 @@ static struct iommu_ops arm_smmu_ops = {
> .get_resv_regions = arm_smmu_get_resv_regions,
> .put_resv_regions = arm_smmu_put_resv_regions,
> .pgsize_bitmap = -1UL, /* Restricted during device attach */
> - .owner = THIS_MODULE,
> };
>
> /* Probing and initialisation functions */
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5ef1f2e100d7..93d332423f6f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1623,7 +1623,6 @@ static struct iommu_ops arm_smmu_ops = {
> .get_resv_regions = arm_smmu_get_resv_regions,
> .put_resv_regions = arm_smmu_put_resv_regions,
> .pgsize_bitmap = -1UL, /* Restricted during device attach */
> - .owner = THIS_MODULE,
> };
>
> static void arm_smmu_device_reset(struct arm_smmu_device *smmu)
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e9f94d3f7a04..90007c92ad2d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -388,12 +388,19 @@ void iommu_device_sysfs_remove(struct iommu_device
> *iommu);
> int iommu_device_link(struct iommu_device *iommu, struct device *link);
> void iommu_device_unlink(struct iommu_device *iommu, struct device *link);
>
> -static inline void iommu_device_set_ops(struct iommu_device *iommu,
> - const struct iommu_ops *ops)
> +static inline void __iommu_device_set_ops(struct iommu_device *iommu,
> + const struct iommu_ops *ops)
> {
> iommu->ops = ops;
> }
>
> +#define iommu_device_set_ops(iommu, ops) \
> +do { \
> + struct iommu_ops *__ops = (struct iommu_ops *)(ops);\
> + __ops->owner = THIS_MODULE; \
> + __iommu_device_set_ops(iommu, __ops); \
> +} while (0)
> +
> static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
> struct fwnode_handle *fwnode)
> {
Looks good:
Reviewed-by: Greg Kroah-Hartman
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 2/4] uacce: add uacce driver
On Sat, Jan 11, 2020 at 10:48:37AM +0800, Zhangfei Gao wrote:
> +static int uacce_fops_open(struct inode *inode, struct file *filep)
> +{
> + struct uacce_mm *uacce_mm = NULL;
> + struct uacce_device *uacce;
> + struct uacce_queue *q;
> + int ret = 0;
> +
> + uacce = xa_load(&uacce_xa, iminor(inode));
> + if (!uacce)
> + return -ENODEV;
> +
> + if (!try_module_get(uacce->parent->driver->owner))
> + return -ENODEV;
Why are you trying to grab the module reference of the parent device?
Why is that needed and what is that going to help with here?
This shouldn't be needed as the module reference of the owner of the
fileops for this module is incremented, and the "parent" module depends
on this module, so how could it be unloaded without this code being
unloaded?
Yes, if you build this code into the kernel and the "parent" driver is a
module, then you will not have a reference, but when you remove that
parent driver the device will be removed as it has to be unregistered
before that parent driver can be removed from the system, right?
Or what am I missing here?
> +static void uacce_release(struct device *dev)
> +{
> + struct uacce_device *uacce = to_uacce_device(dev);
> +
> + kfree(uacce);
> + uacce = NULL;
That line didn't do anything :)
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 2/4] uacce: add uacce driver
On Mon, Jan 13, 2020 at 11:34:55AM +0800, zhangfei wrote:
> Hi, Greg
>
> Thanks for the review.
>
> On 2020/1/12 上午3:40, Greg Kroah-Hartman wrote:
> > On Sat, Jan 11, 2020 at 10:48:37AM +0800, Zhangfei Gao wrote:
> > > +static int uacce_fops_open(struct inode *inode, struct file *filep)
> > > +{
> > > + struct uacce_mm *uacce_mm = NULL;
> > > + struct uacce_device *uacce;
> > > + struct uacce_queue *q;
> > > + int ret = 0;
> > > +
> > > + uacce = xa_load(&uacce_xa, iminor(inode));
> > > + if (!uacce)
> > > + return -ENODEV;
> > > +
> > > + if (!try_module_get(uacce->parent->driver->owner))
> > > + return -ENODEV;
> > Why are you trying to grab the module reference of the parent device?
> > Why is that needed and what is that going to help with here?
> >
> > This shouldn't be needed as the module reference of the owner of the
> > fileops for this module is incremented, and the "parent" module depends
> > on this module, so how could it be unloaded without this code being
> > unloaded?
> >
> > Yes, if you build this code into the kernel and the "parent" driver is a
> > module, then you will not have a reference, but when you remove that
> > parent driver the device will be removed as it has to be unregistered
> > before that parent driver can be removed from the system, right?
> >
> > Or what am I missing here?
> The refcount here is preventing rmmod "parent" module after fd is opened,
> since user driver has mmap kernel memory to user space, like mmio, which may
> still in-use.
>
> With the refcount protection, rmmod "parent" module will fail until
> application free the fd.
> log like: rmmod: ERROR: Module hisi_zip is in use
But if the "parent" module is to be unloaded, it has to unregister the
"child" device and that will call the destructor in here and then you
will tear everything down and all should be good.
There's no need to "forbid" a module from being unloaded, even if it is
being used. Look at all networking drivers, they work that way, right?
> > > +static void uacce_release(struct device *dev)
> > > +{
> > > + struct uacce_device *uacce = to_uacce_device(dev);
> > > +
> > > + kfree(uacce);
> > > + uacce = NULL;
> > That line didn't do anything :)
> Yes, this is a mistake.
> It is up to caller to set to NULL to prevent release multi times.
Release function is called by the driver core which will not touch the
value again.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 2/4] uacce: add uacce driver
On Wed, Jan 15, 2020 at 07:18:34PM +0800, zhangfei wrote:
> Hi, Greg
>
> On 2020/1/14 下午10:59, Greg Kroah-Hartman wrote:
> > On Mon, Jan 13, 2020 at 11:34:55AM +0800, zhangfei wrote:
> > > Hi, Greg
> > >
> > > Thanks for the review.
> > >
> > > On 2020/1/12 上午3:40, Greg Kroah-Hartman wrote:
> > > > On Sat, Jan 11, 2020 at 10:48:37AM +0800, Zhangfei Gao wrote:
> > > > > +static int uacce_fops_open(struct inode *inode, struct file *filep)
> > > > > +{
> > > > > + struct uacce_mm *uacce_mm = NULL;
> > > > > + struct uacce_device *uacce;
> > > > > + struct uacce_queue *q;
> > > > > + int ret = 0;
> > > > > +
> > > > > + uacce = xa_load(&uacce_xa, iminor(inode));
> > > > > + if (!uacce)
> > > > > + return -ENODEV;
> > > > > +
> > > > > + if (!try_module_get(uacce->parent->driver->owner))
> > > > > + return -ENODEV;
> > > > Why are you trying to grab the module reference of the parent device?
> > > > Why is that needed and what is that going to help with here?
> > > >
> > > > This shouldn't be needed as the module reference of the owner of the
> > > > fileops for this module is incremented, and the "parent" module depends
> > > > on this module, so how could it be unloaded without this code being
> > > > unloaded?
> > > >
> > > > Yes, if you build this code into the kernel and the "parent" driver is a
> > > > module, then you will not have a reference, but when you remove that
> > > > parent driver the device will be removed as it has to be unregistered
> > > > before that parent driver can be removed from the system, right?
> > > >
> > > > Or what am I missing here?
> > > The refcount here is preventing rmmod "parent" module after fd is opened,
> > > since user driver has mmap kernel memory to user space, like mmio, which
> > > may
> > > still in-use.
> > >
> > > With the refcount protection, rmmod "parent" module will fail until
> > > application free the fd.
> > > log like: rmmod: ERROR: Module hisi_zip is in use
> > But if the "parent" module is to be unloaded, it has to unregister the
> > "child" device and that will call the destructor in here and then you
> > will tear everything down and all should be good.
> >
> > There's no need to "forbid" a module from being unloaded, even if it is
> > being used. Look at all networking drivers, they work that way, right?
> Thanks Greg for the kind suggestion.
>
> I still have one uncertainty.
> Does uacce has to block process continue accessing the mmapped area when
> remove "parent" module?
> Uacce can block device access the physical memory when parent module call
> uacce_remove.
> But application is still running, and suppose it is not the kernel driver's
> responsibility to call unmap.
>
> I am looking for some examples in kernel,
> looks vfio does not block process continue accessing when
> vfio_unregister_iommu_driver either.
>
> In my test, application will keep waiting after rmmod parent, until ctrl+c,
> when unmap is called.
> During the process, kernel does not report any error.
>
> Do you have any advice?
Is there no way for the kernel to invalidate the memory and tell the
process to stop? tty drivers do this for when they are removed from the
system.
Anyway, this is all very rare, no kernel module is ever unloaded on a
real system, that is only for when developers are working on them, so
it's probably not that big of an issue, right?
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On Wed, Jan 15, 2020 at 10:12:46PM +0800, Zhangfei Gao wrote: > From: Kenneth Lee > > Uacce (Unified/User-space-access-intended Accelerator Framework) targets to > provide Shared Virtual Addressing (SVA) between accelerators and processes. > So accelerator can access any data structure of the main cpu. > This differs from the data sharing between cpu and io device, which share > only data content rather than address. > Since unified address, hardware and user space of process can share the > same virtual address in the communication. > > Uacce create a chrdev for every registration, the queue is allocated to > the process when the chrdev is opened. Then the process can access the > hardware resource by interact with the queue file. By mmap the queue > file space to user space, the process can directly put requests to the > hardware without syscall to the kernel space. > > The IOMMU core only tracks mm<->device bonds at the moment, because it > only needs to handle IOTLB invalidation and PASID table entries. However > uacce needs a finer granularity since multiple queues from the same > device can be bound to an mm. When the mm exits, all bound queues must > be stopped so that the IOMMU can safely clear the PASID table entry and > reallocate the PASID. > > An intermediate struct uacce_mm links uacce devices and queues. > Note that an mm may be bound to multiple devices but an uacce_mm > structure only ever belongs to a single device, because we don't need > anything more complex (if multiple devices are bound to one mm, then > we'll create one uacce_mm for each bond). > > uacce_device --+-- uacce_mm --+-- uacce_queue >| '-- uacce_queue >| >'-- uacce_mm --+-- uacce_queue > +-- uacce_queue > '-- uacce_queue > > Reviewed-by: Jonathan Cameron > Signed-off-by: Kenneth Lee > Signed-off-by: Zaibo Xu > Signed-off-by: Zhou Wang > Signed-off-by: Jean-Philippe Brucker > Signed-off-by: Zhangfei Gao Looks much saner now, thanks for all of the work on this: Reviewed-by: Greg Kroah-Hartman Or am I supposed to take this in my tree? If so, I can, but I need an ack for the crypto parts. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] uacce: add uacce driver
On Thu, Feb 13, 2020 at 05:15:10PM +0800, Herbert Xu wrote: > On Mon, Feb 10, 2020 at 03:37:11PM -0800, Greg Kroah-Hartman wrote: > > > > Looks much saner now, thanks for all of the work on this: > > > > Reviewed-by: Greg Kroah-Hartman > > > > Or am I supposed to take this in my tree? If so, I can, but I need an > > ack for the crypto parts. > > I can take this series through the crypto tree if that's fine with > you. Please do, thanks! greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 09/16] driver core: add iommu device fault reporting data
On Thu, Oct 05, 2017 at 04:03:37PM -0700, Jacob Pan wrote: > DMA faults can be detected by IOMMU at device level. Adding a pointer > to struct device allows IOMMU subsystem to report relevant faults > back to the device driver for further handling. > For direct assigned device (or user space drivers), guest OS holds > responsibility to handle and respond per device IOMMU fault. > Therefore we need fault reporting mechanism to propagate faults beyond > IOMMU subsystem. > > Signed-off-by: Jacob Pan Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 09/16] driver core: add iommu device fault reporting data
On Fri, Oct 06, 2017 at 12:11:45AM -0700, Christoph Hellwig wrote: > On Thu, Oct 05, 2017 at 04:03:37PM -0700, Jacob Pan wrote: > > DMA faults can be detected by IOMMU at device level. Adding a pointer > > to struct device allows IOMMU subsystem to report relevant faults > > back to the device driver for further handling. > > For direct assigned device (or user space drivers), guest OS holds > > responsibility to handle and respond per device IOMMU fault. > > Therefore we need fault reporting mechanism to propagate faults beyond > > IOMMU subsystem. > > We use struct device all over the system, and I don't think we should > bloat it for fringe case IOMMU bits. > > Someone really needs to take a step back and figure out how to move > this into a structure that's only allocated for device that actually > can do physical DMA (and/or have an iommu attached) > > This is the 3rd iommu field, in addition to 8 dma-specific fields > that we carry around for each struct device. Ick, 8? Yeah, it's getting big... How about just a single pointer for iommu and dma-specific stuff that you all can hang crap like this off of if needed? thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 09/16] driver core: add iommu device fault reporting data
On Fri, Nov 17, 2017 at 10:55:07AM -0800, Jacob Pan wrote: > DMA faults can be detected by IOMMU at device level. Adding a pointer > to struct device allows IOMMU subsystem to report relevant faults > back to the device driver for further handling. > For direct assigned device (or user space drivers), guest OS holds > responsibility to handle and respond per device IOMMU fault. > Therefore we need fault reporting mechanism to propagate faults beyond > IOMMU subsystem. > > There are two other IOMMU data pointers under struct device today, here > we introduce iommu_param as a parent pointer such that all device IOMMU > data can be consolidated here. The idea was suggested here by Greg KH > and Joerg. The name iommu_param is chosen here since iommu_data has been used. > > Suggested-by: Greg Kroah-Hartman > Signed-off-by: Jacob Pan > Link: https://lkml.org/lkml/2017/10/6/81 Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 20/21] staging: vc04_services: Remove depends on HAS_DMA in case of platform dependency
On Fri, Mar 16, 2018 at 02:51:53PM +0100, Geert Uytterhoeven wrote: > Remove dependencies on HAS_DMA where a Kconfig symbol depends on another > symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST". > In most cases this other symbol is an architecture or platform specific > symbol, or PCI. > > Generic symbols and drivers without platform dependencies keep their > dependencies on HAS_DMA, to prevent compiling subsystems or drivers that > cannot work anyway. > > This simplifies the dependencies, and allows to improve compile-testing. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Mark Brown > Acked-by: Robin Murphy Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 18/21] serial: Remove depends on HAS_DMA in case of platform dependency
On Fri, Mar 16, 2018 at 02:51:51PM +0100, Geert Uytterhoeven wrote: > Remove dependencies on HAS_DMA where a Kconfig symbol depends on another > symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST". > In most cases this other symbol is an architecture or platform specific > symbol, or PCI. > > Generic symbols and drivers without platform dependencies keep their > dependencies on HAS_DMA, to prevent compiling subsystems or drivers that > cannot work anyway. > > This simplifies the dependencies, and allows to improve compile-testing. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Mark Brown > Acked-by: Robin Murphy Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/5] usb: gadget: Add NO_DMA dummies for DMA mapping API
On Fri, Mar 16, 2018 at 02:25:42PM +0100, Geert Uytterhoeven wrote:
> Add dummies for usb_gadget_{,un}map_request{,_by_dev}(), to allow
> compile-testing if NO_DMA=y.
>
> This prevents the following from showing up later:
>
> ERROR: "usb_gadget_unmap_request_by_dev"
> [drivers/usb/renesas_usbhs/renesas_usbhs.ko] undefined!
> ERROR: "usb_gadget_map_request_by_dev"
> [drivers/usb/renesas_usbhs/renesas_usbhs.ko] undefined!
> ERROR: "usb_gadget_map_request" [drivers/usb/mtu3/mtu3.ko] undefined!
> ERROR: "usb_gadget_unmap_request" [drivers/usb/mtu3/mtu3.ko] undefined!
> ERROR: "usb_gadget_map_request" [drivers/usb/gadget/udc/renesas_usb3.ko]
> undefined!
> ERROR: "usb_gadget_unmap_request"
> [drivers/usb/gadget/udc/renesas_usb3.ko] undefined!
>
> Signed-off-by: Geert Uytterhoeven
> Reviewed-by: Mark Brown
> Acked-by: Robin Murphy
> Acked-by: Felipe Balbi
Acked-by: Greg Kroah-Hartman
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 21/21] usb: Remove depends on HAS_DMA in case of platform dependency
On Fri, Mar 16, 2018 at 02:51:54PM +0100, Geert Uytterhoeven wrote: > Remove dependencies on HAS_DMA where a Kconfig symbol depends on another > symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST". > In most cases this other symbol is an architecture or platform specific > symbol, or PCI. > > Generic symbols and drivers without platform dependencies keep their > dependencies on HAS_DMA, to prevent compiling subsystems or drivers that > cannot work anyway. > > This simplifies the dependencies, and allows to improve compile-testing. > > Signed-off-by: Geert Uytterhoeven > Reviewed-by: Mark Brown > Acked-by: Robin Murphy > Acked-by: Felipe Balbi [drivers/usb/gadget/] Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 11/22] driver core: add per device iommu param
On Thu, Mar 22, 2018 at 08:12:03PM -0700, Jacob Pan wrote: > DMA faults can be detected by IOMMU at device level. Adding a pointer > to struct device allows IOMMU subsystem to report relevant faults > back to the device driver for further handling. > For direct assigned device (or user space drivers), guest OS holds > responsibility to handle and respond per device IOMMU fault. > Therefore we need fault reporting mechanism to propagate faults beyond > IOMMU subsystem. > > There are two other IOMMU data pointers under struct device today, here > we introduce iommu_param as a parent pointer such that all device IOMMU > data can be consolidated here. The idea was suggested here by Greg KH > and Joerg. The name iommu_param is chosen here since iommu_data has been used. > > Suggested-by: Greg Kroah-Hartman > Signed-off-by: Jacob Pan > Signed-off-by: Jean-Philippe Brucker > Link: https://lkml.org/lkml/2017/10/6/81 Acked-by: Greg Kroah-Hartman ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] driver: core: Allow subsystems to continue deferring probe
On Wed, Jun 05, 2019 at 04:23:12PM +0200, Thierry Reding wrote:
> From: Thierry Reding
>
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
>
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
>
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
>
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
>
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/base/dd.c| 17 -
> drivers/base/power/domain.c | 2 +-
> drivers/iommu/of_iommu.c | 2 +-
> drivers/pinctrl/devicetree.c | 10 ++
> include/linux/device.h | 2 +-
> 5 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..25ffbadf4187 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -238,23 +238,30 @@ __setup("deferred_probe_timeout=",
> deferred_probe_timeout_setup);
> /**
> * driver_deferred_probe_check_state() - Check deferred probe state
> * @dev: device to check
> + * @persist: Boolean flag indicating whether drivers should keep trying to
> + * probe after built-in drivers have had a chance to probe. This is useful
> + * for built-in drivers that rely on resources provided by modular drivers.
Functionality aside, having random boolean flags in a function parameter
list is a horrid way to have an api. Now when you see a call to this
function with either "true" or "false" as an option, you have no idea
what that means. So you need to go look up this definition and then
work backwards.
So even if the idea is sane (I'm not saying that), this implementation
is not acceptable at all.
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] swiotlb: no need to check return value of debugfs_create functions
When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Cc: Konrad Rzeszutek Wilk
Cc: Christoph Hellwig
Cc: Marek Szyprowski
Cc: Robin Murphy
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Greg Kroah-Hartman
---
kernel/dma/swiotlb.c | 25 -
1 file changed, 4 insertions(+), 21 deletions(-)
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 13f0cb080a4d..62fa5a82a065 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -696,29 +696,12 @@ bool is_swiotlb_active(void)
static int __init swiotlb_create_debugfs(void)
{
- struct dentry *d_swiotlb_usage;
- struct dentry *ent;
-
- d_swiotlb_usage = debugfs_create_dir("swiotlb", NULL);
-
- if (!d_swiotlb_usage)
- return -ENOMEM;
-
- ent = debugfs_create_ulong("io_tlb_nslabs", 0400,
- d_swiotlb_usage, &io_tlb_nslabs);
- if (!ent)
- goto fail;
-
- ent = debugfs_create_ulong("io_tlb_used", 0400,
- d_swiotlb_usage, &io_tlb_used);
- if (!ent)
- goto fail;
+ struct dentry *root;
+ root = debugfs_create_dir("swiotlb", NULL);
+ debugfs_create_ulong("io_tlb_nslabs", 0400, root, &io_tlb_nslabs);
+ debugfs_create_ulong("io_tlb_used", 0400, root, &io_tlb_used);
return 0;
-
-fail:
- debugfs_remove_recursive(d_swiotlb_usage);
- return -ENOMEM;
}
late_initcall(swiotlb_create_debugfs);
--
2.22.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> From: Thierry Reding
>
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
>
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
>
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
>
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
>
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
>
> Signed-off-by: Thierry Reding
> ---
> Changes in v2:
> - pass persist flag via flags parameter to make the function call easier
> to understand
>
> drivers/base/dd.c| 19 ++-
> drivers/base/power/domain.c | 2 +-
> drivers/iommu/of_iommu.c | 2 +-
> drivers/pinctrl/devicetree.c | 9 +
> include/linux/device.h | 18 +-
> 5 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..0399a6f6c479 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -238,23 +238,32 @@ __setup("deferred_probe_timeout=",
> deferred_probe_timeout_setup);
> /**
> * driver_deferred_probe_check_state() - Check deferred probe state
> * @dev: device to check
> + * @flags: Flags used to control the behavior of this function. Drivers can
> + * set the DRIVER_DEFER_PROBE_PERSIST flag to indicate that they want to
> + * keep trying to probe after built-in drivers have had a chance to probe.
> + * This is useful for built-in drivers that rely on resources provided by
> + * modular drivers.
> *
> * Returns -ENODEV if init is done and all built-in drivers have had a chance
> - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> + * to probe (i.e. initcalls are done) and unless the
> DRIVER_DEFER_PROBE_PERSIST
> + * flag is set, -ETIMEDOUT if deferred probe debug timeout has expired, or
> + * -EPROBE_DEFER if none of those conditions are met.
> *
> * Drivers or subsystems can opt-in to calling this function instead of
> directly
> * returning -EPROBE_DEFER.
> */
> -int driver_deferred_probe_check_state(struct device *dev)
> +int driver_deferred_probe_check_state(struct device *dev, unsigned long
> flags)
> {
> if (initcalls_done) {
> if (!deferred_probe_timeout) {
> dev_WARN(dev, "deferred probe timeout, ignoring
> dependency");
> return -ETIMEDOUT;
> }
> - dev_warn(dev, "ignoring dependency for device, assuming no
> driver");
> - return -ENODEV;
> +
> + if ((flags & DRIVER_DEFER_PROBE_PERSIST) == 0) {
> + dev_warn(dev, "ignoring dependency for device, assuming
> no driver");
> + return -ENODEV;
> + }
> }
> return -EPROBE_DEFER;
> }
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 33c30c1e6a30..6198c6a30fe2 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2423,7 +2423,7 @@ static int __genpd_dev_pm_attach(struct device *dev,
> struct device *base_dev,
> mutex_unlock(&gpd_list_lock);
> dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
> __func__, PTR_ERR(pd));
> - return driver_deferred_probe_check_state(base_dev);
> + return driver_deferred_probe_check_state(base_dev, 0);
Again, I said no odd flags for functions, how is anyone supposed to know
what "0" means here?
You just s
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Fri, Jun 14, 2019 at 12:10:10PM +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 14, 2019 at 11:39 AM Thierry Reding
> wrote:
> >
> > On Fri, Jun 14, 2019 at 11:10:58AM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> > > > From: Thierry Reding
> > > >
>
> [cut]
>
> >
> > To avoid further back and forth, what exactly is it that you would have
> > me do? That is, what do you consider to be the correct way to do this?
> >
> > Would you prefer me to add another function with a different name that
> > reimplements the functionality only with the exception? Something along
> > the lines of:
> >
> > int driver_deferred_probe_check_state_continue(struct device *dev)
> > {
> > int ret;
> >
> > ret = driver_deferred_probe_check_state(dev);
> > if (ret == -ENODEV)
> > return -EPROBE_DEFER;
> >
> > return ret;
> > }
> >
> > ? I'd need to split that up some more to avoid the warning that the
> > inner function prints before returning -ENODEV, but that's a minor
> > detail. Would that API be more to your liking?
>
> Well, why don't you do
>
> static int deferred_probe_check_state_internal(struct device *dev)
> {
> if (!initcalls_done)
> return -EPROBE_DEFER;
>
> if (!deferred_probe_timeout) {
> dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> return -ETIMEDOUT;
> }
>
> return 0;
> }
>
> int driver_deferred_probe_check_state(struct device *dev)
> {
> int ret = deferred_probe_check_state_internal(dev);
>
> if (ret)
> return ret;
>
> dev_warn(dev, "ignoring dependency for device, assuming no driver");
> return -ENODEV;
> }
>
> int driver_deferred_probe_check_state_continue(struct device *dev)
> {
> int ret = deferred_probe_check_state_internal(dev);
>
> if (ret)
> return ret;
>
> return -EPROBE_DEFER;
> }
Yes, that's much more sane. Self-decribing apis are the key here, I did
not want a boolean flag, or any other flag, as part of the public api as
they do not describe what the call does at all.
thanks,
greg k-h
[PATCH] omap-iommu: no need to check return value of debugfs_create functions
When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Cc: Joerg Roedel
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Greg Kroah-Hartman
---
Warning, not even test-built, but "should" work :)
drivers/iommu/omap-iommu-debug.c | 35 ++--
1 file changed, 6 insertions(+), 29 deletions(-)
diff --git a/drivers/iommu/omap-iommu-debug.c b/drivers/iommu/omap-iommu-debug.c
index 4abc0ef522a8..cea851702f54 100644
--- a/drivers/iommu/omap-iommu-debug.c
+++ b/drivers/iommu/omap-iommu-debug.c
@@ -239,17 +239,6 @@ DEBUG_FOPS_RO(regs);
DEFINE_SHOW_ATTRIBUTE(tlb);
DEFINE_SHOW_ATTRIBUTE(pagetable);
-#define __DEBUG_ADD_FILE(attr, mode) \
- { \
- struct dentry *dent;\
- dent = debugfs_create_file(#attr, mode, obj->debug_dir, \
- obj, &attr##_fops); \
- if (!dent) \
- goto err; \
- }
-
-#define DEBUG_ADD_FILE_RO(name) __DEBUG_ADD_FILE(name, 0400)
-
void omap_iommu_debugfs_add(struct omap_iommu *obj)
{
struct dentry *d;
@@ -257,23 +246,13 @@ void omap_iommu_debugfs_add(struct omap_iommu *obj)
if (!iommu_debug_root)
return;
- obj->debug_dir = debugfs_create_dir(obj->name, iommu_debug_root);
- if (!obj->debug_dir)
- return;
+ d = debugfs_create_dir(obj->name, iommu_debug_root);
+ obj->debug_dir = d;
- d = debugfs_create_u32("nr_tlb_entries", 0400, obj->debug_dir,
- &obj->nr_tlb_entries);
- if (!d)
- return;
-
- DEBUG_ADD_FILE_RO(regs);
- DEBUG_ADD_FILE_RO(tlb);
- DEBUG_ADD_FILE_RO(pagetable);
-
- return;
-
-err:
- debugfs_remove_recursive(obj->debug_dir);
+ debugfs_create_u32("nr_tlb_entries", 0400, d, &obj->nr_tlb_entries);
+ debugfs_create_file("regs", 0400, d, obj, &attrregs_fops);
+ debugfs_create_file("tlb", 0400, d, obj, &attrtlb_fops);
+ debugfs_create_file("pagetable", 0400, d, obj, &attrpagetable_fops);
}
void omap_iommu_debugfs_remove(struct omap_iommu *obj)
@@ -287,8 +266,6 @@ void omap_iommu_debugfs_remove(struct omap_iommu *obj)
void __init omap_iommu_debugfs_init(void)
{
iommu_debug_root = debugfs_create_dir("omap_iommu", NULL);
- if (!iommu_debug_root)
- pr_err("can't create debugfs dir\n");
}
void __exit omap_iommu_debugfs_exit(void)
--
2.22.0
Re: [PATCH] omap-iommu: no need to check return value of debugfs_create functions
On Thu, Jul 04, 2019 at 05:35:52PM +0200, Joerg Roedel wrote:
> On Thu, Jul 04, 2019 at 04:36:49PM +0200, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value. The function can work or not, but the code logic should
> > never do something different based on this.
> >
> > Cc: Joerg Roedel
> > Cc: iommu@lists.linux-foundation.org
> > Signed-off-by: Greg Kroah-Hartman
> > ---
> > Warning, not even test-built, but "should" work :)
>
> It almost did :)
>
> > + debugfs_create_file("regs", 0400, d, obj, &attrregs_fops);
> > + debugfs_create_file("tlb", 0400, d, obj, &attrtlb_fops);
> > + debugfs_create_file("pagetable", 0400, d, obj, &attrpagetable_fops);
>
> The _fops were named without the 'attr' prefix, changed that and it
> compiled. Patch is now applied.
Ah, so close :)
Thanks for fixing it up and applying it!
greg k-h
[PATCH 5.2 202/215] iommu/vt-d: Dont queue_iova() if there is no flush queue
From: Dmitry Safonov
commit effa467870c7612012885df4e246bdb8ffd8e44c upstream.
Intel VT-d driver was reworked to use common deferred flushing
implementation. Previously there was one global per-cpu flush queue,
afterwards - one per domain.
Before deferring a flush, the queue should be allocated and initialized.
Currently only domains with IOMMU_DOMAIN_DMA type initialize their flush
queue. It's probably worth to init it for static or unmanaged domains
too, but it may be arguable - I'm leaving it to iommu folks.
Prevent queuing an iova flush if the domain doesn't have a queue.
The defensive check seems to be worth to keep even if queue would be
initialized for all kinds of domains. And is easy backportable.
On 4.19.43 stable kernel it has a user-visible effect: previously for
devices in si domain there were crashes, on sata devices:
BUG: spinlock bad magic on CPU#6, swapper/0/1
lock: 0x88844f582008, .magic: , .owner: /-1, .owner_cpu: 0
CPU: 6 PID: 1 Comm: swapper/0 Not tainted 4.19.43 #1
Call Trace:
dump_stack+0x61/0x7e
spin_bug+0x9d/0xa3
do_raw_spin_lock+0x22/0x8e
_raw_spin_lock_irqsave+0x32/0x3a
queue_iova+0x45/0x115
intel_unmap+0x107/0x113
intel_unmap_sg+0x6b/0x76
__ata_qc_complete+0x7f/0x103
ata_qc_complete+0x9b/0x26a
ata_qc_complete_multiple+0xd0/0xe3
ahci_handle_port_interrupt+0x3ee/0x48a
ahci_handle_port_intr+0x73/0xa9
ahci_single_level_irq_intr+0x40/0x60
__handle_irq_event_percpu+0x7f/0x19a
handle_irq_event_percpu+0x32/0x72
handle_irq_event+0x38/0x56
handle_edge_irq+0x102/0x121
handle_irq+0x147/0x15c
do_IRQ+0x66/0xf2
common_interrupt+0xf/0xf
RIP: 0010:__do_softirq+0x8c/0x2df
The same for usb devices that use ehci-pci:
BUG: spinlock bad magic on CPU#0, swapper/0/1
lock: 0x88844f402008, .magic: , .owner: /-1, .owner_cpu: 0
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.43 #4
Call Trace:
dump_stack+0x61/0x7e
spin_bug+0x9d/0xa3
do_raw_spin_lock+0x22/0x8e
_raw_spin_lock_irqsave+0x32/0x3a
queue_iova+0x77/0x145
intel_unmap+0x107/0x113
intel_unmap_page+0xe/0x10
usb_hcd_unmap_urb_setup_for_dma+0x53/0x9d
usb_hcd_unmap_urb_for_dma+0x17/0x100
unmap_urb_for_dma+0x22/0x24
__usb_hcd_giveback_urb+0x51/0xc3
usb_giveback_urb_bh+0x97/0xde
tasklet_action_common.isra.4+0x5f/0xa1
tasklet_action+0x2d/0x30
__do_softirq+0x138/0x2df
irq_exit+0x7d/0x8b
smp_apic_timer_interrupt+0x10f/0x151
apic_timer_interrupt+0xf/0x20
RIP: 0010:_raw_spin_unlock_irqrestore+0x17/0x39
Cc: David Woodhouse
Cc: Joerg Roedel
Cc: Lu Baolu
Cc: iommu@lists.linux-foundation.org
Cc: # 4.14+
Fixes: 13cf01744608 ("iommu/vt-d: Make use of iova deferred flushing")
Signed-off-by: Dmitry Safonov
Reviewed-by: Lu Baolu
Signed-off-by: Joerg Roedel
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |3 ++-
drivers/iommu/iova.c| 18 ++
include/linux/iova.h|6 ++
3 files changed, 22 insertions(+), 5 deletions(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3752,7 +3752,8 @@ static void intel_unmap(struct device *d
freelist = domain_unmap(domain, start_pfn, last_pfn);
- if (intel_iommu_strict || (pdev && pdev->untrusted)) {
+ if (intel_iommu_strict || (pdev && pdev->untrusted) ||
+ !has_iova_flush_queue(&domain->iovad)) {
iommu_flush_iotlb_psi(iommu, domain, start_pfn,
nrpages, !freelist, 0);
/* free iova */
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -54,9 +54,14 @@ init_iova_domain(struct iova_domain *iov
}
EXPORT_SYMBOL_GPL(init_iova_domain);
+bool has_iova_flush_queue(struct iova_domain *iovad)
+{
+ return !!iovad->fq;
+}
+
static void free_iova_flush_queue(struct iova_domain *iovad)
{
- if (!iovad->fq)
+ if (!has_iova_flush_queue(iovad))
return;
if (timer_pending(&iovad->fq_timer))
@@ -74,13 +79,14 @@ static void free_iova_flush_queue(struct
int init_iova_flush_queue(struct iova_domain *iovad,
iova_flush_cb flush_cb, iova_entry_dtor entry_dtor)
{
+ struct iova_fq __percpu *queue;
int cpu;
atomic64_set(&iovad->fq_flush_start_cnt, 0);
atomic64_set(&iovad->fq_flush_finish_cnt, 0);
- iovad->fq = alloc_percpu(struct iova_fq);
- if (!iovad->fq)
+ queue = alloc_percpu(struct iova_fq);
+ if (!queue)
return -ENOMEM;
iovad->flush_cb = flush_cb;
@@ -89,13 +95,17 @@ int init_iova_flush_queue(struct iova_do
for_each_possible_cpu(cpu) {
struct iova_fq *fq;
- fq = per_cpu_ptr(iovad->fq, cpu);
+ fq = per_cpu_ptr(queue, cpu);
fq->head = 0;
fq->tail = 0;
spin_lock
Re: [PATCH-4.19-stable 0/2] iommu/vt-d: queue_iova() boot crash backport
On Wed, Jul 31, 2019 at 05:22:18PM +0100, Dmitry Safonov wrote: > Backport commits from master that fix boot failure on some intel > machines. > > Cc: David Woodhouse > Cc: Joerg Roedel > Cc: Joerg Roedel > Cc: Lu Baolu Thanks for the backports, 4.19.y and 4.14.y patches now queued up. greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 4.14 17/25] iommu/vt-d: Dont queue_iova() if there is no flush queue
From: Dmitry Safonov
commit effa467870c7612012885df4e246bdb8ffd8e44c upstream.
Intel VT-d driver was reworked to use common deferred flushing
implementation. Previously there was one global per-cpu flush queue,
afterwards - one per domain.
Before deferring a flush, the queue should be allocated and initialized.
Currently only domains with IOMMU_DOMAIN_DMA type initialize their flush
queue. It's probably worth to init it for static or unmanaged domains
too, but it may be arguable - I'm leaving it to iommu folks.
Prevent queuing an iova flush if the domain doesn't have a queue.
The defensive check seems to be worth to keep even if queue would be
initialized for all kinds of domains. And is easy backportable.
On 4.19.43 stable kernel it has a user-visible effect: previously for
devices in si domain there were crashes, on sata devices:
BUG: spinlock bad magic on CPU#6, swapper/0/1
lock: 0x88844f582008, .magic: , .owner: /-1, .owner_cpu: 0
CPU: 6 PID: 1 Comm: swapper/0 Not tainted 4.19.43 #1
Call Trace:
dump_stack+0x61/0x7e
spin_bug+0x9d/0xa3
do_raw_spin_lock+0x22/0x8e
_raw_spin_lock_irqsave+0x32/0x3a
queue_iova+0x45/0x115
intel_unmap+0x107/0x113
intel_unmap_sg+0x6b/0x76
__ata_qc_complete+0x7f/0x103
ata_qc_complete+0x9b/0x26a
ata_qc_complete_multiple+0xd0/0xe3
ahci_handle_port_interrupt+0x3ee/0x48a
ahci_handle_port_intr+0x73/0xa9
ahci_single_level_irq_intr+0x40/0x60
__handle_irq_event_percpu+0x7f/0x19a
handle_irq_event_percpu+0x32/0x72
handle_irq_event+0x38/0x56
handle_edge_irq+0x102/0x121
handle_irq+0x147/0x15c
do_IRQ+0x66/0xf2
common_interrupt+0xf/0xf
RIP: 0010:__do_softirq+0x8c/0x2df
The same for usb devices that use ehci-pci:
BUG: spinlock bad magic on CPU#0, swapper/0/1
lock: 0x88844f402008, .magic: , .owner: /-1, .owner_cpu: 0
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.43 #4
Call Trace:
dump_stack+0x61/0x7e
spin_bug+0x9d/0xa3
do_raw_spin_lock+0x22/0x8e
_raw_spin_lock_irqsave+0x32/0x3a
queue_iova+0x77/0x145
intel_unmap+0x107/0x113
intel_unmap_page+0xe/0x10
usb_hcd_unmap_urb_setup_for_dma+0x53/0x9d
usb_hcd_unmap_urb_for_dma+0x17/0x100
unmap_urb_for_dma+0x22/0x24
__usb_hcd_giveback_urb+0x51/0xc3
usb_giveback_urb_bh+0x97/0xde
tasklet_action_common.isra.4+0x5f/0xa1
tasklet_action+0x2d/0x30
__do_softirq+0x138/0x2df
irq_exit+0x7d/0x8b
smp_apic_timer_interrupt+0x10f/0x151
apic_timer_interrupt+0xf/0x20
RIP: 0010:_raw_spin_unlock_irqrestore+0x17/0x39
Cc: David Woodhouse
Cc: Joerg Roedel
Cc: Lu Baolu
Cc: iommu@lists.linux-foundation.org
Cc: # 4.14+
Fixes: 13cf01744608 ("iommu/vt-d: Make use of iova deferred flushing")
Signed-off-by: Dmitry Safonov
Reviewed-by: Lu Baolu
Signed-off-by: Joerg Roedel
[v4.14-port notes:
o minor conflict with untrusted IOMMU devices check under if-condition
o setup_timer() near one chunk is timer_setup() in v5.3]
Signed-off-by: Dmitry Safonov
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |2 +-
drivers/iommu/iova.c| 18 ++
include/linux/iova.h|6 ++
3 files changed, 21 insertions(+), 5 deletions(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3702,7 +3702,7 @@ static void intel_unmap(struct device *d
freelist = domain_unmap(domain, start_pfn, last_pfn);
- if (intel_iommu_strict) {
+ if (intel_iommu_strict || !has_iova_flush_queue(&domain->iovad)) {
iommu_flush_iotlb_psi(iommu, domain, start_pfn,
nrpages, !freelist, 0);
/* free iova */
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -58,9 +58,14 @@ init_iova_domain(struct iova_domain *iov
}
EXPORT_SYMBOL_GPL(init_iova_domain);
+bool has_iova_flush_queue(struct iova_domain *iovad)
+{
+ return !!iovad->fq;
+}
+
static void free_iova_flush_queue(struct iova_domain *iovad)
{
- if (!iovad->fq)
+ if (!has_iova_flush_queue(iovad))
return;
if (timer_pending(&iovad->fq_timer))
@@ -78,13 +83,14 @@ static void free_iova_flush_queue(struct
int init_iova_flush_queue(struct iova_domain *iovad,
iova_flush_cb flush_cb, iova_entry_dtor entry_dtor)
{
+ struct iova_fq __percpu *queue;
int cpu;
atomic64_set(&iovad->fq_flush_start_cnt, 0);
atomic64_set(&iovad->fq_flush_finish_cnt, 0);
- iovad->fq = alloc_percpu(struct iova_fq);
- if (!iovad->fq)
+ queue = alloc_percpu(struct iova_fq);
+ if (!queue)
return -ENOMEM;
iovad->flush_cb = flush_cb;
@@ -93,13 +99,17 @@ int init_iova_flush_queue(struct iova_do
for_each_possible_cpu(cpu) {
struct iova_fq *fq;
- fq = per_cpu_ptr(iovad->fq, cpu);
+ fq = per_cpu_ptr(queue, cpu);
fq-&g
[PATCH 4.19 17/32] iommu/vt-d: Dont queue_iova() if there is no flush queue
From: Dmitry Safonov
commit effa467870c7612012885df4e246bdb8ffd8e44c upstream.
Intel VT-d driver was reworked to use common deferred flushing
implementation. Previously there was one global per-cpu flush queue,
afterwards - one per domain.
Before deferring a flush, the queue should be allocated and initialized.
Currently only domains with IOMMU_DOMAIN_DMA type initialize their flush
queue. It's probably worth to init it for static or unmanaged domains
too, but it may be arguable - I'm leaving it to iommu folks.
Prevent queuing an iova flush if the domain doesn't have a queue.
The defensive check seems to be worth to keep even if queue would be
initialized for all kinds of domains. And is easy backportable.
On 4.19.43 stable kernel it has a user-visible effect: previously for
devices in si domain there were crashes, on sata devices:
BUG: spinlock bad magic on CPU#6, swapper/0/1
lock: 0x88844f582008, .magic: , .owner: /-1, .owner_cpu: 0
CPU: 6 PID: 1 Comm: swapper/0 Not tainted 4.19.43 #1
Call Trace:
dump_stack+0x61/0x7e
spin_bug+0x9d/0xa3
do_raw_spin_lock+0x22/0x8e
_raw_spin_lock_irqsave+0x32/0x3a
queue_iova+0x45/0x115
intel_unmap+0x107/0x113
intel_unmap_sg+0x6b/0x76
__ata_qc_complete+0x7f/0x103
ata_qc_complete+0x9b/0x26a
ata_qc_complete_multiple+0xd0/0xe3
ahci_handle_port_interrupt+0x3ee/0x48a
ahci_handle_port_intr+0x73/0xa9
ahci_single_level_irq_intr+0x40/0x60
__handle_irq_event_percpu+0x7f/0x19a
handle_irq_event_percpu+0x32/0x72
handle_irq_event+0x38/0x56
handle_edge_irq+0x102/0x121
handle_irq+0x147/0x15c
do_IRQ+0x66/0xf2
common_interrupt+0xf/0xf
RIP: 0010:__do_softirq+0x8c/0x2df
The same for usb devices that use ehci-pci:
BUG: spinlock bad magic on CPU#0, swapper/0/1
lock: 0x88844f402008, .magic: , .owner: /-1, .owner_cpu: 0
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.19.43 #4
Call Trace:
dump_stack+0x61/0x7e
spin_bug+0x9d/0xa3
do_raw_spin_lock+0x22/0x8e
_raw_spin_lock_irqsave+0x32/0x3a
queue_iova+0x77/0x145
intel_unmap+0x107/0x113
intel_unmap_page+0xe/0x10
usb_hcd_unmap_urb_setup_for_dma+0x53/0x9d
usb_hcd_unmap_urb_for_dma+0x17/0x100
unmap_urb_for_dma+0x22/0x24
__usb_hcd_giveback_urb+0x51/0xc3
usb_giveback_urb_bh+0x97/0xde
tasklet_action_common.isra.4+0x5f/0xa1
tasklet_action+0x2d/0x30
__do_softirq+0x138/0x2df
irq_exit+0x7d/0x8b
smp_apic_timer_interrupt+0x10f/0x151
apic_timer_interrupt+0xf/0x20
RIP: 0010:_raw_spin_unlock_irqrestore+0x17/0x39
Cc: David Woodhouse
Cc: Joerg Roedel
Cc: Lu Baolu
Cc: iommu@lists.linux-foundation.org
Cc: # 4.14+
Fixes: 13cf01744608 ("iommu/vt-d: Make use of iova deferred flushing")
Signed-off-by: Dmitry Safonov
Reviewed-by: Lu Baolu
Signed-off-by: Joerg Roedel
[v4.14-port notes:
o minor conflict with untrusted IOMMU devices check under if-condition]
Signed-off-by: Dmitry Safonov
Signed-off-by: Greg Kroah-Hartman
---
drivers/iommu/intel-iommu.c |2 +-
drivers/iommu/iova.c| 18 ++
include/linux/iova.h|6 ++
3 files changed, 21 insertions(+), 5 deletions(-)
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -3721,7 +3721,7 @@ static void intel_unmap(struct device *d
freelist = domain_unmap(domain, start_pfn, last_pfn);
- if (intel_iommu_strict) {
+ if (intel_iommu_strict || !has_iova_flush_queue(&domain->iovad)) {
iommu_flush_iotlb_psi(iommu, domain, start_pfn,
nrpages, !freelist, 0);
/* free iova */
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -65,9 +65,14 @@ init_iova_domain(struct iova_domain *iov
}
EXPORT_SYMBOL_GPL(init_iova_domain);
+bool has_iova_flush_queue(struct iova_domain *iovad)
+{
+ return !!iovad->fq;
+}
+
static void free_iova_flush_queue(struct iova_domain *iovad)
{
- if (!iovad->fq)
+ if (!has_iova_flush_queue(iovad))
return;
if (timer_pending(&iovad->fq_timer))
@@ -85,13 +90,14 @@ static void free_iova_flush_queue(struct
int init_iova_flush_queue(struct iova_domain *iovad,
iova_flush_cb flush_cb, iova_entry_dtor entry_dtor)
{
+ struct iova_fq __percpu *queue;
int cpu;
atomic64_set(&iovad->fq_flush_start_cnt, 0);
atomic64_set(&iovad->fq_flush_finish_cnt, 0);
- iovad->fq = alloc_percpu(struct iova_fq);
- if (!iovad->fq)
+ queue = alloc_percpu(struct iova_fq);
+ if (!queue)
return -ENOMEM;
iovad->flush_cb = flush_cb;
@@ -100,13 +106,17 @@ int init_iova_flush_queue(struct iova_do
for_each_possible_cpu(cpu) {
struct iova_fq *fq;
- fq = per_cpu_ptr(iovad->fq, cpu);
+ fq = per_cpu_ptr(queue, cpu);
fq->head = 0;
fq->tail
[PATCH 5.2 085/144] iommu/vt-d: Check if domain->pgd was allocated
[ Upstream commit 3ee9eca760e7d0b68c55813243de66bbb499dc3b ]
There is a couple of places where on domain_init() failure domain_exit()
is called. While currently domain_init() can fail only if
alloc_pgtable_page() has failed.
Make domain_exit() check if domain->pgd present, before calling
domain_unmap(), as it theoretically should crash on clearing pte entries
in dma_pte_clear_level().
Cc: David Woodhouse
Cc: Joerg Roedel
Cc: Lu Baolu
Cc: iommu@lists.linux-foundation.org
Signed-off-by: Dmitry Safonov
Reviewed-by: Lu Baolu
Signed-off-by: Joerg Roedel
Signed-off-by: Sasha Levin
---
drivers/iommu/intel-iommu.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2101601adf57d..1ad24367373f4 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1900,7 +1900,6 @@ static int domain_init(struct dmar_domain *domain, struct
intel_iommu *iommu,
static void domain_exit(struct dmar_domain *domain)
{
- struct page *freelist;
/* Remove associated devices and clear attached or cached domains */
rcu_read_lock();
@@ -1910,9 +1909,12 @@ static void domain_exit(struct dmar_domain *domain)
/* destroy iovas */
put_iova_domain(&domain->iovad);
- freelist = domain_unmap(domain, 0, DOMAIN_MAX_PFN(domain->gaw));
+ if (domain->pgd) {
+ struct page *freelist;
- dma_free_pagelist(freelist);
+ freelist = domain_unmap(domain, 0, DOMAIN_MAX_PFN(domain->gaw));
+ dma_free_pagelist(freelist);
+ }
free_domain_mem(domain);
}
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
On Sun, Aug 11, 2019 at 10:05:20AM +0200, Christoph Hellwig wrote:
> We still treat devices without a DMA mask as defaulting to 32-bits for
> both mask, but a few releases ago we've started warning about such
> cases, as they require special cases to work around this sloppyness.
> Add a dma_mask field to struct platform_object so that we can initialize
> the dma_mask pointer in struct device and initialize both masks to
> 32-bits by default. Architectures can still override this in
> arch_setup_pdev_archdata if needed.
>
> Note that the code looks a little odd with the various conditionals
> because we have to support platform_device structures that are
> statically allocated.
>
> Signed-off-by: Christoph Hellwig
> ---
> drivers/base/platform.c | 15 +--
> include/linux/platform_device.h | 1 +
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index ec974ba9c0c4..b216fcb0a8af 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -264,6 +264,17 @@ struct platform_object {
> char name[];
> };
>
> +static void setup_pdev_archdata(struct platform_device *pdev)
> +{
> + if (!pdev->dev.coherent_dma_mask)
> + pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> + if (!pdev->dma_mask)
> + pdev->dma_mask = DMA_BIT_MASK(32);
> + if (!pdev->dev.dma_mask)
> + pdev->dev.dma_mask = &pdev->dma_mask;
> + arch_setup_pdev_archdata(pdev);
> +};
> +
> /**
> * platform_device_put - destroy a platform device
> * @pdev: platform device to free
> @@ -310,7 +321,7 @@ struct platform_device *platform_device_alloc(const char
> *name, int id)
> pa->pdev.id = id;
> device_initialize(&pa->pdev.dev);
> pa->pdev.dev.release = platform_device_release;
> - arch_setup_pdev_archdata(&pa->pdev);
> + setup_pdev_archdata(&pa->pdev);
> }
>
> return pa ? &pa->pdev : NULL;
> @@ -512,7 +523,7 @@ EXPORT_SYMBOL_GPL(platform_device_del);
> int platform_device_register(struct platform_device *pdev)
> {
> device_initialize(&pdev->dev);
> - arch_setup_pdev_archdata(pdev);
> + setup_pdev_archdata(pdev);
> return platform_device_add(pdev);
> }
> EXPORT_SYMBOL_GPL(platform_device_register);
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 9bc36b589827..a2abde2aef25 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -24,6 +24,7 @@ struct platform_device {
> int id;
> boolid_auto;
> struct device dev;
> + u64 dma_mask;
Why is the dma_mask in 'struct device' which is part of this structure,
not sufficient here? Shouldn't the "platform" be setting that up
correctly already in the "archdata" type callback?
confused,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: next take at setting up a dma mask by default for platform devices
On Sun, Aug 11, 2019 at 10:05:14AM +0200, Christoph Hellwig wrote: > Hi all, > > this is another attempt to make sure the dma_mask pointer is always > initialized for platform devices. Not doing so lead to lots of > boilerplate code, and makes platform devices different from all our > major busses like PCI where we always set up a dma_mask. In the long > run this should also help to eventually make dma_mask a scalar value > instead of a pointer and remove even more cruft. > > The bigger blocker for this last time was the fact that the usb > subsystem uses the presence or lack of a dma_mask to check if the core > should do dma mapping for the driver, which is highly unusual. So we > fix this first. Note that this has some overlap with the pending > desire to use the proper dma_mmap_coherent helper for mapping usb > buffers. The first two patches from this series should probably > go into 5.3 and then uses as the basis for the decision to use > dma_mmap_coherent. I've taken the first 2 patches for 5.3-final. Given that patch 3 needs to be fixed, I'll wait for a respin of these before considering them. thanks, greg k-h ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 6/6] driver core: initialize a default DMA mask for platform device
On Thu, Aug 15, 2019 at 03:38:12PM +0200, Christoph Hellwig wrote:
> On Thu, Aug 15, 2019 at 03:03:25PM +0200, Greg Kroah-Hartman wrote:
> > > --- a/include/linux/platform_device.h
> > > +++ b/include/linux/platform_device.h
> > > @@ -24,6 +24,7 @@ struct platform_device {
> > > int id;
> > > boolid_auto;
> > > struct device dev;
> > > + u64 dma_mask;
> >
> > Why is the dma_mask in 'struct device' which is part of this structure,
> > not sufficient here? Shouldn't the "platform" be setting that up
> > correctly already in the "archdata" type callback?
>
> Becaus the dma_mask in struct device is a pointer that needs to point
> to something, and this is the best space we can allocate for 'something'.
> m68k and powerpc currently do something roughly equivalent at the moment,
> while everyone else just has horrible, horrible hacks. As mentioned in
> the changelog the intent of this patch is that we treat platform devices
> like any other bus, where the bus allocates the space for the dma_mask.
> The long term plan is to eventually kill that weird pointer indirection
> that doesn't help anyone, but for that we need to sort out the basics
> first.
Ah, missed that, sorry. Ok, no objection from me. Might as well respin
this series and I can queue it up after 5.3-rc5 is out (which will have
your first 2 patches in it.)
thanks,
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: next take at setting up a dma mask by default for platform devices
On Thu, Aug 15, 2019 at 03:25:31PM +0200, Christoph Hellwig wrote: > On Thu, Aug 15, 2019 at 03:23:18PM +0200, Greg Kroah-Hartman wrote: > > I've taken the first 2 patches for 5.3-final. Given that patch 3 needs > > to be fixed, I'll wait for a respin of these before considering them. > > I have a respun version ready, but I'd really like to hear some > comments from usb developers about the approach before spamming > everyone again.. Spam away, we can take it :) ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote:
> Hello,
>
> On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> wrote:
> >
> > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > wrote:
> > >
> > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain wrote:
> > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig
> > > > > wrote:
> > > >
> > > > ...
> > > >
> > > > > (and likely call it "external" instead of "untrusted".
> > > >
> > > > Which is not okay. 'External' to what? 'untrusted' has been carefully
> > > > chosen by the meaning of it.
> > > > What external does mean for M.2. WWAN card in my laptop? It's in ACPI
> > > > tables, but I can replace it.
> > >
> > > Then your ACPI tables should show this, there is an attribute for it,
> > > right?
> >
> > There is a _PLD() method, but it's for the USB devices (or optional
> > for others, I don't remember by heart). So, most of the ACPI tables,
> > alas, don't show this.
> >
> > > > This is only one example. Or if firmware of some device is altered,
> > > > and it's internal (whatever it means) is it trusted or not?
> > >
> > > That is what people are using policy for today, if you object to this,
> > > please bring it up to those developers :)
> >
> > > > So, please leave it as is (I mean name).
> > >
> > > firmware today exports this attribute, why do you not want userspace to
> > > also know it?
>
> To clarify, the attribute exposed by the firmware today is
> "ExternalFacingPort" and "external-facing" respectively:
>
> 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
>
> The kernel flag was named "untrusted" though, hence the assumption
> that "external=untrusted" is currently baked into the kernel today.
> IMHO, using "external" would fix that (The assumption can thus be
> contained in the IOMMU drivers) and at the same time allow more use of
> this attribute.
>
> > >
> > > Trust is different, yes, don't get the two mixed up please. That should
> > > be a different sysfs attribute for obvious reasons.
> >
> > Yes, as a bottom line that's what I meant as well.
>
> So what is the consensus here? I don't have a strong opinion - but it
> seemed to me Greg is saying "external" and Andy is saying "untrusted"?
Those two things are totally separate things when it comes to a device.
One (external) describes the location of the device in the system.
The other (untrusted) describes what you want the kernel to do with this
device (trust or not trust it).
One you can change (from trust to untrusted or back), the other you can
not, it is a fixed read-only property that describes the hardware device
as defined by the firmware.
Depending on the policy you wish to define, you can use the location of
the device to determine if you want to trust the device or not.
Again, this is what USB does, but I'm getting really tired of saying
this, so I'm going to stop now...
greg k-h
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 4/4] pci: export untrusted attribute in sysfs
On Thu, Jun 18, 2020 at 10:23:38AM -0700, Rajat Jain wrote:
> Thanks Greg and Andy for your continued inputs, and thanks Ashok for chiming
> in.
>
> On Thu, Jun 18, 2020 at 9:23 AM Raj, Ashok wrote:
> >
> > Hi Greg,
> >
> >
> > On Thu, Jun 18, 2020 at 06:02:12PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 18, 2020 at 08:03:49AM -0700, Rajat Jain wrote:
> > > > Hello,
> > > >
> > > > On Thu, Jun 18, 2020 at 2:14 AM Andy Shevchenko
> > > > wrote:
> > > > >
> > > > > On Thu, Jun 18, 2020 at 11:36 AM Greg Kroah-Hartman
> > > > > wrote:
> > > > > >
> > > > > > On Thu, Jun 18, 2020 at 11:12:56AM +0300, Andy Shevchenko wrote:
> > > > > > > On Wed, Jun 17, 2020 at 10:56 PM Rajat Jain
> > > > > > > wrote:
> > > > > > > > On Wed, Jun 17, 2020 at 12:31 AM Christoph Hellwig
> > > > > > > > wrote:
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > (and likely call it "external" instead of "untrusted".
> > > > > > >
> > > > > > > Which is not okay. 'External' to what? 'untrusted' has been
> > > > > > > carefully
> > > > > > > chosen by the meaning of it.
> > > > > > > What external does mean for M.2. WWAN card in my laptop? It's in
> > > > > > > ACPI
> > > > > > > tables, but I can replace it.
> > > > > >
> > > > > > Then your ACPI tables should show this, there is an attribute for
> > > > > > it,
> > > > > > right?
> > > > >
> > > > > There is a _PLD() method, but it's for the USB devices (or optional
> > > > > for others, I don't remember by heart). So, most of the ACPI tables,
> > > > > alas, don't show this.
> > > > >
> > > > > > > This is only one example. Or if firmware of some device is
> > > > > > > altered,
> > > > > > > and it's internal (whatever it means) is it trusted or not?
> > > > > >
> > > > > > That is what people are using policy for today, if you object to
> > > > > > this,
> > > > > > please bring it up to those developers :)
> > > > >
> > > > > > > So, please leave it as is (I mean name).
> > > > > >
> > > > > > firmware today exports this attribute, why do you not want
> > > > > > userspace to
> > > > > > also know it?
> > > >
> > > > To clarify, the attribute exposed by the firmware today is
> > > > "ExternalFacingPort" and "external-facing" respectively:
> > > >
> > > > 617654aae50e ("PCI / ACPI: Identify untrusted PCI devices")
> > > > 9cb30a71ac45d("PCI: OF: Support "external-facing" property")
> > > >
> > > > The kernel flag was named "untrusted" though, hence the assumption
> > > > that "external=untrusted" is currently baked into the kernel today.
> > > > IMHO, using "external" would fix that (The assumption can thus be
> > > > contained in the IOMMU drivers) and at the same time allow more use of
> > > > this attribute.
> > > >
> > > > > >
> > > > > > Trust is different, yes, don't get the two mixed up please. That
> > > > > > should
> > > > > > be a different sysfs attribute for obvious reasons.
> > > > >
> > > > > Yes, as a bottom line that's what I meant as well.
> > > >
> > > > So what is the consensus here? I don't have a strong opinion - but it
> > > > seemed to me Greg is saying "external" and Andy is saying "untrusted"?
> > >
> > > Those two things are totally separate things when it comes to a device.
> >
> > Agree that these are two separate attributes, and better not mixed.
>
> +1.
>
> >
> > >
> > > One (external) describes the location of the device in the system.
> > >
> > > The other (untrusted) describes what you want the kernel to do with this
> > > device (trust or not trust it).
> > >
>
Re: [PATCH 2/2] pci: Add parameter to disable attaching untrusted devices
On Thu, Jun 25, 2020 at 05:27:10PM -0700, Rajat Jain wrote: > Introduce a PCI parameter that disables the automatic attachment of > untrusted devices to their drivers. You didn't document this new api anywhere :( ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
