| 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: <20210113032414.GY2771@vkoul-mobl>
Date: Wed, 13 Jan 2021 08:54:14 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Doug Anderson <dianders@...omium.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
On 12-01-21, 16:01, Doug Anderson wrote:
> 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...
Thanks for the comments, I will update the comments and post v2. All the
below comments look good to me, I will respin..
> > 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?
--
~Vinod
Powered by blists - more mailing lists