lkml.org 
[lkml]   [2019]   [Jun]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [EXT] Re: [V3 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA controller driver for Layerscape SoCs
    Date
    Hi, Vinod,

    Please see my comments inline, thanks very much.

    Best Regards,
    Peng

    >-----Original Message-----
    >From: Vinod Koul <vkoul@kernel.org>
    >Sent: 2019年4月29日 13:32
    >To: Peng Ma <peng.ma@nxp.com>
    >Cc: dan.j.williams@intel.com; Leo Li <leoyang.li@nxp.com>;
    >linux-kernel@vger.kernel.org; dmaengine@vger.kernel.org
    >Subject: [EXT] Re: [V3 2/2] dmaengine: fsl-dpaa2-qdma: Add NXP dpaa2 qDMA
    >controller driver for Layerscape SoCs
    >
    >Caution: EXT Email
    >
    >On 09-04-19, 15:22, Peng Ma wrote:
    >> DPPA2(Data Path Acceleration Architecture 2) qDMA The qDMA supports
    >> channel virtualization by allowing DMA jobs to be enqueued into
    >> different frame queues. Core can initiate a DMA transaction by
    >> preparing a frame descriptor(FD) for each DMA job and enqueuing this job to
    >a frame queue.
    >> through a hardware portal. The qDMA prefetches DMA jobs from the frame
    >queues.
    >> It then schedules and dispatches to internal DMA hardware engines,
    >> which generate read and write requests. Both qDMA source data and
    >> destination data can be either contiguous or non-contiguous using one or
    >more scatter/gather tables.
    >> The qDMA supports global bandwidth flow control where all DMA
    >> transactions are stalled if the bandwidth threshold has been reached.
    >> Also supported are transaction based read throttling.
    >>
    >> Add NXP dppa2 qDMA to support some of Layerscape SoCs.
    >> such as: LS1088A, LS208xA, LX2, etc.
    >>
    >> Signed-off-by: Peng Ma <peng.ma@nxp.com>
    >> ---
    >> changed for v3:
    >> - Add depends on arm64 for dpaa2 qdma driver
    >> - The dpaa2_io_service_[de]register functions have a new
    >parameter
    >> So update all calls to some functions
    >>
    >> drivers/dma/Kconfig | 2 +
    >> drivers/dma/Makefile | 1 +
    >> drivers/dma/fsl-dpaa2-qdma/Kconfig | 9 +
    >> drivers/dma/fsl-dpaa2-qdma/Makefile | 3 +
    >> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c | 782
    >> +++++++++++++++++++++++++++++++
    >> drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.h | 152 ++++++
    >> 6 files changed, 949 insertions(+), 0 deletions(-) create mode
    >> 100644 drivers/dma/fsl-dpaa2-qdma/Kconfig
    >> create mode 100644 drivers/dma/fsl-dpaa2-qdma/Makefile
    >> create mode 100644 drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
    >> create mode 100644 drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.h
    >>
    >> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index
    >> eaf78f4..08aae01 100644
    >> --- a/drivers/dma/Kconfig
    >> +++ b/drivers/dma/Kconfig
    >> @@ -671,6 +671,8 @@ source "drivers/dma/sh/Kconfig"
    >>
    >> source "drivers/dma/ti/Kconfig"
    >>
    >> +source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
    >> +
    >> # clients
    >> comment "DMA Clients"
    >> depends on DMA_ENGINE
    >> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile index
    >> 6126e1c..2499ed8 100644
    >> --- a/drivers/dma/Makefile
    >> +++ b/drivers/dma/Makefile
    >> @@ -75,6 +75,7 @@ obj-$(CONFIG_UNIPHIER_MDMAC) +=
    >uniphier-mdmac.o
    >> obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
    >> obj-$(CONFIG_ZX_DMA) += zx_dma.o
    >> obj-$(CONFIG_ST_FDMA) += st_fdma.o
    >> +obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
    >>
    >> obj-y += mediatek/
    >> obj-y += qcom/
    >> diff --git a/drivers/dma/fsl-dpaa2-qdma/Kconfig
    >> b/drivers/dma/fsl-dpaa2-qdma/Kconfig
    >> new file mode 100644
    >> index 0000000..258ed6b
    >> --- /dev/null
    >> +++ b/drivers/dma/fsl-dpaa2-qdma/Kconfig
    >> @@ -0,0 +1,9 @@
    >> +menuconfig FSL_DPAA2_QDMA
    >> + tristate "NXP DPAA2 QDMA"
    >> + depends on ARM64
    >> + depends on FSL_MC_BUS && FSL_MC_DPIO
    >> + select DMA_ENGINE
    >> + select DMA_VIRTUAL_CHANNELS
    >> + help
    >> + NXP Data Path Acceleration Architecture 2 QDMA driver,
    >> + using the NXP MC bus driver.
    >> diff --git a/drivers/dma/fsl-dpaa2-qdma/Makefile
    >> b/drivers/dma/fsl-dpaa2-qdma/Makefile
    >> new file mode 100644
    >> index 0000000..c1d0226
    >> --- /dev/null
    >> +++ b/drivers/dma/fsl-dpaa2-qdma/Makefile
    >> @@ -0,0 +1,3 @@
    >> +# SPDX-License-Identifier: GPL-2.0
    >> +# Makefile for the NXP DPAA2 qDMA controllers
    >> +obj-$(CONFIG_FSL_DPAA2_QDMA) += dpaa2-qdma.o dpdmai.o
    >> diff --git a/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
    >> b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
    >> new file mode 100644
    >> index 0000000..0cdde0f
    >> --- /dev/null
    >> +++ b/drivers/dma/fsl-dpaa2-qdma/dpaa2-qdma.c
    >> @@ -0,0 +1,782 @@
    >> +// SPDX-License-Identifier: GPL-2.0
    >> +// Copyright 2014-2018 NXP
    >> +
    >> +/*
    >> + * Author: Changming Huang <jerry.huang@nxp.com>
    >> + *
    >> + * Driver for the NXP QDMA engine with QMan mode.
    >> + * Channel virtualization is supported through enqueuing of DMA jobs
    >> +to,
    >> + * or dequeuing DMA jobs from different work queues with QMan portal.
    >> + * This module can be found on NXP LS2 SoCs.
    >> + *
    >> + */
    >> +
    >> +#include <linux/init.h>
    >> +#include <linux/module.h>
    >> +#include <linux/dmapool.h>
    >> +#include <linux/of_irq.h>
    >> +#include <linux/iommu.h>
    >> +#include <linux/sys_soc.h>
    >> +#include <linux/fsl/mc.h>
    >> +#include <soc/fsl/dpaa2-io.h>
    >> +
    >> +#include "../virt-dma.h"
    >> +#include "dpdmai_cmd.h"
    >> +#include "dpdmai.h"
    >> +#include "dpaa2-qdma.h"
    >> +
    >> +static bool smmu_disable = true;
    >> +
    >> +static struct dpaa2_qdma_chan *to_dpaa2_qdma_chan(struct dma_chan
    >> +*chan) {
    >> + return container_of(chan, struct dpaa2_qdma_chan, vchan.chan); }
    >> +
    >> +static struct dpaa2_qdma_comp *to_fsl_qdma_comp(struct virt_dma_desc
    >> +*vd) {
    >> + return container_of(vd, struct dpaa2_qdma_comp, vdesc); }
    >> +
    >> +static int dpaa2_qdma_alloc_chan_resources(struct dma_chan *chan) {
    >> + struct dpaa2_qdma_chan *dpaa2_chan =
    >to_dpaa2_qdma_chan(chan);
    >> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
    >> + struct device *dev = &dpaa2_qdma->priv->dpdmai_dev->dev;
    >> +
    >> + dpaa2_chan->fd_pool = dma_pool_create("fd_pool", dev,
    >> + FD_POOL_SIZE, 32,
    >0);
    >> + if (!dpaa2_chan->fd_pool)
    >> + return -ENOMEM;
    >> +
    >> + return dpaa2_qdma->desc_allocated++; }
    >> +
    >> +static void dpaa2_qdma_free_chan_resources(struct dma_chan *chan) {
    >> + struct dpaa2_qdma_chan *dpaa2_chan =
    >to_dpaa2_qdma_chan(chan);
    >> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
    >> + unsigned long flags;
    >> +
    >> + LIST_HEAD(head);
    >> +
    >> + spin_lock_irqsave(&dpaa2_chan->vchan.lock, flags);
    >> + vchan_get_all_descriptors(&dpaa2_chan->vchan, &head);
    >> + spin_unlock_irqrestore(&dpaa2_chan->vchan.lock, flags);
    >> +
    >> + vchan_dma_desc_free_list(&dpaa2_chan->vchan, &head);
    >> +
    >> + dpaa2_dpdmai_free_comp(dpaa2_chan,
    >&dpaa2_chan->comp_used);
    >> + dpaa2_dpdmai_free_comp(dpaa2_chan, &dpaa2_chan->comp_free);
    >> +
    >> + dma_pool_destroy(dpaa2_chan->fd_pool);
    >> + dpaa2_qdma->desc_allocated--;
    >> +}
    >> +
    >> +/*
    >> + * Request a command descriptor for enqueue.
    >> + */
    >> +static struct dpaa2_qdma_comp *
    >> +dpaa2_qdma_request_desc(struct dpaa2_qdma_chan *dpaa2_chan) {
    >> + struct dpaa2_qdma_comp *comp_temp = NULL;
    >> + unsigned long flags;
    >> +
    >> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
    >> + if (list_empty(&dpaa2_chan->comp_free)) {
    >> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
    >> + comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL);
    >
    >GFP_NOWAIT?
    [Peng Ma] Got it.
    >
    >> + if (!comp_temp)
    >> + goto err;
    >> + comp_temp->fd_virt_addr =
    >> + dma_pool_alloc(dpaa2_chan->fd_pool,
    >GFP_NOWAIT,
    >> + &comp_temp->fd_bus_addr);
    >> + if (!comp_temp->fd_virt_addr)
    >
    >err handling seems incorrect, you dont clean up, caller doesnt check return!
    >
    [Peng Ma] Yes, It's my fault.
    >> + goto err;
    >> +
    >> + comp_temp->fl_virt_addr =
    >> + (void *)((struct dpaa2_fd *)
    >> + comp_temp->fd_virt_addr + 1);
    >
    >casts and pointer math, what could go wrong!! This doesnt smell right!
    >
    >> + comp_temp->fl_bus_addr = comp_temp->fd_bus_addr +
    >> + sizeof(struct dpaa2_fd);
    >
    >why not use fl_virt_addr and get the bus_address?
    What you mean is I should use virt_to_phys to get the bus_address?
    >
    >> + comp_temp->desc_virt_addr =
    >> + (void *)((struct dpaa2_fl_entry *)
    >> + comp_temp->fl_virt_addr + 3);
    >> + comp_temp->desc_bus_addr = comp_temp->fl_bus_addr +
    >> + sizeof(struct dpaa2_fl_entry) * 3;
    >
    >pointer math in the two calls doesnt match and as I said doesnt look good...
    Should I do this as follows:
    - comp_temp->fl_virt_addr =
    - (void *)((struct dpaa2_fd *)
    - comp_temp->fd_virt_addr + 1);
    - comp_temp->fl_bus_addr = comp_temp->fd_bus_addr +
    - sizeof(struct dpaa2_fd);
    - comp_temp->desc_virt_addr =
    - (void *)((struct dpaa2_fl_entry *)
    - comp_temp->fl_virt_addr + 3);
    - comp_temp->desc_bus_addr = comp_temp->fl_bus_addr +
    - sizeof(struct dpaa2_fl_entry) * 3;
    + comp_temp->fd_virt_addr = (struct dpaa2_fd *)virt_addr_head++;
    + comp_temp->fl_virt_addr = (struct dpaa2_fl_entry *)virt_addr_head;
    + comp_temp->fl_bus_addr = virt_to_phys(comp_temp->fl_virt_addr);
    + virt_addr_head = (struct dpaa2_fl_entry *)virt_addr_head + 3;
    + comp_temp->desc_virt_addr = (struct dpaa2_qdma_sd_d *)virt_addr_head;
    + comp_temp->desc_bus_addr = virt_to_phys(comp_temp->desc_virt_addr);
    >
    >> +
    >> + comp_temp->qchan = dpaa2_chan;
    >> + return comp_temp;
    >> + }
    >> + comp_temp = list_first_entry(&dpaa2_chan->comp_free,
    >> + struct dpaa2_qdma_comp, list);
    >> + list_del(&comp_temp->list);
    >> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags);
    >> +
    >> + comp_temp->qchan = dpaa2_chan;
    >> +err:
    >> + return comp_temp;
    >> +}
    >> +
    >> +static void
    >> +dpaa2_qdma_populate_fd(u32 format, struct dpaa2_qdma_comp
    >> +*dpaa2_comp) {
    >> + struct dpaa2_fd *fd;
    >> +
    >> + fd = (struct dpaa2_fd *)dpaa2_comp->fd_virt_addr;
    >
    >whats with the casts! you seem to like them! You are casting away from void!
    This will avoid after change fd_virt_addr type.
    >
    >> + memset(fd, 0, sizeof(struct dpaa2_fd));
    >> +
    >> + /* fd populated */
    >> + dpaa2_fd_set_addr(fd, dpaa2_comp->fl_bus_addr);
    >> + /* Bypass memory translation, Frame list format, short length disable
    >*/
    >> + /* we need to disable BMT if fsl-mc use iova addr */
    >> + if (smmu_disable)
    >> + dpaa2_fd_set_bpid(fd, QMAN_FD_BMT_ENABLE);
    >> + dpaa2_fd_set_format(fd, QMAN_FD_FMT_ENABLE |
    >> + QMAN_FD_SL_DISABLE);
    >> +
    >> + dpaa2_fd_set_frc(fd, format | QDMA_SER_CTX); }
    >> +
    >> +/* first frame list for descriptor buffer */ static void
    >> +dpaa2_qdma_populate_first_framel(struct dpaa2_fl_entry *f_list,
    >> + struct dpaa2_qdma_comp
    >*dpaa2_comp,
    >> + bool wrt_changed) {
    >> + struct dpaa2_qdma_sd_d *sdd;
    >> +
    >> + sdd = (struct dpaa2_qdma_sd_d *)dpaa2_comp->desc_virt_addr;
    >
    >again
    Same to before.
    >
    >> + memset(sdd, 0, 2 * (sizeof(*sdd)));
    >> +
    >> + /* source descriptor CMD */
    >> + sdd->cmd = cpu_to_le32(QDMA_SD_CMD_RDTTYPE_COHERENT);
    >> + sdd++;
    >> +
    >> + /* dest descriptor CMD */
    >> + if (wrt_changed)
    >> + sdd->cmd =
    >cpu_to_le32(LX2160_QDMA_DD_CMD_WRTTYPE_COHERENT);
    >> + else
    >> + sdd->cmd =
    >cpu_to_le32(QDMA_DD_CMD_WRTTYPE_COHERENT);
    >> +
    >> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
    >> +
    >> + /* first frame list to source descriptor */
    >> + dpaa2_fl_set_addr(f_list, dpaa2_comp->desc_bus_addr);
    >> + dpaa2_fl_set_len(f_list, 0x20);
    >> + dpaa2_fl_set_format(f_list, QDMA_FL_FMT_SBF |
    >QDMA_FL_SL_LONG);
    >> +
    >> + /* bypass memory translation */
    >> + if (smmu_disable)
    >> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE); }
    >> +
    >> +/* source and destination frame list */ static void
    >> +dpaa2_qdma_populate_frames(struct dpaa2_fl_entry *f_list,
    >> + dma_addr_t dst, dma_addr_t src,
    >> + size_t len, uint8_t fmt) {
    >> + /* source frame list to source buffer */
    >> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
    >> +
    >> + dpaa2_fl_set_addr(f_list, src);
    >> + dpaa2_fl_set_len(f_list, len);
    >> +
    >> + /* single buffer frame or scatter gather frame */
    >> + dpaa2_fl_set_format(f_list, (fmt | QDMA_FL_SL_LONG));
    >> +
    >> + /* bypass memory translation */
    >> + if (smmu_disable)
    >> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE);
    >> +
    >> + f_list++;
    >> +
    >> + /* destination frame list to destination buffer */
    >> + memset(f_list, 0, sizeof(struct dpaa2_fl_entry));
    >> +
    >> + dpaa2_fl_set_addr(f_list, dst);
    >> + dpaa2_fl_set_len(f_list, len);
    >> + dpaa2_fl_set_format(f_list, (fmt | QDMA_FL_SL_LONG));
    >> + /* single buffer frame or scatter gather frame */
    >> + dpaa2_fl_set_final(f_list, QDMA_FL_F);
    >> + /* bypass memory translation */
    >> + if (smmu_disable)
    >> + f_list->bpid = cpu_to_le16(QDMA_FL_BMT_ENABLE); }
    >> +
    >> +static struct dma_async_tx_descriptor *dpaa2_qdma_prep_memcpy(struct
    >> +dma_chan *chan, dma_addr_t dst,
    >> + dma_addr_t src, size_t len, ulong flags) {
    >> + struct dpaa2_qdma_chan *dpaa2_chan =
    >to_dpaa2_qdma_chan(chan);
    >> + struct dpaa2_qdma_engine *dpaa2_qdma;
    >> + struct dpaa2_qdma_comp *dpaa2_comp;
    >> + struct dpaa2_fl_entry *f_list;
    >> + bool wrt_changed;
    >> + u32 format;
    >> +
    >> + dpaa2_qdma = dpaa2_chan->qdma;
    >> + dpaa2_comp = dpaa2_qdma_request_desc(dpaa2_chan);
    >> + wrt_changed = (bool)dpaa2_qdma->qdma_wrtype_fixup;
    >> +
    >> +#ifdef LONG_FORMAT
    >
    > compile flag and define, so else part is dead code??
    Ok, I will add it to Kconfig.
    >
    >> + format = QDMA_FD_LONG_FORMAT;
    >> +#else
    >> + format = QDMA_FD_SHORT_FORMAT;
    >> +#endif
    >> + /* populate Frame descriptor */
    >> + dpaa2_qdma_populate_fd(format, dpaa2_comp);
    >> +
    >> + f_list = (struct dpaa2_fl_entry *)dpaa2_comp->fl_virt_addr;
    >> +
    >> +#ifdef LONG_FORMAT
    >> + /* first frame list for descriptor buffer (logn format) */
    >> + dpaa2_qdma_populate_first_framel(f_list, dpaa2_comp,
    >> +wrt_changed);
    >> +
    >> + f_list++;
    >> +#endif
    >> +
    >> + dpaa2_qdma_populate_frames(f_list, dst, src, len,
    >> + QDMA_FL_FMT_SBF);
    >> +
    >> + return vchan_tx_prep(&dpaa2_chan->vchan, &dpaa2_comp->vdesc,
    >> +flags); }
    >> +
    >> +static enum
    >> +dma_status dpaa2_qdma_tx_status(struct dma_chan *chan,
    >> + dma_cookie_t cookie,
    >> + struct dma_tx_state *txstate) {
    >> + return dma_cookie_status(chan, cookie, txstate); }
    >> +
    >> +static void dpaa2_qdma_issue_pending(struct dma_chan *chan) {
    >> + struct dpaa2_qdma_chan *dpaa2_chan =
    >to_dpaa2_qdma_chan(chan);
    >> + struct dpaa2_qdma_engine *dpaa2_qdma = dpaa2_chan->qdma;
    >> + struct dpaa2_qdma_priv *priv = dpaa2_qdma->priv;
    >> + struct dpaa2_qdma_comp *dpaa2_comp;
    >> + struct virt_dma_desc *vdesc;
    >> + struct dpaa2_fd *fd;
    >> + unsigned long flags;
    >> + int err;
    >> +
    >> + spin_lock_irqsave(&dpaa2_chan->queue_lock, flags);
    >> + spin_lock(&dpaa2_chan->vchan.lock);
    >> + if (vchan_issue_pending(&dpaa2_chan->vchan)) {
    >> + vdesc = vchan_next_desc(&dpaa2_chan->vchan);
    >> + if (!vdesc)
    >> + goto err_enqueue;
    >> + dpaa2_comp = to_fsl_qdma_comp(vdesc);
    >> +
    >> + fd = (struct dpaa2_fd *)dpaa2_comp->fd_virt_addr;
    >> +
    >> + list_del(&vdesc->node);
    >> + list_add_tail(&dpaa2_comp->list,
    >> + &dpaa2_chan->comp_used);
    >
    >what does this list do?
    >
    It is will used in interrupt to deal dma jobs.
    >> +
    >> + /* TOBO: priority hard-coded to zero */
    >
    >You mean TODO?
    Yes.
    >
    >> + err = dpaa2_io_service_enqueue_fq(NULL,
    >> + priv->tx_queue_attr[0].fqid, fd);
    >> + if (err) {
    >> + list_del(&dpaa2_comp->list);
    >> + list_add_tail(&dpaa2_comp->list,
    >> + &dpaa2_chan->comp_free);
    >> + }
    >> + }
    >> +err_enqueue:
    >> + spin_unlock(&dpaa2_chan->vchan.lock);
    >> + spin_unlock_irqrestore(&dpaa2_chan->queue_lock, flags); }
    >> +
    >> +static int __cold dpaa2_qdma_setup(struct fsl_mc_device *ls_dev) {
    >> + struct dpaa2_qdma_priv_per_prio *ppriv;
    >> + struct device *dev = &ls_dev->dev;
    >> + struct dpaa2_qdma_priv *priv;
    >> + u8 prio_def = DPDMAI_PRIO_NUM;
    >> + int err;
    >> + int i;
    >> +
    >> + priv = dev_get_drvdata(dev);
    >> +
    >> + priv->dev = dev;
    >> + priv->dpqdma_id = ls_dev->obj_desc.id;
    >> +
    >> + /*Get the handle for the DPDMAI this interface is associate with
    >> + */
    >
    >Please run checkpatch, it should have told you that you need space after
    >comment marker /* foo...
    >
    Ok, I will check it with --strict.
    >> + err = dpdmai_open(priv->mc_io, 0, priv->dpqdma_id,
    >&ls_dev->mc_handle);
    >> + if (err) {
    >> + dev_err(dev, "dpdmai_open() failed\n");
    >> + return err;
    >> + }
    >> + dev_info(dev, "Opened dpdmai object successfully\n");
    >> +
    >> + err = dpdmai_get_attributes(priv->mc_io, 0, ls_dev->mc_handle,
    >> + &priv->dpdmai_attr);
    >> + if (err) {
    >> + dev_err(dev, "dpdmai_get_attributes() failed\n");
    >> + return err;
    >
    >so you dont close what you opened in dpdmai_open() Please give a serious
    >thought and testing to this driver
    OK, got it.
    >
    >> + }
    >> +
    >> + if (priv->dpdmai_attr.version.major > DPDMAI_VER_MAJOR) {
    >> + dev_err(dev, "DPDMAI major version mismatch\n"
    >> + "Found %u.%u, supported version
    >is %u.%u\n",
    >> + priv->dpdmai_attr.version.major,
    >> + priv->dpdmai_attr.version.minor,
    >> + DPDMAI_VER_MAJOR,
    >DPDMAI_VER_MINOR);
    >> + }
    >> +
    >> + if (priv->dpdmai_attr.version.minor > DPDMAI_VER_MINOR) {
    >> + dev_err(dev, "DPDMAI minor version mismatch\n"
    >> + "Found %u.%u, supported version
    >is %u.%u\n",
    >> + priv->dpdmai_attr.version.major,
    >> + priv->dpdmai_attr.version.minor,
    >> + DPDMAI_VER_MAJOR,
    >DPDMAI_VER_MINOR);
    >
    >what is the implication of these error, why not bail out on these?
    There should be return.
    >
    >> + }
    >> +
    >> + priv->num_pairs = min(priv->dpdmai_attr.num_of_priorities,
    >prio_def);
    >> + ppriv = kcalloc(priv->num_pairs, sizeof(*ppriv), GFP_KERNEL);
    >
    >what is the context of the fn, sleepy, atomic?
    This function will be called on qdma probe,Here is not a critical area,
    What's the problem about to use GFP_KERNEL, please let me know.
    >
    >> + if (!ppriv) {
    >> + dev_err(dev, "kzalloc for ppriv failed\n");
    >
    >this need not be logged, core will do so
    OK.
    >
    >> + return -1;
    >
    >really -1??
    I will update.
    >
    >I think this driver needs more work, please fix these issues in the comments
    >above and also see in rest of the code
    OK, I will check all of those patch about these issues in rest of the code. Thanks.
    >
    Best Regards,
    Peng
    >--
    >~Vinod
    \
     
     \ /
      Last update: 2019-06-10 11:52    [W:2.839 / U:0.488 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site