[PATCH v2 0/3] Determine the number of DMA channels by 'dma-channels' property

[PATCH v2 0/3] Determine the number of DMA channels by 'dma-channels' property

[Zong Li]

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

[PATCH v2 1/3] riscv: dts: Add dma-channels property in dma node

[Zong Li]

Add dma-channels property, then we can determine how many channels there
by device tree.

Signed-off-by: Zong Li <zong.li@sifive.com>
---
 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

[PATCH v2 2/3] dt-bindings: Add dma-channels for pdma device node

[Zong Li]

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 <zong.li@sifive.com>
---
 .../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

[PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree

[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 <zong.li@sifive.com>
---
 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

Re: [PATCH v2 3/3] dmaengine: sf-pdma: Get number of channel by device tree

[Geert Uytterhoeven]

Hi Zong,

On Tue, Jan 11, 2022 at 9:51 AM Zong Li <zong.li@sifive.com> 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 <zong.li@sifive.com>

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 -- geert@linux-m68k.org

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

[Zong Li]

On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Zong,
>
> On Tue, Jan 11, 2022 at 9:51 AM Zong Li <zong.li@sifive.com> 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 <zong.li@sifive.com>
>
> 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 -- geert@linux-m68k.org
>
> 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

[Zong Li]

On Thu, Jan 13, 2022 at 2:53 PM Zong Li <zong.li@sifive.com> wrote:
>
> On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Zong,
> >
> > On Tue, Jan 11, 2022 at 9:51 AM Zong Li <zong.li@sifive.com> 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 <zong.li@sifive.com>
> >
> > 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 -- geert@linux-m68k.org
> >
> > 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

[Geert Uytterhoeven]

Hi Zong,

On Thu, Jan 13, 2022 at 8:26 AM Zong Li <zong.li@sifive.com> wrote:
> On Thu, Jan 13, 2022 at 2:53 PM Zong Li <zong.li@sifive.com> wrote:
> > On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, Jan 11, 2022 at 9:51 AM Zong Li <zong.li@sifive.com> 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 <zong.li@sifive.com>
> > >
> > > 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 -- geert@linux-m68k.org

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

[Zong Li]

On Thu, Jan 13, 2022 at 4:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Zong,
>
> On Thu, Jan 13, 2022 at 8:26 AM Zong Li <zong.li@sifive.com> wrote:
> > On Thu, Jan 13, 2022 at 2:53 PM Zong Li <zong.li@sifive.com> wrote:
> > > On Wed, Jan 12, 2022 at 4:28 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, Jan 11, 2022 at 9:51 AM Zong Li <zong.li@sifive.com> 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 <zong.li@sifive.com>
> > > >
> > > > 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 -- geert@linux-m68k.org
>
> 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