linux-kernel - Re: [PATCH v2 3/4] spi: spi-geni-qcom: Don't try to set CS if an xfer is pending

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]

Date:   Thu, 17 Dec 2020 13:21:14 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Mark Brown <broonie@...nel.org>, msavaliy@....qualcomm.com,
        Akash Asthana <akashast@...eaurora.org>,
        Roja Rani Yarubandi <rojay@...eaurora.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] spi: spi-geni-qcom: Don't try to set CS if an xfer
 is pending

Hi,

On Wed, Dec 16, 2020 at 8:25 PM Stephen Boyd <swboyd@...omium.org> wrote:
>
> Quoting Douglas Anderson (2020-12-16 14:41:51)
> > If we get a timeout sending then this happens:
> > * spi_transfer_wait() will get a timeout.
> > * We'll set the chip select
> > * We'll call handle_err() => handle_fifo_timeout().
> >
> > Unfortunately that won't work so well on geni.  If we got a timeout
> > transferring then it's likely that our interrupt handler is blocked,
> > but we need that same interrupt handler to adjust the chip select.
> > Trying to set the chip select doesn't crash us but ends up confusing
> > our state machine and leads to messages like:
> >   Premature done. rx_rem = 32 bpw8
> >
> > Let's just drop the chip select request in this case.  Sure, we might
> > leave the chip select in the wrong state but it's likely it was going
> > to fail anyway and this avoids getting the driver even more confused
> > about what it's doing.
> >
> > The SPI core in general assumes that setting chip select is a simple
> > operation that doesn't fail.  Yet another reason to just reconfigure
> > the chip select line as GPIOs.
>
> Indeed.
>
> >
> > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > ---
> >
> > Changes in v2:
> > - ("spi: spi-geni-qcom: Don't try to set CS if an xfer is pending") new for v2.
> >
> >  drivers/spi/spi-geni-qcom.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > index d988463e606f..0e4fa52ac017 100644
> > --- a/drivers/spi/spi-geni-qcom.c
> > +++ b/drivers/spi/spi-geni-qcom.c
> > @@ -204,9 +204,14 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag)
> >                 goto exit;
> >         }
> >
> > -       mas->cs_flag = set_flag;
> > -
> >         spin_lock_irq(&mas->lock);
> > +       if (mas->cur_xfer) {
>
> How is it possible that cs change happens when cur_xfer is non-NULL?

I'll add this to the commit message:

spi_transfer_one_message()
 ->transfer_one() AKA spi_geni_transfer_one()
  setup_fifo_xfer()
   mas->cur_xfer = non-NULL
 spi_transfer_wait() => TIMES OUT
 msg->status != -EINPROGRESS => goto out
 if (ret != 0 ...)
  spi_set_cs()
   ->set_cs AKA spi_geni_set_cs()
    # mas->cur_xfer is non-NULL

Specifically the place where cur_xfer is usually made NULL is in the
interrupt handler.  If that doesn't run then it will be non-NULL.


> > +               dev_err(mas->dev, "Can't set CS when prev xfter running\n");
>
> xfer? or xfter?

Will fix.


-Doug