arm: dma-mapping: Fix mapping size value

PATCH RESEND New Ritesh Harjani 4 participants 9 messages Apr 21, 2014 – Apr 23, 2014 Arm (32-bit)

lore

Share & tools

lore

Ritesh Harjani

author

2014-04-21 06:47 UTC #0 lore raw

PATCH RESEND 1/1 arm: dma-mapping: Fix mapping size value

Previous patch message got corrupted by my mail-server (copy-paste) So resending it.

[history]: http://www.spinics.net/lists/arm-kernel/msg323904.html

Ritesh Harjani (1):
arm: dma-mapping: Fix mapping size value

 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Signature · 1 line

1.8.1.3

Ritesh Harjani

author

2014-04-21 06:47 UTC #1 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:

Unable to handle kernel NULL pointer dereference at virtual

address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164

Fix it by correcting mapping->size value.

Signed-off-by Ritesh Harjani <[email protected]>

Acked-by Laurent Pinchart <[email protected]>

---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mm/dma-mapping.c +1 -1

index f62aa06..6b00be1 100644
@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
 	mapping->nr_bitmaps = 1;
 	mapping->extensions = extensions;
 	mapping->base = base;
-	mapping->size = bitmap_size << PAGE_SHIFT;
 	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->size = mapping->bits << PAGE_SHIFT;
 
 	spin_lock_init(&mapping->lock);

Signature · 1 line

1.8.1.3

Will Deacon

2014-04-22 08:53 UTC #2 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

Quoted 25 lines · Ritesh Harjani

68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164
Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +0 -0
index f62aa06..6b00be1 100644

arch/arm/mm/dma-mapping.c arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) · line 1967

Quoted 6 lines

arch/arm/mm/dma-mapping.c +1 -1

@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
 	mapping->nr_bitmaps = 1;
 	mapping->extensions = extensions;
 	mapping->base = base;
-	mapping->size = bitmap_size << PAGE_SHIFT;
 	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->size = mapping->bits << PAGE_SHIFT;

Ok, but given that mapping->size is derived from mapping->bits, do we really need both of these fields in struct dma_iommu_mapping?

Will

Marek Szyprowski

2014-04-23 08:53 UTC #3 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

Hello,

Quoted 21 lines · Will Deacon

Quoted 20 lines · Ritesh Harjani
68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164
Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 14 lines

arch/arm/mm/dma-mapping.c arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) · line 1967
Quoted 11 lines
arch/arm/mm/dma-mapping.c +1 -1
index f62aa06..6b00be1 100644
@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
 	mapping->nr_bitmaps = 1;
 	mapping->extensions = extensions;
 	mapping->base = base;
-	mapping->size = bitmap_size << PAGE_SHIFT;
 	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->size = mapping->bits << PAGE_SHIFT;
Ok, but given that mapping->size is derived from mapping->bits, do we really need both of these fields in struct dma_iommu_mapping?

You are right. I didn't notice this while I was refactoring the code. Ritesh,
could you update your patch and simply replace all references of mapping->size
with (mapping->bits << PAGE_SHIFT), probably with some temporary variable to make the code easier to understand? I've didn't apply your patch yet.

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Laurent Pinchart

2014-04-23 09:30 UTC #4 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

Hi Marek,

Quoted 42 lines · On Wednesday 23 April 2014 10:53:24 Marek Szyprowski wrote:

Quoted 32 lines · Will Deacon
Quoted 31 lines · Ritesh Harjani
68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164
Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +0 -0
index f62aa06..6b00be1 100644
@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus,
dma_addr_t base, size_t size)> >
arch/arm/mm/dma-mapping.c +0 -0
 	mapping->nr_bitmaps = 1;
 	mapping->extensions = extensions;
 	mapping->base = base;
Quoted 8 lines
arch/arm/mm/dma-mapping.c arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
Quoted 5 lines
arch/arm/mm/dma-mapping.c +1 -1
-	mapping->size = bitmap_size << PAGE_SHIFT;
 
 	mapping->bits = BITS_PER_BYTE * bitmap_size;
 
