dma-direct: Fix return value of dma_direct_supported
PATCH 1/1 dma-direct: Fix return value of dma_direct_supported
It appears that in commit 9d7a224b463e ("dma-direct: always allow dma mask <= physiscal memory size") the logic of the test was changed from a "<" to a ">=" however I don't see any reason for that change. I am assuming that there was some additional change planned, specifically I suspect the logic was intended to be reversed and possibly used for a return. Since that is the case I have gone ahead and done that.
This addresses issues I had on my system that prevented me from booting with the above mentioned commit applied on an x86_64 system w/ Intel IOMMU.
--- kernel/dma/direct.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
kernel/dma/direct.c +1 -3
@@ -301,9 +301,7 @@ int dma_direct_supported(struct device *dev, u64 mask) min_mask =min_t (u64, min_mask, (max_pfn -1 ) <<PAGE_SHIFT ); -if (mask >=phys_to_dma (dev, min_mask)) -return 0 ; -return 1 ; +return mask >=phys_to_dma (dev, min_mask); }int dma_direct_mapping_error (struct device *dev,dma_addr_t dma_addr)
Robin Murphy
reviewerQuoted 6 lines · Alexander Duyck
It appears that in commit
9d7a224b463e("dma-direct: always allow dma mask <= physiscal memory size") the logic of the test was changed from a "<" to a ">=" however I don't see any reason for that change. I am assuming that there was some additional change planned, specifically I suspect the logic was intended to be reversed and possibly used for a return. Since that is the case I have gone ahead and done that.
Bah, seems I got hung up on the min_mask code above it and totally overlooked that the condition itself got flipped. It probably also can't help that it's an int return type, but treated as a bool by callers rather than "0 for success" as int tends to imply in isolation.
Anyway, paying a bit more attention this time, I think this looks like the right fix - cheers Alex.
Robin.
Quoted 30 lines
This addresses issues I had on my system that prevented me from booting with the above mentioned commit applied on an
x86_64system w/ Intel IOMMU.Fixes 9d7a224b463e ("dma-direct: always allow dma mask <= physiscal memory size")--- kernel/dma/direct.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)kernel/dma/direct.c +1
-3@@ -301,9 +301,7 @@ int dma_direct_supported(struct device *dev, u64 mask) min_mask =min_t (u64, min_mask, (max_pfn -1 ) <<PAGE_SHIFT ); -if (mask >=phys_to_dma (dev, min_mask)) -return 0 ; -return 1 ; +return mask >=phys_to_dma (dev, min_mask); }int dma_direct_mapping_error (struct device *dev,dma_addr_t dma_addr)_______________________________________________
iommu mailing list iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Quoted 14 lines · Robin Murphy
Quoted 6 lines · Alexander Duyck
It appears that in commit
9d7a224b463e("dma-direct: always allow dma mask <= physiscal memory size") the logic of the test was changed from a "<" to a ">=" however I don't see any reason for that change. I am assuming that there was some additional change planned, specifically I suspect the logic was intended to be reversed and possibly used for a return. Since that is the case I have gone ahead and done that.Bah, seems I got hung up on the
min_maskcode above it and totally overlooked that the condition itself got flipped. It probably also can't help that it's an int return type, but treated as a bool by callers rather than "0 for success" as int tends to imply in isolation.
Quoted 4 lines
Anyway, paying a bit more attention this time, I think this looks like the right fix - cheers Alex.
Robin.
Thanks for the review.
- Alex
P.S. It looks like I forgot to add Christoph to the original mail since I had just copied the To and Cc from the original submission, so I added him to the Cc for this.
Quoted 34 lines
Quoted 25 lines
This addresses issues I had on my system that prevented me from booting with the above mentioned commit applied on an
x86_64system w/ Intel IOMMU.Fixes 9d7a224b463e ("dma-direct: always allow dma mask <= physiscal memory size")Signed-off-by Alexander Duyck <[email protected]>--- kernel/dma/direct.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)kernel/dma/direct.c +1
-3@@ -301,9 +301,7 @@ int dma_direct_supported(struct device *dev, u64 mask) min_mask =min_t (u64, min_mask, (max_pfn -1 ) <<PAGE_SHIFT ); -if (mask >=phys_to_dma (dev, min_mask)) -return 0 ; -return 1 ; +return mask >=phys_to_dma (dev, min_mask); }int dma_direct_mapping_error (struct device *dev,dma_addr_t dma_addr)Quoted 5 lines
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Quoted 7 lines · Alexander Duyck
Thanks for the review.
- Alex
P.S. It looks like I forgot to add Christoph to the original mail since I had just copied the To and Cc from the original submission, so I added him to the Cc for this.
Yes, there was some oddness with replies appearing out of nowhere that
I couldn't understand in my jetlagged state yesterday. Now thay I've
caught up on iommu list traffic it all makes sense now. I've applied this with an Acked-by for Robin added based on his reply.
Quoted 43 lines · Alexander Duyck
Quoted 14 lines · Robin Murphy
Quoted 6 lines · Alexander Duyck
It appears that in commit
9d7a224b463e("dma-direct: always allow dma mask <= physiscal memory size") the logic of the test was changed from a "<" to a ">=" however I don't see any reason for that change. I am assuming that there was some additional change planned, specifically I suspect the logic was intended to be reversed and possibly used for a return. Since that is the case I have gone ahead and done that.Bah, seems I got hung up on the
min_maskcode above it and totally overlooked that the condition itself got flipped. It probably also can't help that it's an int return type, but treated as a bool by callers rather than "0 for success" as int tends to imply in isolation.Quoted 4 lines
Anyway, paying a bit more attention this time, I think this looks like the right fix - cheers Alex.
Robin.
Thanks for the review.
- Alex
P.S. It looks like I forgot to add Christoph to the original mail since I had just copied the To and Cc from the original submission, so I added him to the Cc for this.Quoted 15 lines
Quoted 15 lines
This addresses issues I had on my system that prevented me from booting with the above mentioned commit applied on an
x86_64system w/ Intel IOMMU.Fixes 9d7a224b463e ("dma-direct: always allow dma mask <= physiscal memory size")Signed-off-by Alexander Duyck <[email protected]>--- kernel/dma/direct.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)kernel/dma/direct.c +0
-0@@ -301,9 +301,7 @@ int dma_direct_supported(struct device *dev, u64 mask)
Quoted 6 lines
Quoted 6 lines
kernel/dma/direct.c int dma_direct_supported(struct device *dev, u64 mask) · line 304
Quoted 6 lines
kernel/dma/direct.c +1
-3min_mask =min_t (u64, min_mask, (max_pfn -1 ) <<PAGE_SHIFT ); -if (mask >=phys_to_dma (dev, min_mask)) -return 0 ; -return 1 ; +return mask >=phys_to_dma (dev, min_mask);
So I think this needs to be __phys_to_dma() here. I only recently got a system that had a device where the driver only supported 32-bit DMA and found that when SME is active this returns 0 and causes the driver to fail to initialize. This is because the SME encryption bit (bit 47) is part of the check when using phys_to_dma(). During actual DMA when SME is active, bounce buffers will be used for anything that can't meet the 48-bit requirement. But for this test, using __phys_to_dma() should give the desired results, right?
If you agree with this, I'll submit a patch to make the change. I missed this in 4.19, so I'll need to submit something to stable, too. The only issue there is the 4.20 fix won't apply cleanly to 4.19.
Thanks,
Tom
Quoted 13 lines
Quoted 13 lines
Quoted 9 lines
kernel/dma/direct.c +0
-0}int dma_direct_mapping_error (struct device *dev,dma_addr_t dma_addr)_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Quoted 9 lines · Thomas
So I think this needs to be
__phys_to_dma()here. I only recently got a system that had a device where the driver only supported 32-bit DMA and found that when SME is active this returns 0 and causes the driver to fail to initialize. This is because the SME encryption bit (bit 47) is part of the check when usingphys_to_dma(). During actual DMA when SME is active, bounce buffers will be used for anything that can't meet the 48-bit requirement. But for this test, using__phys_to_dma()should give the desired results, right?
Quoted 3 lines
If you agree with this, I'll submit a patch to make the change. I missed this in 4.19, so I'll need to submit something to stable, too. The only issue there is the 4.20 fix won't apply cleanly to 4.19.
Yes, please send a patch. Please make sure it includes a code comment that explains why the __-prefixed version is used.