Re: [PATCH v1 2/3] spi: geni-qcom: Fix abort sequence execution for serial engine errors

[Praveen Talari]

Hi Konrad,

On 1/29/2026 5:18 PM, Konrad Dybcio wrote:
> On 1/28/26 5:22 PM, Praveen Talari wrote:
> > HI Konrad,
> >
> > Thank you for review.
> >
> > On 1/27/2026 6:47 PM, Konrad Dybcio wrote:
> > > On 1/22/26 4:10 PM, Praveen Talari wrote:
> > > > The driver currently skips the abort sequence for target mode when serial
> > > > engine errors occur. This leads to improper error recovery as the serial
> > > > engine may remain in an undefined state without proper cleanup, potentially
> > > > causing subsequent operations to fail or behave unpredictably.
> > > >
> > > > Fix this by ensuring the abort sequence and DMA reset always execute during
> > > > error recovery, as both are required for proper serial engine error
> > > > handling.
> > > >
> > > > Signed-off-by: Praveen Talari <praveen.talari@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/spi/spi-geni-qcom.c | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > > > index f5d05025b196..e5320e2fb834 100644
> > > > --- a/drivers/spi/spi-geni-qcom.c
> > > > +++ b/drivers/spi/spi-geni-qcom.c
> > > > @@ -167,7 +167,7 @@ static void handle_se_timeout(struct spi_controller *spi,
> > > >             * doesn`t support CMD Cancel sequnece
> > > >             */
> > > >            spin_unlock_irq(&mas->lock);
> > > > -        goto reset_if_dma;
> > > > +        goto abort;
> > > >        }
> > > >          reinit_completion(&mas->cancel_done);
> > > > @@ -178,6 +178,7 @@ static void handle_se_timeout(struct spi_controller *spi,
> > > >        if (time_left)
> > > >            goto reset_if_dma;
> > > >    +abort:
> > > >        spin_lock_irq(&mas->lock);
> > >
> > > Now that the jump is just 5 LoC, you can dispose of the goto and change it
> > > to an if-statement
> >
> > Is the modification below good?
> >
> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > index f5d05025b196..4feaf24d47ea 100644
> > --- a/drivers/spi/spi-geni-qcom.c
> > +++ b/drivers/spi/spi-geni-qcom.c
> > @@ -167,16 +167,15 @@ static void handle_se_timeout(struct spi_controller *spi,
> >                   * doesn`t support CMD Cancel sequnece
> >                   */
> >                  spin_unlock_irq(&mas->lock);
> > -               goto reset_if_dma;
> > -       }
> > -
> > -       reinit_completion(&mas->cancel_done);
> > -       geni_se_cancel_m_cmd(se);
> > -       spin_unlock_irq(&mas->lock);
> > +       } else {
> > +               reinit_completion(&mas->cancel_done);
> > +               geni_se_cancel_m_cmd(se);
> > +               spin_unlock_irq(&mas->lock);
> >
> > -       time_left = wait_for_completion_timeout(&mas->cancel_done, HZ);
> > -       if (time_left)
> > -               goto reset_if_dma;
> > +               time_left = wait_for_completion_timeout(&mas->cancel_done, HZ);
> > +               if (time_left)
> > +                       goto reset_if_dma;
> > +       }
> >
> >          spin_lock_irq(&mas->lock);
> >          reinit_completion(&mas->abort_done);
>
> I think we can make it even shorter:
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 231fd31de048..59567ef6759e 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -161,25 +161,20 @@ static void handle_se_timeout(struct spi_controller *spi,
>          xfer = mas->cur_xfer;
>          mas->cur_xfer = NULL;
>   
> -       if (spi->target) {
> -               /*
> -                * skip CMD Cancel sequnece since spi target
> -                * doesn`t support CMD Cancel sequnece
> -                */
> +       /* The controller doesn't support the Cancel commnand in target mode */
> +       if (!spi->target) {
> +               reinit_completion(&mas->cancel_done);
> +               geni_se_cancel_m_cmd(se);
> +
>                  spin_unlock_irq(&mas->lock);
> -               goto abort;
> +
> +               time_left = wait_for_completion_timeout(&mas->cancel_done, HZ);
> +               if (time_left)
> +                       goto reset_if_dma;
> +
> +               spin_lock_irq(&mas->lock);
>          }
>   
> -       reinit_completion(&mas->cancel_done);
> -       geni_se_cancel_m_cmd(se);
> -       spin_unlock_irq(&mas->lock);
> -
> -       time_left = wait_for_completion_timeout(&mas->cancel_done, HZ);
> -       if (time_left)
> -               goto reset_if_dma;
> -
> -abort:
> -       spin_lock_irq(&mas->lock);
>          reinit_completion(&mas->abort_done);
>          geni_se_abort_m_cmd(se);
>          spin_unlock_irq(&mas->lock);

Thank you for help. will review and update in next patch.

Thanks,
Praveen

> Konrad