+	mapping->size = mapping->bits << PAGE_SHIFT;
Ok, but given that mapping->size is derived from mapping->bits, do we really need both of these fields in struct dma_iommu_mapping?

Quoted 5 lines

You are right. I didn't notice this while I was refactoring the code. Ritesh, could you update your patch and simply replace all references of mapping->size with (mapping->bits << PAGE_SHIFT), probably with some temporary variable to make the code easier to understand? I've didn't apply your patch yet.

As this patch fixes a v3.15 regression, shouldn't it be applied as-is and ASAP, with the cleanup that removes mapping->size coming in a later, less urgent patch ?

Signature · 3 lines

Regards,

Laurent Pinchart

Ritesh Harjani

author

2014-04-23 10:04 UTC #5 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

Hi Marek/Will

Quoted 51 lines · Laurent Pinchart

Hi Marek,
Quoted 42 lines · On Wednesday 23 April 2014 10:53:24 Marek Szyprowski wrote:
Quoted 32 lines · Will Deacon
Quoted 31 lines · Ritesh Harjani
68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164
Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +0 -0
index f62aa06..6b00be1 100644
@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus,
dma_addr_t base, size_t size)> >
mapping->nr_bitmaps = 1;
mapping->extensions = extensions;
mapping->base = base;
Quoted 8 lines
Quoted 5 lines
- mapping->size = bitmap_size << PAGE_SHIFT;
  mapping->bits = BITS_PER_BYTE * bitmap_size;
+ mapping->size = mapping->bits << PAGE_SHIFT;
Ok, but given that mapping->size is derived from mapping->bits, do we really need both of these fields in struct dma_iommu_mapping?
Quoted 5 lines
You are right. I didn't notice this while I was refactoring the code. Ritesh, could you update your patch and simply replace all references of mapping->size with (mapping->bits << PAGE_SHIFT), probably with some temporary variable to make the code easier to understand? I've didn't apply your patch yet.

Quoted 3 lines

As this patch fixes a v3.15 regression, shouldn't it be applied as-is and ASAP, with the cleanup that removes mapping->size coming in a later, less urgent patch ?

I agree with Laurent. Anyway this cleanup can be taken care when we will be doing refactoring of common code to lib/iommu-helper.c.

Anyways, if you still insist I can prepare and submit the patch. Let me know again on this.

Quoted 6 lines

--
Regards,
Laurent Pinchart

Thanks
Ritesh

Marek Szyprowski

2014-04-23 13:17 UTC #6 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

Hello,

Quoted 59 lines · Ritesh Harjani

Hi Marek/Will
Quoted 51 lines · Laurent Pinchart
Hi Marek,
Quoted 42 lines · On Wednesday 23 April 2014 10:53:24 Marek Szyprowski wrote:
Quoted 32 lines · Will Deacon
Quoted 31 lines · Ritesh Harjani
68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164
Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +0 -0
index f62aa06..6b00be1 100644
@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus,
dma_addr_t base, size_t size)> >
mapping->nr_bitmaps = 1;
mapping->extensions = extensions;
mapping->base = base;
Quoted 8 lines
Quoted 5 lines
- mapping->size = bitmap_size << PAGE_SHIFT;
  mapping->bits = BITS_PER_BYTE * bitmap_size;
+ mapping->size = mapping->bits << PAGE_SHIFT;
Ok, but given that mapping->size is derived from mapping->bits, do we really need both of these fields in struct dma_iommu_mapping?
Quoted 5 lines
You are right. I didn't notice this while I was refactoring the code. Ritesh, could you update your patch and simply replace all references of mapping->size with (mapping->bits << PAGE_SHIFT), probably with some temporary variable to make the code easier to understand? I've didn't apply your patch yet.
Quoted 3 lines
As this patch fixes a v3.15 regression, shouldn't it be applied as-is and ASAP, with the cleanup that removes mapping->size coming in a later, less urgent patch ?

Quoted 5 lines

I agree with Laurent. Anyway this cleanup can be taken care when we will be doing refactoring of common code to lib/iommu-helper.c.
Anyways, if you still insist I can prepare and submit the patch. Let me know again on this.

