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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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@oss.qualcomm.com>
>>> ---
>>>   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);


Konrad