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

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

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@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);

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

Thanks,
Praveen

> 
> 
> Konrad