| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Mar 2022 13:04:27 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Serge Semin <Sergey.Semin@...kalelectronics.ru>
Cc: Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
Vinod Koul <vkoul@...nel.org>,
Jingoo Han <jingoohan1@...il.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Frank Li <Frank.Li@....com>,
Serge Semin <fancer.lancer@...il.com>,
Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Rob Herring <robh@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
linux-pci@...r.kernel.org, dmaengine@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/25] dmaengine: dw-edma: Join Write/Read channels into
a single device
On Thu, Mar 24, 2022 at 04:48:29AM +0300, Serge Semin wrote:
> Indeed there is no point in such split up because due to multiple reasons.
> First of all eDMA read and write channels belong to one physical
> controller. Splitting them up illogical. Secondly the channels
> differentiating can be done by means of the filtering and the
> dma_get_slave_caps() method. Finally having these channels handled
> separately not only needlessly complicates the code, but also causes the
> DebugFS error printed to console:
>
> >> Debugfs: Directory '1f052000.pcie' with parent 'dmaengine' already present!
>
> So to speak let's join the read/write channels into a single DMA device.
> The client drivers will be able to choose the channel with required
> capability by getting the DMA slave direction setting. It's default value
> is overridden by the dw_edma_device_caps() callback in accordance with the
> channel nature.
>
> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
I'm all in for this!
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Thanks,
Mani
> ---
> drivers/dma/dw-edma/dw-edma-core.c | 117 +++++++++++++++--------------
> drivers/dma/dw-edma/dw-edma-core.h | 5 +-
> 2 files changed, 62 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index cefa73412bf7..a391e44da039 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -209,6 +209,24 @@ static void dw_edma_start_transfer(struct dw_edma_chan *chan)
> desc->chunks_alloc--;
> }
>
> +static void dw_edma_device_caps(struct dma_chan *dchan,
> + struct dma_slave_caps *caps)
> +{
> + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> + if (chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
> + if (chan->dir == EDMA_DIR_READ)
> + caps->directions = BIT(DMA_DEV_TO_MEM);
> + else
> + caps->directions = BIT(DMA_MEM_TO_DEV);
> + } else {
> + if (chan->dir == EDMA_DIR_WRITE)
> + caps->directions = BIT(DMA_DEV_TO_MEM);
> + else
> + caps->directions = BIT(DMA_MEM_TO_DEV);
> + }
> +}
> +
> static int dw_edma_device_config(struct dma_chan *dchan,
> struct dma_slave_config *config)
> {
> @@ -723,8 +741,8 @@ static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> pm_runtime_put(chan->dw->chip->dev);
> }
>
> -static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write,
> - u32 wr_alloc, u32 rd_alloc)
> +static int dw_edma_channel_setup(struct dw_edma_chip *chip, u32 wr_alloc,
> + u32 rd_alloc)
> {
> struct dw_edma_region *dt_region;
> struct device *dev = chip->dev;
> @@ -732,27 +750,15 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write,
> struct dw_edma_chan *chan;
> struct dw_edma_irq *irq;
> struct dma_device *dma;
> - u32 alloc, off_alloc;
> - u32 i, j, cnt;
> - int err = 0;
> + u32 i, ch_cnt;
> u32 pos;
>
> - if (write) {
> - i = 0;
> - cnt = dw->wr_ch_cnt;
> - dma = &dw->wr_edma;
> - alloc = wr_alloc;
> - off_alloc = 0;
> - } else {
> - i = dw->wr_ch_cnt;
> - cnt = dw->rd_ch_cnt;
> - dma = &dw->rd_edma;
> - alloc = rd_alloc;
> - off_alloc = wr_alloc;
> - }
> + ch_cnt = dw->wr_ch_cnt + dw->rd_ch_cnt;
> + dma = &dw->dma;
>
> INIT_LIST_HEAD(&dma->channels);
> - for (j = 0; (alloc || chip->nr_irqs == 1) && j < cnt; j++, i++) {
> +
> + for (i = 0; i < ch_cnt; i++) {
> chan = &dw->chan[i];
>
> dt_region = devm_kzalloc(dev, sizeof(*dt_region), GFP_KERNEL);
> @@ -762,52 +768,62 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write,
> chan->vc.chan.private = dt_region;
>
> chan->dw = dw;
> - chan->id = j;
> - chan->dir = write ? EDMA_DIR_WRITE : EDMA_DIR_READ;
> +
> + if (i < dw->wr_ch_cnt) {
> + chan->id = i;
> + chan->dir = EDMA_DIR_WRITE;
> + } else {
> + chan->id = i - dw->wr_ch_cnt;
> + chan->dir = EDMA_DIR_READ;
> + }
> +
> chan->configured = false;
> chan->request = EDMA_REQ_NONE;
> chan->status = EDMA_ST_IDLE;
>
> - if (write)
> - chan->ll_max = (chip->ll_region_wr[j].sz / EDMA_LL_SZ);
> + if (chan->dir == EDMA_DIR_WRITE)
> + chan->ll_max = (chip->ll_region_wr[chan->id].sz / EDMA_LL_SZ);
> else
> - chan->ll_max = (chip->ll_region_rd[j].sz / EDMA_LL_SZ);
> + chan->ll_max = (chip->ll_region_rd[chan->id].sz / EDMA_LL_SZ);
> chan->ll_max -= 1;
>
> dev_vdbg(dev, "L. List:\tChannel %s[%u] max_cnt=%u\n",
> - write ? "write" : "read", j, chan->ll_max);
> + chan->dir == EDMA_DIR_WRITE ? "write" : "read",
> + chan->id, chan->ll_max);
>
> if (chip->nr_irqs == 1)
> pos = 0;
> + else if (chan->dir == EDMA_DIR_WRITE)
> + pos = chan->id % wr_alloc;
> else
> - pos = off_alloc + (j % alloc);
> + pos = wr_alloc + chan->id % rd_alloc;
>
> irq = &dw->irq[pos];
>
> - if (write)
> - irq->wr_mask |= BIT(j);
> + if (chan->dir == EDMA_DIR_WRITE)
> + irq->wr_mask |= BIT(chan->id);
> else
> - irq->rd_mask |= BIT(j);
> + irq->rd_mask |= BIT(chan->id);
>
> irq->dw = dw;
> memcpy(&chan->msi, &irq->msi, sizeof(chan->msi));
>
> dev_vdbg(dev, "MSI:\t\tChannel %s[%u] addr=0x%.8x%.8x, data=0x%.8x\n",
> - write ? "write" : "read", j,
> + chan->dir == EDMA_DIR_WRITE ? "write" : "read", chan->id,
> chan->msi.address_hi, chan->msi.address_lo,
> chan->msi.data);
>
> chan->vc.desc_free = vchan_free_desc;
> vchan_init(&chan->vc, dma);
>
> - if (write) {
> - dt_region->paddr = chip->dt_region_wr[j].paddr;
> - dt_region->vaddr = chip->dt_region_wr[j].vaddr;
> - dt_region->sz = chip->dt_region_wr[j].sz;
> + if (chan->dir == EDMA_DIR_WRITE) {
> + dt_region->paddr = chip->dt_region_wr[chan->id].paddr;
> + dt_region->vaddr = chip->dt_region_wr[chan->id].vaddr;
> + dt_region->sz = chip->dt_region_wr[chan->id].sz;
> } else {
> - dt_region->paddr = chip->dt_region_rd[j].paddr;
> - dt_region->vaddr = chip->dt_region_rd[j].vaddr;
> - dt_region->sz = chip->dt_region_rd[j].sz;
> + dt_region->paddr = chip->dt_region_rd[chan->id].paddr;
> + dt_region->vaddr = chip->dt_region_rd[chan->id].vaddr;
> + dt_region->sz = chip->dt_region_rd[chan->id].sz;
> }
>
> dw_edma_v0_core_device_config(chan);
> @@ -819,7 +835,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write,
> dma_cap_set(DMA_CYCLIC, dma->cap_mask);
> dma_cap_set(DMA_PRIVATE, dma->cap_mask);
> dma_cap_set(DMA_INTERLEAVE, dma->cap_mask);
> - dma->directions = BIT(write ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV);
> + dma->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> dma->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> dma->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> dma->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> @@ -828,6 +844,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write,
> dma->dev = chip->dev;
> dma->device_alloc_chan_resources = dw_edma_alloc_chan_resources;
> dma->device_free_chan_resources = dw_edma_free_chan_resources;
> + dma->device_caps = dw_edma_device_caps;
> dma->device_config = dw_edma_device_config;
> dma->device_pause = dw_edma_device_pause;
> dma->device_resume = dw_edma_device_resume;
> @@ -841,9 +858,7 @@ static int dw_edma_channel_setup(struct dw_edma_chip *chip, bool write,
> dma_set_max_seg_size(dma->dev, U32_MAX);
>
> /* Register DMA device */
> - err = dma_async_device_register(dma);
> -
> - return err;
> + return dma_async_device_register(dma);
> }
>
> static inline void dw_edma_dec_irq_alloc(int *nr_irqs, u32 *alloc, u16 cnt)
> @@ -982,13 +997,8 @@ int dw_edma_probe(struct dw_edma_chip *chip)
> if (err)
> return err;
>
> - /* Setup write channels */
> - err = dw_edma_channel_setup(chip, true, wr_alloc, rd_alloc);
> - if (err)
> - goto err_irq_free;
> -
> - /* Setup read channels */
> - err = dw_edma_channel_setup(chip, false, wr_alloc, rd_alloc);
> + /* Setup write/read channels */
> + err = dw_edma_channel_setup(chip, wr_alloc, rd_alloc);
> if (err)
> goto err_irq_free;
>
> @@ -1028,15 +1038,8 @@ int dw_edma_remove(struct dw_edma_chip *chip)
> pm_runtime_disable(dev);
>
> /* Deregister eDMA device */
> - dma_async_device_unregister(&dw->wr_edma);
> - list_for_each_entry_safe(chan, _chan, &dw->wr_edma.channels,
> - vc.chan.device_node) {
> - tasklet_kill(&chan->vc.task);
> - list_del(&chan->vc.chan.device_node);
> - }
> -
> - dma_async_device_unregister(&dw->rd_edma);
> - list_for_each_entry_safe(chan, _chan, &dw->rd_edma.channels,
> + dma_async_device_unregister(&dw->dma);
> + list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
> vc.chan.device_node) {
> tasklet_kill(&chan->vc.task);
> list_del(&chan->vc.chan.device_node);
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index e254c2fc3d9c..ec9f84a857d1 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
> @@ -98,10 +98,9 @@ struct dw_edma_irq {
> struct dw_edma {
> char name[20];
>
> - struct dma_device wr_edma;
> - u16 wr_ch_cnt;
> + struct dma_device dma;
>
> - struct dma_device rd_edma;
> + u16 wr_ch_cnt;
> u16 rd_ch_cnt;
>
> struct dw_edma_irq *irq;
> --
> 2.35.1
>
Powered by blists - more mailing lists