| 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
| ||
|
Message-ID: <20150109170849.GA16970@qualcomm.com>
Date: Fri, 9 Jan 2015 11:08:49 -0600
From: Andy Gross <agross@...eaurora.org>
To: Archit Taneja <architt@...eaurora.org>
Cc: Vinod Koul <vinod.koul@...el.com>, dmaengine@...r.kernel.org,
Kumar Gala <galak@...eaurora.org>,
linux-arm-kernel@...ts.infradead.org,
linux-arm-msm@...r.kernel.org,
Bjorn Andersson <bjorn.andersson@...ymobile.com>,
linux-kernel@...r.kernel.org
Subject: Re: [Patch v2 1/2] dmaengine: Add ADM driver
On Fri, Jan 09, 2015 at 10:08:20AM +0530, Archit Taneja wrote:
> Hi,
>
> On 01/08/2015 08:56 AM, Andy Gross wrote:
> >Signed-off-by: Andy Gross <agross@...eaurora.org>
> <snip>
>
> >+static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan,
> >+ struct scatterlist *sgl, unsigned int sg_len,
> >+ enum dma_transfer_direction direction, unsigned long flags,
> >+ void *context)
> >+{
> >+ struct adm_chan *achan = to_adm_chan(chan);
> >+ struct adm_device *adev = achan->adev;
> >+ struct adm_async_desc *async_desc;
> >+ struct scatterlist *sg;
> >+ u32 i;
> >+ u32 single_count = 0, box_count = 0, desc_offset = 0, crci = 0;
> >+ struct adm_desc_hw_box *box_desc;
> >+ struct adm_desc_hw_single *single_desc;
> >+ void *desc;
> >+ u32 *cple, *last_cmd;
> >+ u32 burst;
> >+ int blk_size = -EINVAL;
>
> blk_size should be set to 0 here. Otherwise async_desc->blk_size
> takes -EINVAL when no flow control.
We only use the blk_size value in the start_dma function, and only if
device_fc=1. So for no flow control, any value is acceptable. You can't prep a
descriptor with a bad blk_size and flow control on. So we're covered both ways.
>
> >+
> >+
> >+ if (!is_slave_direction(direction)) {
> >+ dev_err(adev->dev, "invalid dma direction\n");
> >+ return NULL;
> >+ }
> >+
> >+ /*
> >+ * get burst value from slave configuration
> >+ * If zero, default to maximum burst size
> >+ * If larger than the max transfer size, set to ADM_MAX_XFER
> >+ */
> >+ burst = (direction == DMA_MEM_TO_DEV) ?
> >+ achan->slave.dst_maxburst :
> >+ achan->slave.src_maxburst;
> >+
> >+ if (!burst || burst > ADM_MAX_XFER)
> >+ burst = ADM_MAX_XFER;
> >+
> >+ /* if using flow control, validate burst and crci values */
> >+ if (achan->slave.device_fc) {
> >+
> >+ blk_size = adm_get_blksize(burst);
> >+ if (blk_size < 0) {
> >+ dev_err(adev->dev, "invalid burst value w/ crci: %d\n",
> >+ burst);
> >+ return ERR_PTR(-EINVAL);
> >+ }
> >+
> >+ crci = achan->slave.slave_id;
> >+ if (!(crci & 0xf)) {
>
> This check isn't sufficient for the crci to lie between 1 and 15.
> slave_id passed as, say, 0xf5, will be valid.
True. The real range would be 0x10->0x1F, as bit 8 denotes the mux setting.
I'll put in a check here.
>
> <snip>
>
>
> >+/**
> >+ * adm_dma_irq - irq handler for ADM controller
> >+ * @irq: IRQ of interrupt
> >+ * @data: callback data
> >+ *
> >+ * IRQ handler for the bam controller
> >+ */
> >+static irqreturn_t adm_dma_irq(int irq, void *data)
> >+{
> >+ struct adm_device *adev = data;
> >+ u32 srcs, i;
> >+ struct adm_async_desc *async_desc;
> >+ unsigned long flags;
> >+
> >+ srcs = readl_relaxed(adev->regs +
> >+ HI_SEC_DOMAIN_IRQ_STATUS(adev->ee));
> >+
> >+ for (i = 0; i < 16; i++) {
>
> we could use adev->num_channels here instead of 16.
Right. will fix.
> <snip>
>
> >+
> >+static int adm_dma_probe(struct platform_device *pdev)
> >+{
> >+ struct adm_device *adev;
> >+ struct resource *iores;
> >+ int ret;
> >+ u32 i;
> >+
> >+ adev = devm_kzalloc(&pdev->dev, sizeof(*adev), GFP_KERNEL);
> >+ if (!adev)
> >+ return -ENOMEM;
> >+
> >+ adev->dev = &pdev->dev;
> >+
> >+ iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >+ adev->regs = devm_ioremap_resource(&pdev->dev, iores);
> >+ if (IS_ERR(adev->regs))
> >+ return PTR_ERR(adev->regs);
> >+
> >+ adev->irq = platform_get_irq(pdev, 0);
> >+ if (adev->irq < 0)
> >+ return adev->irq;
> >+
> >+ ret = of_property_read_u32(pdev->dev.of_node, "qcom,ee", &adev->ee);
> >+ if (ret) {
> >+ dev_err(adev->dev, "Execution environment unspecified\n");
> >+ return ret;
> >+ }
> >+
> >+ adev->core_clk = devm_clk_get(adev->dev, "core");
> >+ if (IS_ERR(adev->core_clk))
> >+ return PTR_ERR(adev->core_clk);
> >+
> >+ ret = clk_prepare_enable(adev->core_clk);
> >+ if (ret) {
> >+ dev_err(adev->dev, "failed to prepare/enable core clock\n");
> >+ return ret;
> >+ }
> >+
> >+ adev->iface_clk = devm_clk_get(adev->dev, "iface");
> >+ if (IS_ERR(adev->iface_clk))
> >+ return PTR_ERR(adev->iface_clk);
> >+
>
> An error here or below would leave core_clk on.
Will fix, thanks.
> >+ dev_err(adev->dev, "failed to get ADM0 C2 reset\n");
> >+ return PTR_ERR(adev->c2_reset);
> >+ }
>
> All the devm_reset_control_get() failures should disable the clocks too.
Thanks, I'll fix that up.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists