Determine the number of DMA channels by 'dma-channels' property
Subsystems (6)
v2 cover Determine the number of DMA channels by 'dma-channels' property
The PDMA driver currently assumes there are four channels by default, it might cause the error if there is actually less than four channels. Change that by getting number of channel dynamically from device tree. For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
This patch set contains the dts and dt-bindings change.
Changed in v2:
- Rebase on tag v5.16
- Use 4 as default value of dma-channels
Zong Li (3):
riscv: dts: Add dma-channels property in dma node
dt-bindings: Add dma-channels for pdma device node
dmaengine: sf-pdma: Get number of channel by device tree
.../bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++ arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + drivers/dma/sf-pdma/sf-pdma.c | 15 +++++++++------ drivers/dma/sf-pdma/sf-pdma.h | 8 ++------ 5 files changed, 20 insertions(+), 12 deletions(-)
Signature · 1 line
2.31.1
PATCH v2 1/3 riscv: dts: Add dma-channels property in dma node
Add dma-channels property, then we can determine how many channels there by device tree.
--- arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 + 2 files changed, 2 insertions(+)
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi +1 -0
@@ -188,6 +188,7 @@ dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; };
arch/riscv/boot/dts/sifive/fu540-c000.dtsi +1 -0
@@ -171,6 +171,7 @@ dma: dma@3000000 { reg = <0x0 0x3000000 0x0 0x8000>; interrupt-parent = <&plic0>; interrupts = <23 24 25 26 27 28 29 30>; + dma-channels = <4>; #dma-cells = <1>; }; uart1: serial@10011000 {
Signature · 1 line
2.31.1
PATCH v2 2/3 dt-bindings: Add dma-channels for pdma device node
In reply to Zong LiAdd dma-channels property, then we can determine how many channels there by device tree, rather than statically defines it in PDMA driver
--- .../devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++ 1 file changed, 7 insertions(+)
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml +7 -0
@@ -34,6 +34,12 @@ properties: minItems: 1 maxItems: 8 + dma-channels: + description: For backwards-compatible, the default value is 4 + minimum: 1 + maximum: 4 + default: 4 + '#dma-cells': const: 1
@@ -50,6 +56,7 @@ examples: dma@3000000 { compatible = "sifive,fu540-c000-pdma"; reg = <0x3000000 0x8000>; + dma-channels = <4>; interrupts = <23 24 25 26 27 28 29 30>; #dma-cells = <1>; };
Signature · 1 line
2.31.1
PATCH v2 3/3 dmaengine: sf-pdma: Get number of channel by device tree
In reply to Zong LiIt currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
--- drivers/dma/sf-pdma/sf-pdma.c | 15 +++++++++------ drivers/dma/sf-pdma/sf-pdma.h | 8 ++------ 2 files changed, 11 insertions(+), 12 deletions(-)
drivers/dma/sf-pdma/sf-pdma.c +9 -6
@@ -484,21 +484,24 @@ static int sf_pdma_probe(struct platform_device *pdev)struct sf_pdma *pdma;struct sf_pdma_chan *chan;struct resource *res; -int len, chans; -int ret; +int len, ret;const enum dma_slave_buswidth widths =DMA_SLAVE_BUSWIDTH_1_BYTE |DMA_SLAVE_BUSWIDTH_2_BYTES |DMA_SLAVE_BUSWIDTH_4_BYTES |DMA_SLAVE_BUSWIDTH_8_BYTES |DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; + len =sizeof (*pdma) +sizeof (*chan) *PDMA_MAX_NR_CH ; pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);
@@ -556,7 +559,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];devm_free_irq (&pdev->dev, ch->txirq, ch);
drivers/dma/sf-pdma/sf-pdma.h +2 -6
@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000
@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */
Signature · 1 line
2.31.1
Re: [PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Hi Zong,
Quoted 8 lines · Zong Li
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Signed-off-by Zong Li <[email protected]>
Thanks for your patch!
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-2@@ -484,21 +484,24 @@ static int sf_pdma_probe(struct platform_device *pdev)struct sf_pdma *pdma;struct sf_pdma_chan *chan;struct resource *res; -int len, chans; -int ret;
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 494
Quoted 10 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-2+int len, ret;const enum dma_slave_buswidth widths =DMA_SLAVE_BUSWIDTH_1_BYTE |DMA_SLAVE_BUSWIDTH_2_BYTES |DMA_SLAVE_BUSWIDTH_4_BYTES |DMA_SLAVE_BUSWIDTH_8_BYTES |DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; + len =sizeof (*pdma) +sizeof (*chan) *PDMA_MAX_NR_CH ;
Why is the last part added (yes, this is a pre-existing issue)? struct sf_pdma already contains space for chans[PDMA_MAX_NR_CH]. Either drop the last part, or change sf_pdma.chans[] to a flexible array member.
BTW, you can use the struct_size() or flex_array_size() helper to calculate len.
Quoted 15 lines
drivers/dma/sf-pdma/sf-pdma.c +6
-1pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_remove(struct platform_device *pdev) · line 563
Quoted 6 lines
drivers/dma/sf-pdma/sf-pdma.c +1
-1@@ -556,7 +559,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];
If dma-channels in DT > PDMA_NR_CH, this becomes an out-of-bound access.
Quoted 20 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-0devm_free_irq (&pdev->dev, ch->txirq, ch);drivers/dma/sf-pdma/sf-pdma.h +1
-5@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000
drivers/dma/sf-pdma/sf-pdma.h struct sf_pdma { · line 120
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.h +1
-1@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */
-
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 39 lines · Geert Uytterhoeven
Hi Zong,
Quoted 8 lines · Zong Li
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Signed-off-by Zong Li <[email protected]>Thanks for your patch!
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-2@@ -484,21 +484,24 @@ static int sf_pdma_probe(struct platform_device *pdev)struct sf_pdma *pdma;struct sf_pdma_chan *chan;struct resource *res; -int len, chans; -int ret;drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 494
Quoted 10 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-2+int len, ret;const enum dma_slave_buswidth widths =DMA_SLAVE_BUSWIDTH_1_BYTE |DMA_SLAVE_BUSWIDTH_2_BYTES |DMA_SLAVE_BUSWIDTH_4_BYTES |DMA_SLAVE_BUSWIDTH_8_BYTES |DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; + len =sizeof (*pdma) +sizeof (*chan) *PDMA_MAX_NR_CH ;Why is the last part added (yes, this is a pre-existing issue)? struct
sf_pdmaalready contains space for chans[PDMA_MAX_NR_CH]. Either drop the last part, or change sf_pdma.chans[] to a flexible array member.
Quoted 2 lines
BTW, you can use the
struct_size()orflex_array_size()helper to calculate len.
Thanks for your suggestions, let me fix it in the next version.
Quoted 19 lines
Quoted 19 lines
drivers/dma/sf-pdma/sf-pdma.c +6
-1pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);@@ -556,7 +559,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i;
Quoted 7 lines
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_remove(struct platform_device *pdev) · line 563
Quoted 3 lines
drivers/dma/sf-pdma/sf-pdma.c +1
-1-for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];If dma-channels in DT >
PDMA_NR_CH, this becomes an out-of-bound access.
Okay, let me get the min() between pdma->chans and PDMA_MAX_NR_CH, please let me know if it isn't good to you.
Quoted 39 lines
Quoted 20 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-0devm_free_irq (&pdev->dev, ch->txirq, ch);drivers/dma/sf-pdma/sf-pdma.h +1
-5@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000drivers/dma/sf-pdma/sf-pdma.h struct sf_pdma { · line 120
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.h +1
-1@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */ -
Gr{oetje,eeting}s,Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 73 lines · Zong Li
Quoted 39 lines · Geert Uytterhoeven
Hi Zong,
Quoted 8 lines · Zong Li
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Signed-off-by Zong Li <[email protected]>Thanks for your patch!
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-2@@ -484,21 +484,24 @@ static int sf_pdma_probe(struct platform_device *pdev)struct sf_pdma *pdma;struct sf_pdma_chan *chan;struct resource *res; -int len, chans; -int ret;drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 494
Quoted 10 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-2+int len, ret;const enum dma_slave_buswidth widths =DMA_SLAVE_BUSWIDTH_1_BYTE |DMA_SLAVE_BUSWIDTH_2_BYTES |DMA_SLAVE_BUSWIDTH_4_BYTES |DMA_SLAVE_BUSWIDTH_8_BYTES |DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; + len =sizeof (*pdma) +sizeof (*chan) *PDMA_MAX_NR_CH ;Why is the last part added (yes, this is a pre-existing issue)? struct
sf_pdmaalready contains space for chans[PDMA_MAX_NR_CH]. Either drop the last part, or change sf_pdma.chans[] to a flexible array member.Quoted 2 lines
BTW, you can use the
struct_size()orflex_array_size()helper to calculate len.Thanks for your suggestions, let me fix it in the next version.
Quoted 19 lines
Quoted 19 lines
drivers/dma/sf-pdma/sf-pdma.c +6
-1pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);@@ -556,7 +559,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i;Quoted 7 lines
drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_remove(struct platform_device *pdev) · line 563
Quoted 3 lines
drivers/dma/sf-pdma/sf-pdma.c +1
-1-for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];If dma-channels in DT >
PDMA_NR_CH, this becomes an out-of-bound access.
Quoted 2 lines
Okay, let me get the
min()betweenpdma->chansandPDMA_MAX_NR_CH, please let me know if it isn't good to you.
Please allow me give more details on it, I would compare the value of pdma->chans with PDMA_MAX_NR_CH in probe function, and set the pdma->chans to PDMA_MAX_NR_CH if the value in DT is bigger than PDMA_MAX_NR_CH.
Quoted 40 lines
Quoted 40 lines
Quoted 21 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-0devm_free_irq (&pdev->dev, ch->txirq, ch);drivers/dma/sf-pdma/sf-pdma.h +1
-5@@ -22,11 +22,7 @@#include "../dmaengine.h" #include "../virt-dma.h" -#define PDMA_NR_CH 4 - -#if (PDMA_NR_CH !=4 ) -#error "Please define PDMA_NR_CH to 4" -#endif +#define PDMA_MAX_NR_CH 4#define PDMA_BASE_ADDR 0x3000000#define PDMA_CHAN_OFFSET 0x1000drivers/dma/sf-pdma/sf-pdma.h struct sf_pdma { · line 120
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.h +1
-1@@ -118,7 +114,7 @@ struct sf_pdma {void __iomem *membase;void __iomem *mappedbase;u32 n_chans; - struct sf_pdma_chan chans[PDMA_NR_CH]; + struct sf_pdma_chan chans[PDMA_MAX_NR_CH]; };#endif /* _SF_PDMA_H */ -
Gr{oetje,eeting}s,Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Hi Zong,
Quoted 70 lines · Zong Li
Quoted 69 lines · Zong Li
Quoted 36 lines · Geert Uytterhoeven
Quoted 8 lines · Zong Li
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Signed-off-by Zong Li <[email protected]>Thanks for your patch!
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-2@@ -484,21 +484,24 @@ static int sf_pdma_probe(struct platform_device *pdev)struct sf_pdma *pdma;struct sf_pdma_chan *chan;struct resource *res; -int len, chans; -int ret;drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 494
Quoted 10 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-2+int len, ret;const enum dma_slave_buswidth widths =DMA_SLAVE_BUSWIDTH_1_BYTE |DMA_SLAVE_BUSWIDTH_2_BYTES |DMA_SLAVE_BUSWIDTH_4_BYTES |DMA_SLAVE_BUSWIDTH_8_BYTES |DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; + len =sizeof (*pdma) +sizeof (*chan) *PDMA_MAX_NR_CH ;Why is the last part added (yes, this is a pre-existing issue)? struct
sf_pdmaalready contains space for chans[PDMA_MAX_NR_CH]. Either drop the last part, or change sf_pdma.chans[] to a flexible array member.Quoted 2 lines
BTW, you can use the
struct_size()orflex_array_size()helper to calculate len.Thanks for your suggestions, let me fix it in the next version.
Quoted 26 lines
Quoted 16 lines
drivers/dma/sf-pdma/sf-pdma.c +6
-1pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_remove(struct platform_device *pdev) · line 563
Quoted 6 lines
drivers/dma/sf-pdma/sf-pdma.c +1
-1@@ -556,7 +559,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];If dma-channels in DT >
PDMA_NR_CH, this becomes an out-of-bound access.
Quoted 7 lines
Quoted 2 lines
Okay, let me get the
min()betweenpdma->chansandPDMA_MAX_NR_CH, please let me know if it isn't good to you.Please allow me give more details on it, I would compare the value of
pdma->chanswithPDMA_MAX_NR_CHin probe function, and set thepdma->chanstoPDMA_MAX_NR_CHif the value in DT is bigger thanPDMA_MAX_NR_CH.
Silently limiting "dma-channels" to PDMA_MAX_NR_CH is not a good idea, as that may lead to hard-to-track problems.
Basically you have two options:
1. Just use the value of "dma-channels" if present.
This has the advantage that it will work automatically with future variants that have more channels, but allows the developer to trigger memory exhaustion by providing a very large value.
2. Return -EINVAL if "dma-channels" is larger than PDMA_MAX_NR_CH. This is the safest, but requires driver changes for a future variant that has more channels.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree
Quoted 85 lines · Geert Uytterhoeven
Hi Zong,
Quoted 70 lines · Zong Li
Quoted 69 lines · Zong Li
Quoted 36 lines · Geert Uytterhoeven
Quoted 8 lines · Zong Li
It currently assumes that there are always four channels, it would cause the error if there is actually less than four channels. Change that by getting number of channel from device tree.
For backwards-compatible, it uses the default value (i.e. 4) when there is no 'dma-channels' information in dts.
Signed-off-by Zong Li <[email protected]>Thanks for your patch!
Quoted 8 lines
drivers/dma/sf-pdma/sf-pdma.c +0
-2@@ -484,21 +484,24 @@ static int sf_pdma_probe(struct platform_device *pdev)struct sf_pdma *pdma;struct sf_pdma_chan *chan;struct resource *res; -int len, chans; -int ret;drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_probe(struct platform_device *pdev) · line 494
Quoted 10 lines
drivers/dma/sf-pdma/sf-pdma.c +2
-2+int len, ret;const enum dma_slave_buswidth widths =DMA_SLAVE_BUSWIDTH_1_BYTE |DMA_SLAVE_BUSWIDTH_2_BYTES |DMA_SLAVE_BUSWIDTH_4_BYTES |DMA_SLAVE_BUSWIDTH_8_BYTES |DMA_SLAVE_BUSWIDTH_16_BYTES |DMA_SLAVE_BUSWIDTH_32_BYTES |DMA_SLAVE_BUSWIDTH_64_BYTES ; - chans =PDMA_NR_CH ; - len =sizeof (*pdma) +sizeof (*chan) * chans; + len =sizeof (*pdma) +sizeof (*chan) *PDMA_MAX_NR_CH ;Why is the last part added (yes, this is a pre-existing issue)? struct
sf_pdmaalready contains space for chans[PDMA_MAX_NR_CH]. Either drop the last part, or change sf_pdma.chans[] to a flexible array member.Quoted 2 lines
BTW, you can use the
struct_size()orflex_array_size()helper to calculate len.Thanks for your suggestions, let me fix it in the next version.
Quoted 26 lines
Quoted 16 lines
drivers/dma/sf-pdma/sf-pdma.c +6
-1pdma =devm_kzalloc (&pdev->dev, len,GFP_KERNEL );if (!pdma)return -ENOMEM ; - pdma->n_chans = chans; + ret =of_property_read_u32 (pdev->dev.of_node,"dma-channels" , + &pdma->n_chans); +if (ret) { +dev_notice (&pdev->dev,"set number of channels to default value: 4\n" ); + pdma->n_chans =PDMA_MAX_NR_CH ; + } res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pdma->membase =devm_ioremap_resource (&pdev->dev, res);drivers/dma/sf-pdma/sf-pdma.c static int sf_pdma_remove(struct platform_device *pdev) · line 563
Quoted 6 lines
drivers/dma/sf-pdma/sf-pdma.c +1
-1@@ -556,7 +559,7 @@ static int sf_pdma_remove(struct platform_device *pdev)struct sf_pdma_chan *ch;int i; -for (i =0 ; i <PDMA_NR_CH ; i++) { +for (i =0 ; i < pdma->n_chans; i++) { ch = &pdma->chans[i];If dma-channels in DT >
PDMA_NR_CH, this becomes an out-of-bound access.Quoted 7 lines
Quoted 2 lines
Okay, let me get the
min()betweenpdma->chansandPDMA_MAX_NR_CH, please let me know if it isn't good to you.Please allow me give more details on it, I would compare the value of
pdma->chanswithPDMA_MAX_NR_CHin probe function, and set thepdma->chanstoPDMA_MAX_NR_CHif the value in DT is bigger thanPDMA_MAX_NR_CH.Silently limiting "dma-channels" to
PDMA_MAX_NR_CHis not a good idea, as that may lead to hard-to-track problems.
Quoted 8 lines
Basically you have two options:
1. Just use the value of "dma-channels" if present.
This has the advantage that it will work automatically with future variants that have more channels, but allows the developer to trigger memory exhaustion by providing a very large value.
2. Return-EINVALif "dma-channels" is larger thanPDMA_MAX_NR_CH. This is the safest, but requires driver changes for a future variant that has more channels.
Many thanks for your tips on that. The second approach might be better to me, because it is safer and it seems to me that we don't currently have a plan to extend the channels in the pdma. I will modify it in the next version.
Quoted 11 lines
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds