| Message ID | 78cfa00a02cbd10202040058af22a73caa9c5ae8.1641890718.git.zong.li@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Determine the number of DMA channels by 'dma-channels' property | expand |
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
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
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
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
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
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 */
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(-)