| 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: Tue, 12 Jan 2021 16:01:18 -0800
From: Doug Anderson <dianders@...omium.org>
To: Vinod Koul <vkoul@...nel.org>
Cc: Bjorn Andersson <bjorn.andersson@...aro.org>,
Mark Brown <broonie@...nel.org>, Wolfram Sang <wsa@...nel.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
Andy Gross <agross@...nel.org>,
Matthias Kaehlcke <mka@...omium.org>,
Sumit Semwal <sumit.semwal@...aro.org>,
Amit Pundir <amit.pundir@...aro.org>,
linux-spi <linux-spi@...r.kernel.org>, linux-i2c@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/7] spi: spi-geni-qcom: Add support for GPI dma
Hi,
On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@...nel.org> wrote:
>
> We can use GPI DMA for devices where it is enabled by firmware. Add
> support for this mode
>
> Signed-off-by: Vinod Koul <vkoul@...nel.org>
> ---
> drivers/spi/spi-geni-qcom.c | 395 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 384 insertions(+), 11 deletions(-)
I did a somewhat cursory review, mostly focusing on making sure that
the non-GPI/GSI stuff doesn't regress. ;-) I think you've already
got a bunch of feedback for v2 so I'll plan to look back when I see
the v2 and maybe will find time to look at some of the GSI/GPI stuff
too...
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 512e925d5ea4..5bb0e2192734 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -2,6 +2,8 @@
> // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
>
> #include <linux/clk.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/log2.h>
> @@ -10,6 +12,7 @@
> #include <linux/pm_opp.h>
> #include <linux/pm_runtime.h>
> #include <linux/qcom-geni-se.h>
> +#include <linux/dma/qcom-gpi-dma.h>
nit: sort ordering doesn't match other includes. It seems like
existing includes in this file are sorted ignoring subdirs.
> static int spi_geni_prepare_message(struct spi_master *spi,
> struct spi_message *spi_msg)
> {
> int ret;
> struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + struct geni_se *se = &mas->se;
> +
> + mas->cur_xfer_mode = get_xfer_mode(spi);
> +
> + if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> + geni_se_select_mode(se, GENI_SE_FIFO);
You don't need to do this over and over again. We set up FIFO mode in
spi_geni_init() and it'll never change.
> + reinit_completion(&mas->xfer_done);
> + ret = setup_fifo_params(spi_msg->spi, spi);
> + if (ret)
> + dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> +
> + } else if (mas->cur_xfer_mode == GENI_GPI_DMA) {
> + mas->num_tx_eot = 0;
> + mas->num_rx_eot = 0;
> + mas->num_xfers = 0;
> + reinit_completion(&mas->tx_cb);
> + reinit_completion(&mas->rx_cb);
> + memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER));
> + geni_se_select_mode(se, GENI_GPI_DMA);
> + ret = spi_geni_map_buf(mas, spi_msg);
> +
Extra blank line?
> + } else {
> + dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode);
Please no __func__ in error messages unless you're doing a non-"dev"
print. If you want to fill your log with function names you should
redefine the generic dev_xxx() functions to prefix "__func__" in your
own kernel. You probably don't even need a printout here since
get_xfer_mode() already printed.
> +static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg)
> +{
> + struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> + mas->cur_speed_hz = 0;
> + mas->cur_bits_per_word = 0;
I think doing the above zeros will make the code a bunch slower for
FIFO mode. Specifically we can avoid a whole bunch of (very slow)
interconnect code if the speed doesn't change between transfers and
the runtime PM auto power down hasn't hit.
> @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
> spi_tx_cfg &= ~CS_TOGGLE;
> writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>
> + mas->tx = dma_request_slave_channel(mas->dev, "tx");
> + if (IS_ERR_OR_NULL(mas->tx)) {
I didn't look too closely at this since I think Mark wanted you to
look into the core DMA support, but...
In general, don't you only need to do the DMA requests if you're in GPI mode?
> + dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
> + ret = PTR_ERR(mas->tx);
> + goto out_pm;
> + } else {
No need for else since last "if" ended up with goto".
> + mas->rx = dma_request_slave_channel(mas->dev, "rx");
> + if (IS_ERR_OR_NULL(mas->rx)) {
> + dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx));
> + dma_release_channel(mas->tx);
> + ret = PTR_ERR(mas->rx);
> + goto out_pm;
> + }
> +
> + gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER;
> + mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL);
> + if (IS_ERR_OR_NULL(mas->gsi)) {
Is it ever an error? Just check against NULL?
> + dma_release_channel(mas->tx);
> + dma_release_channel(mas->rx);
> + mas->tx = NULL;
> + mas->rx = NULL;
ret = -ENOMEM ?
> static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
> len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
> len &= TRANS_LEN_MSK;
>
> + if (!xfer->cs_change) {
> + if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> + m_param |= FRAGMENTATION;
> + }
Why are you changing this? It's for FIFO mode which works correctly
the way it is. We _always_ want the FRAGMENTATION bit set because we
explicitly set the CS. I haven't tried it, but I'd imagine this
change breaks stuff? I'd expect all changes in setup_fifo_xfer() to
be removed from your patch. If there's some reason you need them then
post a separate patch.
> @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
> struct spi_transfer *xfer)
> {
> struct spi_geni_master *mas = spi_master_get_devdata(spi);
> + unsigned long timeout, jiffies;
Doesn't this shadow the global "jiffies"?
> + int ret = 0i, i;
>
> /* Terminate and return success for 0 byte length transfer */
> if (!xfer->len)
> - return 0;
> + return ret;
It feels more documenting to just leave this as "return 0".
> +
> + if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> + setup_fifo_xfer(xfer, mas, slv->mode, spi);
It's super important to return "1" in this case to tell the SPI core
that you left the transfer in progress. You don't do that anymore, so
boom.
> + } else {
> + setup_gsi_xfer(xfer, mas, slv, spi);
This feels very non-symmetric. In the FIFO case you just call a
function. in the GSI case you have a whole pile of stuff inline. Can
all the stuff below be stuck in setup_gsi_xfer() or maybe you can add
an extra wrapper function? That means you don't need the weird goto
flow in this function...
> @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
> if (ret)
> goto spi_geni_probe_runtime_disable;
>
> + /*
> + * query the mode supported and set_cs for fifo mode only
> + * for dma (gsi) mode, the gsi will set cs based on params passed in
> + * TRE
> + */
> + mas->cur_xfer_mode = get_xfer_mode(spi);
> + if (mas->cur_xfer_mode == GENI_SE_FIFO)
nit: check against != GPI mode?
Powered by blists - more mailing lists