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(-)
--
2.31.1
Add dma-channels property, then we can determine how many channels there
by device tree.
Signed-off-by: Zong Li <[email protected]>
---
arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi | 1 +
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
index c9f6d205d2ba..3c48f2d7a4a4 100644
--- a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
@@ -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>;
};
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index 0655b5c4201d..2bdfe7f06e4b 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -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 {
--
2.31.1
Add dma-channels property, then we can determine how many channels there
by device tree, rather than statically defines it in PDMA driver
Signed-off-by: Zong Li <[email protected]>
---
.../devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
index d32a71b975fe..3dbb8caefc17 100644
--- a/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
+++ b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
@@ -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>;
};
--
2.31.1
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]>
---
drivers/dma/sf-pdma/sf-pdma.c | 15 +++++++++------
drivers/dma/sf-pdma/sf-pdma.h | 8 ++------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
index f12606aeff87..5d386037bf36 100644
--- a/drivers/dma/sf-pdma/sf-pdma.c
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -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);
diff --git a/drivers/dma/sf-pdma/sf-pdma.h b/drivers/dma/sf-pdma/sf-pdma.h
index 0c20167b097d..8127d792f639 100644
--- a/drivers/dma/sf-pdma/sf-pdma.h
+++ b/drivers/dma/sf-pdma/sf-pdma.h
@@ -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 */
--
2.31.1
Hi Zong,
On Tue, Jan 11, 2022 at 9:51 AM Zong Li <[email protected]> wrote:
> 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!
> --- a/drivers/dma/sf-pdma/sf-pdma.c
> +++ b/drivers/dma/sf-pdma/sf-pdma.c
> @@ -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;
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.
> 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];
If dma-channels in DT > PDMA_NR_CH, this becomes an out-of-bound
access.
>
> devm_free_irq(&pdev->dev, ch->txirq, ch);
> diff --git a/drivers/dma/sf-pdma/sf-pdma.h b/drivers/dma/sf-pdma/sf-pdma.h
> index 0c20167b097d..8127d792f639 100644
> --- a/drivers/dma/sf-pdma/sf-pdma.h
> +++ b/drivers/dma/sf-pdma/sf-pdma.h
> @@ -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 */
-
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
On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Zong,
>
> On Tue, Jan 11, 2022 at 9:51 AM Zong Li <[email protected]> wrote:
> > 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!
>
> > --- a/drivers/dma/sf-pdma/sf-pdma.c
> > +++ b/drivers/dma/sf-pdma/sf-pdma.c
> > @@ -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;
>
> 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.
Thanks for your suggestions, let me fix it in the next version.
>
> > 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];
>
> 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.
> >
> > devm_free_irq(&pdev->dev, ch->txirq, ch);
> > diff --git a/drivers/dma/sf-pdma/sf-pdma.h b/drivers/dma/sf-pdma/sf-pdma.h
> > index 0c20167b097d..8127d792f639 100644
> > --- a/drivers/dma/sf-pdma/sf-pdma.h
> > +++ b/drivers/dma/sf-pdma/sf-pdma.h
> > @@ -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 */
> -
> 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
On Thu, Jan 13, 2022 at 2:53 PM Zong Li <[email protected]> wrote:
>
> On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <[email protected]> wrote:
> >
> > Hi Zong,
> >
> > On Tue, Jan 11, 2022 at 9:51 AM Zong Li <[email protected]> wrote:
> > > 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!
> >
> > > --- a/drivers/dma/sf-pdma/sf-pdma.c
> > > +++ b/drivers/dma/sf-pdma/sf-pdma.c
> > > @@ -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;
> >
> > 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.
>
> Thanks for your suggestions, let me fix it in the next version.
>
> >
> > > 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];
> >
> > 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.
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.
>
> > >
> > > devm_free_irq(&pdev->dev, ch->txirq, ch);
> > > diff --git a/drivers/dma/sf-pdma/sf-pdma.h b/drivers/dma/sf-pdma/sf-pdma.h
> > > index 0c20167b097d..8127d792f639 100644
> > > --- a/drivers/dma/sf-pdma/sf-pdma.h
> > > +++ b/drivers/dma/sf-pdma/sf-pdma.h
> > > @@ -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 */
> > -
> > 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
Hi Zong,
On Thu, Jan 13, 2022 at 8:26 AM Zong Li <[email protected]> wrote:
> On Thu, Jan 13, 2022 at 2:53 PM Zong Li <[email protected]> wrote:
> > On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <[email protected]> wrote:
> > > On Tue, Jan 11, 2022 at 9:51 AM Zong Li <[email protected]> wrote:
> > > > 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!
> > >
> > > > --- a/drivers/dma/sf-pdma/sf-pdma.c
> > > > +++ b/drivers/dma/sf-pdma/sf-pdma.c
> > > > @@ -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;
> > >
> > > 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.
> >
> > Thanks for your suggestions, let me fix it in the next version.
> >
> > >
> > > > 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];
> > >
> > > 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.
>
> 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.
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
On Thu, Jan 13, 2022 at 4:57 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Zong,
>
> On Thu, Jan 13, 2022 at 8:26 AM Zong Li <[email protected]> wrote:
> > On Thu, Jan 13, 2022 at 2:53 PM Zong Li <[email protected]> wrote:
> > > On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > On Tue, Jan 11, 2022 at 9:51 AM Zong Li <[email protected]> wrote:
> > > > > 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!
> > > >
> > > > > --- a/drivers/dma/sf-pdma/sf-pdma.c
> > > > > +++ b/drivers/dma/sf-pdma/sf-pdma.c
> > > > > @@ -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;
> > > >
> > > > 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.
> > >
> > > Thanks for your suggestions, let me fix it in the next version.
> > >
> > > >
> > > > > 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];
> > > >
> > > > 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.
> >
> > 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.
>
> 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.
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.
>
> 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