linux-kernel - Re: [PATCH v4 2/4] dmaengine: Add STM32 DMAMUX driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a88a83ba-9477-36ae-87ba-a35f477a5ca1@ti.com>
Date:   Thu, 21 Sep 2017 14:25:58 +0300
From:   Peter Ujfalusi <peter.ujfalusi@...com>
To:     Pierre-Yves MORDRET <pierre-yves.mordret@...com>,
        Vinod Koul <vinod.koul@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Russell King <linux@...linux.org.uk>,
        Dan Williams <dan.j.williams@...el.com>,
        "M'boumba Cedric Madianga" <cedric.madianga@...il.com>,
        Fabrice GASNIER <fabrice.gasnier@...com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Fabien DESSENNE <fabien.dessenne@...com>,
        Amelie Delaunay <amelie.delaunay@...com>,
        <dmaengine@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] dmaengine: Add STM32 DMAMUX driver


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 2017-09-07 14:52, Pierre-Yves MORDRET wrote:
> This patch implements the STM32 DMAMUX driver.
> 
> The DMAMUX request multiplexer allows routing a DMA request line between
> the peripherals and the DMA controllers of the product. The routing
> function is ensured by a programmable multi-channel DMA request line
> multiplexer. Each channel selects a unique DMA request line,
> unconditionally or synchronously with events from its DMAMUX
> synchronization inputs. The DMAMUX may also be used as a DMA request
> generator from programmable events on its input trigger signals
> 
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@...il.com>
> Signed-off-by: Pierre-Yves MORDRET <pierre-yves.mordret@...com>
> ---
>  Version history:
>     v4:
>         * Get rid of st,dmamux property and custom API between STM32
>           DMAMUX and DMA.
>           DMAMUX will read DMA masters from Device Tree from now on.
>           Merely one DMAMUX node is needed now.
>         * Only STM32 DMA are allowed to be connected onto DMAMUX
>         * channelID is computed locally within the driver and crafted in
>           dma_psec to be passed toward DMA master.
>           DMAMUX router sorts out which DMA master will serve the
>           request automatically.
>         * This version forbids the use of DMA in standalone and DMAMUX at
>           the same time : all clients need to be connected either on DMA
>           or DMAMUX ; no mix up

Great that you got it working w/o a custom API!
I have one comment, which actually valid for the ti-dma-crossbar driver
as well...

> +static void *stm32_dmamux_route_allocate(struct of_phandle_args *dma_spec,
> +					 struct of_dma *ofdma)
> +{
> +	struct platform_device *pdev = of_find_device_by_node(ofdma->of_node);
> +	struct stm32_dmamux_data *dmamux = platform_get_drvdata(pdev);
> +	struct stm32_dmamux *mux;
> +	u32 i, min, max, ret;
> +	unsigned long flags;
> +
> +	if (dma_spec->args_count != 3) {
> +		dev_err(&pdev->dev, "invalid number of dma mux args\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (dma_spec->args[0] > dmamux->dmamux_requests) {
> +		dev_err(&pdev->dev, "invalid mux request number: %d\n",
> +			dma_spec->args[0]);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> +	if (!mux)
> +		return ERR_PTR(-ENOMEM);
> +
> +	spin_lock_irqsave(&dmamux->lock, flags);
> +	mux->chan_id = find_first_zero_bit(dmamux->dma_inuse,
> +					   dmamux->dma_requests);

you pick the first available chan_id here under the lock.

> +	spin_unlock_irqrestore(&dmamux->lock, flags);
> +	if (mux->chan_id == dmamux->dma_requests) {
> +		dev_err(&pdev->dev, "Run out of free DMA requests\n");
> +		kfree(mux);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	/* Look for DMA Master */
> +	for (i = 1, min = 0, max = dmamux->dma_reqs[i];
> +	     i <= dmamux->dma_reqs[0];
> +	     min += dmamux->dma_reqs[i], max += dmamux->dma_reqs[++i])
> +		if (mux->chan_id < max)
> +			break;
> +	mux->master = i - 1;
> +
> +	/* The of_node_put() will be done in of_dma_router_xlate function */
> +	dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", i - 1);
> +	if (!dma_spec->np) {
> +		dev_err(&pdev->dev, "can't get dma master\n");
> +		kfree(mux);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/* Set dma request */
> +	spin_lock_irqsave(&dmamux->lock, flags);
> +	if (!IS_ERR(dmamux->clk)) {
> +		ret = clk_enable(dmamux->clk);
> +		if (ret < 0) {
> +			spin_unlock_irqrestore(&dmamux->lock, flags);
> +			kfree(mux);
> +			dev_err(&pdev->dev, "clk_prep_enable issue: %d\n", ret);
> +			return ERR_PTR(ret);
> +		}
> +	}
> +	spin_unlock_irqrestore(&dmamux->lock, flags);
> +
> +	set_bit(mux->chan_id, dmamux->dma_inuse);

But nothing stops other parallel threads to pick the same chan_id since
you have released the lock (released, got the lock to protect the set
dma request and released it again). imho the find_first_zero_bit() and
the set_bit() should be done within the same lock to avoid race conditions.

- Péter