| 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: Mon, 14 Feb 2022 16:10:09 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Cc: Kishon Vijay Abraham I <kishon@...com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Krzysztof Wilczyński <kw@...ux.com>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Marek Vasut <marek.vasut+renesas@...il.com>,
Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
Rob Herring <robh@...nel.org>, linux-pci@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
Prabhakar <prabhakar.csengg@...il.com>,
Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [RFC PATCH 5/5] PCI: rcar-ep: Add support for DMAC
On Wed, Jan 26, 2022 at 07:50:43PM +0000, Lad Prabhakar wrote:
> R-Car PCIe controller has an internal DMAC to support data transfer
> between Internal Bus -> PCI Express and vice versa.
>
> This patch fills in the required flags and ops for the PCIe EP to
> support DMAC transfer.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> ---
> drivers/pci/controller/pcie-rcar-ep.c | 227 ++++++++++++++++++++++++++
> drivers/pci/controller/pcie-rcar.h | 23 +++
> 2 files changed, 250 insertions(+)
>
> diff --git a/drivers/pci/controller/pcie-rcar-ep.c b/drivers/pci/controller/pcie-rcar-ep.c
> index f9682df1da61..c49b25069328 100644
> --- a/drivers/pci/controller/pcie-rcar-ep.c
> +++ b/drivers/pci/controller/pcie-rcar-ep.c
> @@ -18,6 +18,21 @@
>
> #define RCAR_EPC_MAX_FUNCTIONS 1
>
> +#define RCAR_PCIE_MAX_DMAC_BYTE_COUNT 0x7FFFFFFU
> +#define RCAR_PCIE_DMAC_BYTE_COUNT_MULTIPLE 8
> +#define RCAR_PCIE_DMAC_TIMEOUT (msecs_to_jiffies(3 * 1000))
> +#define RCAR_PCIE_DMAC_DEFAULT_CHANNEL 0
> +
> +enum rcar_pcie_ep_dmac_xfr_status {
> + RCAR_PCIE_DMA_XFR_SUCCESS,
> + RCAR_PCIE_DMA_XFR_ERROR,
> +};
> +
> +struct rcar_pcie_ep_dmac_info {
> + enum rcar_pcie_ep_dmac_xfr_status status;
> + size_t bytes;
> +};
> +
> /* Structure representing the PCIe interface */
> struct rcar_pcie_endpoint {
> struct rcar_pcie pcie;
> @@ -28,8 +43,114 @@ struct rcar_pcie_endpoint {
> unsigned long *ib_window_map;
> u32 num_ib_windows;
> u32 num_ob_windows;
> + struct completion irq_raised;
> + struct mutex dma_operation;
> + spinlock_t lock;
Locks should be documented.
> + struct rcar_pcie_ep_dmac_info xfr;
> };
>
> +static inline bool rcar_pcie_ep_is_dmac_active(struct rcar_pcie_endpoint *ep)
No need of "inline", compiler should be able to judge it.
> +{
> + if (rcar_pci_read_reg(&ep->pcie, PCIEDMAOR) & PCIEDMAOR_DMAACT)
> + return true;
> +
> + return false;
> +}
> +
> +static void
> +rcar_pcie_ep_setup_dmac_request(struct rcar_pcie_endpoint *ep,
> + dma_addr_t dma_dst, dma_addr_t dma_src,
> + size_t len, enum pci_epf_xfr_direction dir, u8 ch)
> +{
> + struct rcar_pcie *pcie = &ep->pcie;
> + u32 val;
> +
> + ep->xfr.status = RCAR_PCIE_DMA_XFR_ERROR;
> + ep->xfr.bytes = RCAR_PCIE_MAX_DMAC_BYTE_COUNT;
> +
> + /* swap values if xfr is from pcie to internal */
> + if (dir == PCIE_TO_INTERNAL)
> + swap(dma_dst, dma_src);
> +
> + /* Configure the PCI Express lower */
> + rcar_pci_write_reg(pcie, lower_32_bits(dma_dst), PCIEDMPALR(ch));
> +
> + /* Configure the PCI Express upper */
> + rcar_pci_write_reg(pcie, upper_32_bits(dma_dst), PCIEDMPAUR(ch));
> +
> + /* Configure the internal bus address */
> + rcar_pci_write_reg(pcie, lower_32_bits(dma_src), PCIEDMIAR(ch));
> +
> + /* Configure the byte count values */
> + rcar_pci_write_reg(pcie, len, PCIEDMBCNTR(ch));
> +
> + /* Enable interrupts */
> + val = rcar_pci_read_reg(pcie, PCIEDMCHSR(ch));
> +
> + /* set enable flags */
> + val |= PCIEDMCHSR_IE;
> + val |= PCIEDMCHSR_IBEE;
> + val |= PCIEDMCHSR_PEEE;
> + val |= PCIEDMCHSR_CHTCE;
> +
> + /* Clear error flags */
> + val &= ~PCIEDMCHSR_TE;
> + val &= ~PCIEDMCHSR_PEE;
> + val &= ~PCIEDMCHSR_IBE;
> + val &= ~PCIEDMCHSR_CHTC;
> +
Can you use bitfield operations everywhere?
> + rcar_pci_write_reg(pcie, val, PCIEDMCHSR(ch));
> +
> + wmb(); /* flush the settings */
Since the write is at the end of the function, no need to put a barrier here.
> +}
> +
> +static void rcar_pcie_ep_execute_dmac_request(struct rcar_pcie_endpoint *ep,
> + enum pci_epf_xfr_direction dir, u8 ch)
> +{
> + struct rcar_pcie *pcie = &ep->pcie;
> + u32 val;
> +
> + /* Enable DMA */
> + val = rcar_pci_read_reg(pcie, PCIEDMAOR);
> + val |= PCIEDMAOR_DMAE;
> + rcar_pci_write_reg(pcie, val, PCIEDMAOR);
> +
> + /* Configure the DMA direction */
> + val = rcar_pci_read_reg(pcie, PCIEDMCHCR(ch));
> + if (dir == INTERNAL_TO_PCIE)
> + val |= PCIEDMCHCR_DIR;
> + else
> + val &= ~PCIEDMCHCR_DIR;
> +
> + val |= PCIEDMCHCR_CHE;
> + rcar_pci_write_reg(pcie, val, PCIEDMCHCR(ch));
> +
> + wmb(); /* flush the settings */
Same here.
> +}
> +
> +static enum rcar_pcie_ep_dmac_xfr_status
> +rcar_pcie_ep_get_dmac_status(struct rcar_pcie_endpoint *ep,
> + size_t *count, u8 ch)
ch is unused.
> +{
> + *count = ep->xfr.bytes;
> + return ep->xfr.status;
> +}
> +
> +static void rcar_pcie_ep_stop_dmac_request(struct rcar_pcie_endpoint *ep, u8 ch)
> +{
> + struct rcar_pcie *pcie = &ep->pcie;
> + u32 val;
> +
> + val = rcar_pci_read_reg(pcie, PCIEDMCHCR(ch));
> + val &= ~PCIEDMCHCR_CHE;
> + rcar_pci_write_reg(pcie, val, PCIEDMCHCR(ch));
> +
> + /* Disable interrupt */
> + val = rcar_pci_read_reg(pcie, PCIEDMAOR);
> + val &= ~PCIEDMAOR_DMAE;
> + rcar_pci_write_reg(pcie, val, PCIEDMAOR);
> +}
> +
> static void rcar_pcie_ep_hw_init(struct rcar_pcie *pcie)
> {
> u32 val;
> @@ -419,6 +540,44 @@ static int rcar_pcie_ep_raise_irq(struct pci_epc *epc, u8 fn, u8 vfn,
> }
> }
>
> +static int rcar_pcie_ep_data_transfer(struct pci_epc *epc, struct pci_epf *epf,
> + dma_addr_t dma_dst, dma_addr_t dma_src,
> + size_t len, enum pci_epf_xfr_direction dir)
> +{
> + struct rcar_pcie_endpoint *ep = epc_get_drvdata(epc);
> + u8 ch = RCAR_PCIE_DMAC_DEFAULT_CHANNEL;
> + enum rcar_pcie_ep_dmac_xfr_status stat;
> + int ret = -EINVAL;
> + long wait_status;
> + size_t count;
> +
> + if (len > RCAR_PCIE_MAX_DMAC_BYTE_COUNT ||
> + (len % RCAR_PCIE_DMAC_BYTE_COUNT_MULTIPLE) != 0)
> + return -EINVAL;
A comment here would be informative.
> +
> + if (mutex_is_locked(&ep->dma_operation) || rcar_pcie_ep_is_dmac_active(ep))
Why the mutex should be checked here? Why can't this thread sleep below if mutex
is not available?
> + return -EBUSY;
> +
> + mutex_lock(&ep->dma_operation);
> +
> + rcar_pcie_ep_setup_dmac_request(ep, dma_dst, dma_src, len, dir, ch);
> +
> + rcar_pcie_ep_execute_dmac_request(ep, dir, ch);
> +
> + wait_status = wait_for_completion_interruptible_timeout(&ep->irq_raised,
> + RCAR_PCIE_DMAC_TIMEOUT);
> + if (wait_status <= 0) {
> + rcar_pcie_ep_stop_dmac_request(ep, ch);
ret = -ETIMEDOUT;?
> + } else {
> + stat = rcar_pcie_ep_get_dmac_status(ep, &count, ch);
> + if (stat == RCAR_PCIE_DMA_XFR_SUCCESS && !count)
> + ret = 0;
> + }
> +
> + mutex_unlock(&ep->dma_operation);
> + return ret;
> +}
> +
> static int rcar_pcie_ep_start(struct pci_epc *epc)
> {
> struct rcar_pcie_endpoint *ep = epc_get_drvdata(epc);
> @@ -429,6 +588,55 @@ static int rcar_pcie_ep_start(struct pci_epc *epc)
> return 0;
> }
>
> +static irqreturn_t rcar_pcie_ep_dmac_irq_handler(int irq, void *arg)
> +{
> + u8 ch = RCAR_PCIE_DMAC_DEFAULT_CHANNEL;
> + struct rcar_pcie_endpoint *ep = arg;
> + struct rcar_pcie *pcie = &ep->pcie;
> + unsigned long flags;
> + u32 chsr_val;
> + u32 chcr_val;
> + u32 bytes;
> +
> + spin_lock_irqsave(&ep->lock, flags);
Since the lock is taken in isr, there is no need to do irqsave/restore.
Just spin_lock/unlock is enough.
Also, this lock is only used in isr. I'm wondering why the lock is needed.
> +
> + chsr_val = rcar_pci_read_reg(pcie, PCIEDMCHSR(ch));
> +
> + chcr_val = rcar_pci_read_reg(pcie, PCIEDMCHCR(ch));
> +
> + if (mutex_is_locked(&ep->dma_operation)) {
Why should the mutex be checked? Are you expecting the irq after transfer
timeout?
> + if ((chsr_val & PCIEDMCHSR_PEE) ||
> + (chsr_val & PCIEDMCHSR_IBE) ||
> + (chsr_val & PCIEDMCHSR_CHTC))
> + ep->xfr.status = RCAR_PCIE_DMA_XFR_ERROR;
> + else if (chsr_val & PCIEDMCHSR_TE)
> + ep->xfr.status = RCAR_PCIE_DMA_XFR_SUCCESS;
> +
> + /* get byte count */
> + bytes = rcar_pci_read_reg(pcie, PCIEDMBCNTR(ch));
> + ep->xfr.bytes = bytes;
> +
> + if ((chsr_val & PCIEDMCHSR_PEE) || (chsr_val & PCIEDMCHSR_IBE) ||
> + (chsr_val & PCIEDMCHSR_TE) || (chsr_val & PCIEDMCHSR_CHTC)) {
> + complete(&ep->irq_raised);
> + }
> + } else {
> + spin_unlock_irqrestore(&ep->lock, flags);
> + return IRQ_NONE;
> + }
> +
> + if (chcr_val & PCIEDMCHCR_CHE)
> + chcr_val &= ~PCIEDMCHCR_CHE;
> + rcar_pci_write_reg(pcie, chcr_val, PCIEDMCHCR(ch));
> +
> + /* Clear DMA interrupt source */
> + rcar_pci_write_reg(pcie, chsr_val, PCIEDMCHSR(ch));
> +
> + spin_unlock_irqrestore(&ep->lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> static void rcar_pcie_ep_stop(struct pci_epc *epc)
> {
> struct rcar_pcie_endpoint *ep = epc_get_drvdata(epc);
> @@ -446,6 +654,8 @@ static const struct pci_epc_features rcar_pcie_epc_features = {
> .bar_fixed_size[0] = 128,
> .bar_fixed_size[2] = 256,
> .bar_fixed_size[4] = 256,
> + .internal_dmac = true,
> + .internal_dmac_mask = DMA_BIT_MASK(32),
> };
>
> static const struct pci_epc_features*
> @@ -466,6 +676,7 @@ static const struct pci_epc_ops rcar_pcie_epc_ops = {
> .start = rcar_pcie_ep_start,
> .stop = rcar_pcie_ep_stop,
> .get_features = rcar_pcie_ep_get_features,
> + .dmac_transfer = rcar_pcie_ep_data_transfer,
> };
>
> static const struct of_device_id rcar_pcie_ep_of_match[] = {
> @@ -480,6 +691,7 @@ static int rcar_pcie_ep_probe(struct platform_device *pdev)
> struct rcar_pcie_endpoint *ep;
> struct rcar_pcie *pcie;
> struct pci_epc *epc;
> + int dmac_irq;
> int err;
>
> ep = devm_kzalloc(dev, sizeof(*ep), GFP_KERNEL);
> @@ -502,6 +714,14 @@ static int rcar_pcie_ep_probe(struct platform_device *pdev)
> goto err_pm_put;
> }
>
> + dmac_irq = platform_get_irq(pdev, 1);
> + if (dmac_irq < 0)
"err = dmac_irq" or just use "err".
Thanks,
Mani
> + goto err_pm_put;
> +
> + init_completion(&ep->irq_raised);
> + mutex_init(&ep->dma_operation);
> + spin_lock_init(&ep->lock);
> +
> ep->num_ib_windows = MAX_NR_INBOUND_MAPS;
> ep->ib_window_map =
> devm_kcalloc(dev, BITS_TO_LONGS(ep->num_ib_windows),
> @@ -533,6 +753,13 @@ static int rcar_pcie_ep_probe(struct platform_device *pdev)
>
> rcar_pcie_ep_hw_init(pcie);
>
> + err = devm_request_irq(dev, dmac_irq, rcar_pcie_ep_dmac_irq_handler,
> + 0, "pcie-rcar-ep-dmac", pcie);
> + if (err) {
> + dev_err(dev, "failed to request dmac irq\n");
> + goto err_pm_put;
> + }
> +
> err = pci_epc_multi_mem_init(epc, ep->ob_window, ep->num_ob_windows);
> if (err < 0) {
> dev_err(dev, "failed to initialize the epc memory space\n");
> diff --git a/drivers/pci/controller/pcie-rcar.h b/drivers/pci/controller/pcie-rcar.h
> index 9bb125db85c6..874f8a384e6d 100644
> --- a/drivers/pci/controller/pcie-rcar.h
> +++ b/drivers/pci/controller/pcie-rcar.h
> @@ -54,6 +54,29 @@
> #define PAR_ENABLE BIT(31)
> #define IO_SPACE BIT(8)
>
> +/* PCIe DMAC control reg & mask */
> +#define PCIEDMAOR 0x04000
> +#define PCIEDMAOR_DMAE BIT(31)
> +#define PCIEDMAOR_DMAACT BIT(16)
> +#define PCIEDMPALR(x) (0x04100 + ((x) * 0x40))
> +#define PCIEDMPAUR(x) (0x04104 + ((x) * 0x40))
> +#define PCIEDMIAR(x) (0x04108 + ((x) * 0x40))
> +#define PCIEDMBCNTR(x) (0x04110 + ((x) * 0x40))
> +#define PCIEDMCCAR(x) (0x04120 + ((x) * 0x40))
> +#define PCIEDMCHCR(x) (0x04128 + ((x) * 0x40))
> +#define PCIEDMCHCR_CHE BIT(31)
> +#define PCIEDMCHCR_DIR BIT(30)
> +#define PCIEDMCHSR(x) (0x0412c + ((x) * 0x40))
> +#define PCIEDMCHSR_CHTCE BIT(28)
> +#define PCIEDMCHSR_PEEE BIT(27)
> +#define PCIEDMCHSR_IBEE BIT(25)
> +#define PCIEDMCHSR_CHTC BIT(12)
> +#define PCIEDMCHSR_PEE BIT(11)
> +#define PCIEDMCHSR_IBE BIT(9)
> +#define PCIEDMCHSR_IE BIT(3)
> +#define PCIEDMCHSR_TE BIT(0)
> +#define PCIEDMCHC2R(x) (0x04130 + ((x) * 0x40))
> +
> /* Configuration */
> #define PCICONF(x) (0x010000 + ((x) * 0x4))
> #define INTDIS BIT(10)
> --
> 2.25.1
>
Powered by blists - more mailing lists