Ok, I've merged the patch as is and I will send pull request soon. Please include the above discussed cleanup while refactoring common code to lib.

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland

Ritesh Harjani

author

2014-04-23 13:22 UTC #7 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

Ok thanks Marek,

I was about to send a new patch (as I had now got hold of my system). Anyways, I will add this discussion of cleaning up this variable in my to-do list.

Thanks
Ritesh

Quoted 88 lines · Marek Szyprowski

Hello,
Quoted 65 lines · Ritesh Harjani
Hi Marek/Will
Quoted 54 lines · Laurent Pinchart
Hi Marek,
Quoted 43 lines · On Wednesday 23 April 2014 10:53:24 Marek Szyprowski wrote:
Quoted 33 lines · Will Deacon
Quoted 32 lines · Ritesh Harjani
68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164
Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>
---
 arch/arm/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mm/dma-mapping.c +0 -0
index f62aa06..6b00be1 100644
@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type
 *bus,
dma_addr_t base, size_t size)> >
mapping->nr_bitmaps = 1;
mapping->extensions = extensions;
mapping->base = base;
Quoted 8 lines
Quoted 5 lines
- mapping->size = bitmap_size << PAGE_SHIFT;
  mapping->bits = BITS_PER_BYTE * bitmap_size;
+ mapping->size = mapping->bits << PAGE_SHIFT;
Ok, but given that mapping->size is derived from mapping->bits, do we really need both of these fields in struct dma_iommu_mapping?
Quoted 7 lines
You are right. I didn't notice this while I was refactoring the code. Ritesh, could you update your patch and simply replace all references of
mapping->size with (mapping->bits << PAGE_SHIFT), probably with some temporary variable to make the code easier to understand? I've didn't apply
your patch yet.
Quoted 5 lines
As this patch fixes a v3.15 regression, shouldn't it be applied as-is and
ASAP, with the cleanup that removes mapping->size coming in a later, less
urgent patch ?
Quoted 7 lines
I agree with Laurent. Anyway this cleanup can be taken care when we will be
doing refactoring of common code to lib/iommu-helper.c.
Anyways, if you still insist I can prepare and submit the patch. Let me know
again on this.
Ok, I've merged the patch as is and I will send pull request soon. Please include the above discussed cleanup while refactoring common code to lib.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Marek Szyprowski

2014-04-22 09:09 UTC #8 lore raw

PATCH 1/1 arm: dma-mapping: Fix mapping size value

In reply to Ritesh Harjani

Hello,

Quoted 12 lines · Ritesh Harjani

68efd7d2fb("arm: dma-mapping: remove order parameter from arm_iommu_create_mapping()") is causing kernel panic because it wrongly sets the value of mapping->size:
Unable to handle kernel NULL pointer dereference at virtual
address 000000a0
pgd = e7a84000
[000000a0] *pgd=00000000
...
PC is at bitmap_clear+0x48/0xd0
LR is at __iommu_remove_mapping+0x130/0x164

Quoted 4 lines

Fix it by correcting mapping->size value.
Signed-off-by Ritesh Harjani <[email protected]>
Acked-by Laurent Pinchart <[email protected]>

Thanks for spotting this issue! I'm really sorry for introducing it. I will push it to the fixes branch asap.

Quoted 9 lines

---
  arch/arm/mm/dma-mapping.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mm/dma-mapping.c +0 -0

index f62aa06..6b00be1 100644

arch/arm/mm/dma-mapping.c arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size) · line 1969

Quoted 8 lines

arch/arm/mm/dma-mapping.c +1 -1

@@ -1963,8 +1963,8 @@ arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size)
  	mapping->nr_bitmaps = 1;
  	mapping->extensions = extensions;
  	mapping->base = base;
-	mapping->size = bitmap_size << PAGE_SHIFT;
  	mapping->bits = BITS_PER_BYTE * bitmap_size;
+	mapping->size = mapping->bits << PAGE_SHIFT;
 
  	spin_lock_init(&mapping->lock);

Best regards

Signature · 2 lines

Marek Szyprowski, PhD
Samsung R&D Institute Poland