DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT
cover DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT
Rob's recent pull request (Highbank fixes for 3.12) promoted the dma_addr_t to 64bit:
commit ac072f427a4266b28a948c67a97a8c46f49b662d
Author: Rob Herring <[email protected]> Date: Tue Aug 13 15:55:07 2013 -0500
ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
Unfortunately this breaks the AMBA PL08x driver. Fixes are pretty easy: replacing modulo with AND and fix printk format types.
Andre Przywara (2):
DMA: fix AMBA PL08x driver issue with 64bit DMA address type
DMA: fix printk warning in AMBA PL08x DMA driver
drivers/dma/amba-pl08x.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)
Signature · 1 line
1.7.12.1
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
In Rob's recent pull request the patch
ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
promotes dma_addr_t to 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.
--- drivers/dma/amba-pl08x.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/dma/amba-pl08x.c +10 -6
@@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx) return container_of(tx, struct pl08x_txd, vd.tx); } +static int bus_addr_offset (struct pl08x_bus_data *bus) +{ +return bus->addr & (bus->buswidth -1 ); +} +/* * Mux handling. *
@@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (bus_addr_offset (&bd.srcbus) || +bus_addr_offset (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",
@@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (bus_addr_offset (mbus)) { + early_bytes = mbus->buswidth -bus_addr_offset (mbus);if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }
@@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (bus_addr_offset (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__);
Signature · 1 line
1.7.12.1
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
NAK.
Quoted 65 lines · Andre Przywara
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.Signed-off-by Andre Przywara <[email protected]>--- drivers/dma/amba-pl08x.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)drivers/dma/amba-pl08x.c +10
-6@@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx) return container_of(tx, struct pl08x_txd, vd.tx); } +static int bus_addr_offset (struct pl08x_bus_data *bus) +{ +return bus->addr & (bus->buswidth -1 ); +} +/* * Mux handling. * @@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (bus_addr_offset (&bd.srcbus) || +bus_addr_offset (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",@@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (bus_addr_offset (mbus)) { + early_bytes = mbus->buswidth -bus_addr_offset (mbus);if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }@@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (bus_addr_offset (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__);Signature · 1 line
1.7.12.1
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
Quoted 1 line · Russell King - ARM Linux
NAK.
This patch has nothing to do with dma masks or your dma mask series. The code deals with bus alignment and cleans up the code to do alignment operations in a sane way compared to modulo operator. The only thing 64-bit dma_addr_t did was expose crap code.
Perhaps bus_addr_offset needs a better name to indicate it is dealing with bus alignment rather than bus address offset.
Rob
Quoted 71 lines · Andre Przywara
Quoted 63 lines
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.Signed-off-by Andre Przywara <[email protected]>--- drivers/dma/amba-pl08x.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)drivers/dma/amba-pl08x.c +10
-7@@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx) return container_of(tx, struct pl08x_txd, vd.tx); } +static int bus_addr_offset (struct pl08x_bus_data *bus) +{ +return bus->addr & (bus->buswidth -1 ); +} +/* * Mux handling. * @@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (bus_addr_offset (&bd.srcbus) || +bus_addr_offset (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",@@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (bus_addr_offset (mbus)) { + early_bytes = mbus->buswidth -bus_addr_offset (mbus);if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }@@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (bus_addr_offset (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__); --Quoted 2 lines
1.7.12.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
Quoted 8 lines · Rob Herring
Quoted 1 line · Russell King - ARM Linux
NAK.
This patch has nothing to do with dma masks or your dma mask series. The code deals with bus alignment and cleans up the code to do alignment operations in a sane way compared to modulo operator. The only thing 64-bit
dma_addr_tdid was expose crap code.
I agree. Actually I'd see the DMA mask thing just as an opportunity to fix this code, not as the reason. I guess there are gazillions of drivers in the ARM world which have problems with any address related variable being bigger than 32bit, and those should all be fixed eventually.
Quoted 2 lines
Perhaps
bus_addr_offsetneeds a better name to indicate it is dealing with bus alignment rather than bus address offset.
Actually my first patch version called this function unaligned_bus_addr(), but this was rather odd with the one non-boolean usage of it - where it actually wants to know the offset.
Regards,
Andre.
Quoted 71 lines
Quoted 71 lines · Andre Przywara
Quoted 63 lines
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.Signed-off-by Andre Przywara <[email protected]>--- drivers/dma/amba-pl08x.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)drivers/dma/amba-pl08x.c +10
-7@@ -286,6 +286,11 @@ static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx) return container_of(tx, struct pl08x_txd, vd.tx); } +static int bus_addr_offset (struct pl08x_bus_data *bus) +{ +return bus->addr & (bus->buswidth -1 ); +} +/* * Mux handling. * @@ -886,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (bus_addr_offset (&bd.srcbus) || +bus_addr_offset (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",@@ -908,9 +913,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (bus_addr_offset (mbus)) { + early_bytes = mbus->buswidth -bus_addr_offset (mbus);if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }@@ -928,7 +932,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (bus_addr_offset (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__); --Quoted 2 lines
1.7.12.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
Quoted 10 lines · Andre Przywara
Quoted 8 lines · Rob Herring
Quoted 1 line · Russell King - ARM Linux
NAK.
This patch has nothing to do with dma masks or your dma mask series. The code deals with bus alignment and cleans up the code to do alignment operations in a sane way compared to modulo operator. The only thing 64-bit
dma_addr_tdid was expose crap code.
Quoted 5 lines
I agree. Actually I'd see the DMA mask thing just as an opportunity to fix this code, not as the reason. I guess there are gazillions of drivers in the ARM world which have problems with any address related variable being bigger than 32bit, and those should all be fixed eventually.
No they shouldn't. If the DMA address registers are 32-bit, then the DMA engine can *only* handle 32-bit addresses.
That's partly the point of my patch set: if the DMA registers are 32-bit, then the device driver should set a 32-bit DMA mask to tell the rest of the kernel that this driver only supports 32-bit addresses.
If DMA bus addresses of 0-4G translates to memory in the range (say) of 4G-8G physical, and you have memory in the region of 4G-8G, then that should work. At the moment, it doesn't - and that's what those patches address.
The fact that the PL08x driver was ending up with DMA addresses in the 4G-8G range is a bug. DMA addresses after mapping are supposed to be the _exact_ value you program into the hardware. If that value is not, then the DMA API is buggy for that platform.
That all said, yes this patch is mostly fine, except:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/dma/amba-pl08x.c static inline struct pl08x_txd *to_pl08x_txd(struct dma_async_tx_descriptor *tx) · line 292
Quoted 4 lines
drivers/dma/amba-pl08x.c +4
-0+static int bus_addr_offset (struct pl08x_bus_data *bus) +{ +return bus->addr & (bus->buswidth -1 ); +}
that this is a poor name for this function - and we don't need it because we have a generic form of this - IS_ALIGNED(). Is there any reason to re-code what's already provided?
PATCH v2 1/1 DMA: fix AMBA PL08x compilation issue with 64bit DMA address type
When dma_addr_t is 64 bits long, compilation of the AMBA PL08x DMA driver breaks due to a missing 64bit%8bit modulo operation. Looking more closely the divisor in these operations can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced with simple bit masking.
Change from v1:
Replace open-coded function with existing IS_ALIGNED macro and use a macro around that to avoid a line becoming too long.
--- drivers/dma/amba-pl08x.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
drivers/dma/amba-pl08x.c +8 -6
@@ -133,6 +133,8 @@ struct pl08x_bus_data {u8 buswidth; }; +#define IS_BUS_ALIGNED (bus) IS_ALIGNED((bus)->addr, (bus)->buswidth) +/** * struct pl08x_phy_chan - holder for the physical channels * @id: physical index to this channel
@@ -889,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (!IS_BUS_ALIGNED (&bd.srcbus) || + !IS_BUS_ALIGNED (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",
@@ -911,9 +913,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (!IS_BUS_ALIGNED (mbus)) { + early_bytes = mbus->buswidth - + (mbus->addr & (mbus->buswidth -1 ));if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }
@@ -931,7 +933,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (!IS_BUS_ALIGNED (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__);
Signature · 1 line
1.7.12.1
PATCH v2 1/1 DMA: fix AMBA PL08x compilation issue with 64bit DMA address type
Quoted 11 lines · Andre Przywara
When
dma_addr_tis 64 bits long, compilation of the AMBA PL08x DMA driver breaks due to a missing 64bit%8bit modulo operation. Looking more closely the divisor in these operations can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced with simple bit masking.Change from v1:
Replace open-coded function with existingIS_ALIGNEDmacro and use a macro around that to avoid a line becoming too long.Signed-off-by Andre Przywara <[email protected]>
Vinod,
Can I have your ack and take this thru arm-soc tree. I'm dependent on this to select 64-bit dma_addr_t for highbank.
Rob
Quoted 51 lines
--- drivers/dma/amba-pl08x.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)drivers/dma/amba-pl08x.c +8
-6@@ -133,6 +133,8 @@ struct pl08x_bus_data {u8 buswidth; }; +#define IS_BUS_ALIGNED (bus) IS_ALIGNED((bus)->addr, (bus)->buswidth) +/** * struct pl08x_phy_chan - holder for the physical channels * @id: physical index to this channel @@ -889,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (!IS_BUS_ALIGNED (&bd.srcbus) || + !IS_BUS_ALIGNED (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",@@ -911,9 +913,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (!IS_BUS_ALIGNED (mbus)) { + early_bytes = mbus->buswidth - + (mbus->addr & (mbus->buswidth -1 ));if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }@@ -931,7 +933,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (!IS_BUS_ALIGNED (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__);
PATCH v2 1/1 DMA: fix AMBA PL08x compilation issue with 64bit DMA address type
In reply to Andre PrzywaraQuoted 11 lines · Andre Przywara
When
dma_addr_tis 64 bits long, compilation of the AMBA PL08x DMA driver breaks due to a missing 64bit%8bit modulo operation. Looking more closely the divisor in these operations can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced with simple bit masking.Change from v1:
Replace open-coded function with existingIS_ALIGNEDmacro and use a macro around that to avoid a line becoming too long.Signed-off-by Andre Przywara <[email protected]>
~Vinod
Quoted 53 lines
--- drivers/dma/amba-pl08x.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)drivers/dma/amba-pl08x.c +8
-6@@ -133,6 +133,8 @@ struct pl08x_bus_data {u8 buswidth; }; +#define IS_BUS_ALIGNED (bus) IS_ALIGNED((bus)->addr, (bus)->buswidth) +/** * struct pl08x_phy_chan - holder for the physical channels * @id: physical index to this channel @@ -889,8 +891,8 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,return 0 ; } -if ((bd.srcbus.addr % bd.srcbus.buswidth) || - (bd.dstbus.addr % bd.dstbus.buswidth)) { +if (!IS_BUS_ALIGNED (&bd.srcbus) || + !IS_BUS_ALIGNED (&bd.dstbus)) { dev_err(&pl08x->adev->dev, "%s src & dst address must be aligned to src" " & dst width if peripheral is flow controller",@@ -911,9 +913,9 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,*/ if (bd.remainder < mbus->buswidth) early_bytes = bd.remainder; -else if ((mbus->addr) % (mbus->buswidth)) { - early_bytes = mbus->buswidth - (mbus->addr) % - (mbus->buswidth); +else if (!IS_BUS_ALIGNED (mbus)) { + early_bytes = mbus->buswidth - + (mbus->addr & (mbus->buswidth -1 ));if ((bd.remainder - early_bytes) < mbus->buswidth) early_bytes = bd.remainder; }@@ -931,7 +933,7 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,* Master now aligned * - if slave is not then we must set its width down */ -if (sbus->addr % sbus->buswidth) { +if (!IS_BUS_ALIGNED (sbus)) {dev_dbg (&pl08x->adev->dev,"%s set down bus width to one byte\n" , __func__);Signature · 1 line
1.7.12.1
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
In reply to Andre PrzywaraQuoted 10 lines · Andre Przywara
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.Signed-off-by Andre Przywara <[email protected]>
Can you give some context? Which platform have you tested this on after the change, or is this only compile-tested?
Of the current defconfigs selecting this driver, none is 64bit, so makes me a bit curious if there is really some PL08x on the Highbank or similar?
Yours,
Linus Walleij
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
Quoted 12 lines · Linus Walleij
Quoted 9 lines · Andre Przywara
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.
Quoted 4 lines
Quoted 1 line
Signed-off-by Andre Przywara <[email protected]>Can you give some context? Which platform have you tested this on after the change, or is this only compile-tested?
Only compile tested (sorry, forgot to mention that). Actually I don't have this hardware anywhere, I just happened to had this in my config - probably just by accident as a leftover from some experiments.
So if you have access to any of those platforms, please give it a test.
Quoted 1 line
Of the current defconfigs selecting this driver, none is 64bit,
But it would be selected by allyesconfig or randconfig, right? And with more and more platforms moving into single zImage, we will spot more of those, as one "64-bit platform" will promote the types for every other.
Quoted 2 lines
so makes me a bit curious if there is really some PL08x on the Highbank or similar?
Sorry, no hidden DMA controller on Highbank ;-)
Regards,
Andre.
PATCH 1/2 DMA: fix AMBA PL08x driver issue with 64bit DMA address type
In reply to Linus WalleijQuoted 12 lines · Linus Walleij
Quoted 9 lines · Andre Przywara
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, which breaks compilation of the AMBA PL08x DMA driver.
GCC has no function for the 64bit/8bit modulo operation. Looking more closely the divisor can only be 1, 2 or 4, so the full featured '%' modulo operation is overkill and can be replaced by simple bit masking.
Quoted 8 lines
Quoted 1 line
Signed-off-by Andre Przywara <[email protected]>Can you give some context? Which platform have you tested this on after the change, or is this only compile-tested?
Of the current defconfigs selecting this driver, none is 64bit, so makes me a bit curious if there is really some PL08x on the Highbank or similar?
I thought the whole PL08x thing was just a side-effect, no?
The issues here I see that Rob is trying to fix:
* If you want to put a 64-bit address into a DMA controller somewhere
- be it a register (64-bit) or descriptor (64-bit), dma_addr_t is how the kernel handles it. If dma_addr_t is only 32-bit, how can you specify the address there? How can you do some mapping and return a pointer into 64-bit address space into a 32-bit value? How can you even allocate memory (dma_alloc_coherent, for example) for the range you want?
The objections here are obvious - as below:
1) In the case where you have an LPAE platform which enables Rob's patch, and you use dma_addr_t and the write descriptors in memory that are 32-bit or registers that are 32-bit using specific knowledge of the size of dma_addr_t or just assume it's a 32-bit wide value (why would anyone do that?) then these drivers ARE buggy and should be fixed. But that could be all of them if they're not setting their masks correctly.. considering you can mix DMA controllers in a system, some with 32-bit address spaces and some with >32-bit address spaces, making dma_addr_t 64-bit might fix one and break all the others depending on driver quality (i.e. masks..)
2) dma_addr_t being 64-bit everywhere couldn't be that much of a problem, though, with properly written drivers. The main culprits will be (if they're broken):
- If they're writing values into descriptors and just using writel() then surely there'll be a compiler warning first, which may be ignored by the developer, and then end up writing the wrong 32-bit value to memory or just the lower 32-bits giving a perfectly 'random' DMA failure where it silently walks over something 4GB below where it needed to be or somewhere else (so you'd see memory trashing, random bus faults..)
- Passing dma_addr_t in function calls to assembly code, with an assumption of the register usage in the following case where it's called from C (rarer but, could happen):
void write_some_dma_thing_in_assembly(int a, dma_addr_t b, char c);
-> r0, r1, r2 on 32-bit dma_addr_t
-> r0, r2+r3, stack on 64-bit dma_addr_t
If the assembly code assumes the dma_addr_t is 32-bit and therefore the address (per EABI) is in r1 and not r2+r3 it will get garbage from elsewhere in the 64-bit dma_addr_t case, and plug it into the target descriptor or register. Same memory trashing and random bus faults as above.
They're going to explode and there's not going to be ANY warning until there's an explosion..
On the second point, I wonder if sparse or checkpatch can figure out somehow if a 64-bit value is being passed as the second argument to a function where the first argument is smaller, and print a nitpick warning about how one might want to be careful about doing this considering possible ABI differences depending on possible configuration differences? Does it already?
~~
So, here's the question; how do you determine what size dma_addr_t is considering that it may be possible that the actual size for the DMA controller may be either, but you have a minimum of 64-bit because at least ONE DMA controller needs that can address the full range? If any drivers are accidentally setting >32-bit masks on non-LPAE then there're bad addresses coming back anyway and these should have been caught already, right?
So nothing will magically get trashed in the beginning, only during rafts of patches to "update the dma mask to full 64-bit" where it actually depends on the runtime CPU and config options in play anyway?
I recall something around late-to-discontinued time on Windows XP/Server 2003 where they quit certifying 32-bit drivers that would bail or do weird things given a 64-bit pointer. The driver core of Windows managed to move up to 64-bit dma addresses (even on "32-bit" hardware) internally forcing the driver developers with it. The main
reason: every x86 chip worth a damn that they supported had PAE so it could have had 8GB of RAM, on Server 2003 Datacenter Edition I think the upper limit was 32GB. It was perfectly possible to have a "32-bit" system with DMA going right up to the end of the extended physical memory address..
Did I miss something here?
-- Matt
PATCH 2/2 DMA: fix printk warning in AMBA PL08x DMA driver
In reply to Andre PrzywaraIn Rob's recent pull request the patch
ARM: highbank: select ARCH_DMA_ADDR_T_64BIT for LPAE
promotes dma_addr_t to 64bit, so printk generates a warning about an incorrect type. Fix this by casting it to u64 and using %llx. Fixing long lines on the way.
--- drivers/dma/amba-pl08x.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/dma/amba-pl08x.c +6 -3
@@ -850,10 +850,13 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,pl08x_choose_master_bus (&bd, &mbus, &sbus, cctl); -dev_vdbg (&pl08x->adev->dev,"src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n" , - bd.srcbus.addr, cctl &PL080_CONTROL_SRC_INCR ?"+" :"" , +dev_vdbg (&pl08x->adev->dev, +"src=0x%08llx%s/%u dst=0x%08llx%s/%u len=%zu\n" , + (u64 )bd.srcbus.addr, + cctl &PL080_CONTROL_SRC_INCR ?"+" :"" , bd.srcbus.buswidth, - bd.dstbus.addr, cctl &PL080_CONTROL_DST_INCR ?"+" :"" , + (u64 )bd.dstbus.addr, + cctl &PL080_CONTROL_DST_INCR ?"+" :"" , bd.dstbus.buswidth, bd.remainder); dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",
Signature · 1 line
1.7.12.1
PATCH 2/2 DMA: fix printk warning in AMBA PL08x DMA driver
NAK.
Quoted 35 lines · Andre Przywara
In Rob's recent pull request the patch
ARM: highbank: selectARCH_DMA_ADDR_T_64BITfor LPAE
promotesdma_addr_tto 64bit, so printk generates a warning about an incorrect type. Fix this by casting it to u64 and using %llx. Fixing long lines on the way.Signed-off-by Andre Przywara <[email protected]>--- drivers/dma/amba-pl08x.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/dma/amba-pl08x.c +6
-3@@ -850,10 +850,13 @@ static int pl08x_fill_llis_for_desc(struct pl08x_driver_data *pl08x,pl08x_choose_master_bus (&bd, &mbus, &sbus, cctl); -dev_vdbg (&pl08x->adev->dev,"src=0x%08x%s/%u dst=0x%08x%s/%u len=%zu\n" , - bd.srcbus.addr, cctl &PL080_CONTROL_SRC_INCR ?"+" :"" , +dev_vdbg (&pl08x->adev->dev, +"src=0x%08llx%s/%u dst=0x%08llx%s/%u len=%zu\n" , + (u64 )bd.srcbus.addr, + cctl &PL080_CONTROL_SRC_INCR ?"+" :"" , bd.srcbus.buswidth, - bd.dstbus.addr, cctl &PL080_CONTROL_DST_INCR ?"+" :"" , + (u64 )bd.dstbus.addr, + cctl &PL080_CONTROL_DST_INCR ?"+" :"" , bd.dstbus.buswidth, bd.remainder); dev_vdbg(&pl08x->adev->dev, "mbus=%s sbus=%s\n",Signature · 1 line
1.7.12.1
cover DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT
In reply to Andre PrzywaraQuoted 11 lines · Andre Przywara
Rob's recent pull request (Highbank fixes for 3.12) promoted the
dma_addr_tto 64bit:
commitac072f427a4266b28a948c67a97a8c46f49b662dAuthor: Rob Herring <[email protected]> Date: Tue Aug 13 15:55:07 2013 -0500ARM: highbank: select
ARCH_DMA_ADDR_T_64BITfor LPAEUnfortunately this breaks the AMBA PL08x driver. Fixes are pretty easy: replacing modulo with AND and fix printk format types.
I thought I effectively said no to that patch, and told Rob to go and look at my patch series.
Your patch series is wrong to fix this. We're just piling more bad fixes on top of a bug in the generic parts of the kernel. Please stop this madness.
cover DMA: AMBA PL08x DMA driver fixes due to ARCH_DMA_ADDR_T_64BIT
Quoted 7 lines · Russell King - ARM Linux
Quoted 6 lines · Andre Przywara
Rob's recent pull request (Highbank fixes for 3.12) promoted the
dma_addr_tto 64bit:
commitac072f427a4266b28a948c67a97a8c46f49b662dAuthor: Rob Herring <[email protected]> Date: Tue Aug 13 15:55:07 2013 -0500
Quoted 8 lines
Quoted 5 lines
ARM: highbank: select
ARCH_DMA_ADDR_T_64BITfor LPAEUnfortunately this breaks the AMBA PL08x driver. Fixes are pretty easy: replacing modulo with AND and fix printk format types.
I thought I effectively said no to that patch, and told Rob to go and look at my patch series.
I thought you only objected to doing this unconditionally for all LPAE, so I made it highbank specific like exynos is.
Rob