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);
reinit_completion(&mas->abort_done);
geni_se_abort_m_cmd(se);
--
2.34.1> 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. How do you think about to add any tags (like “Fixes” and “Cc”) accordingly? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc7#n145 Regards, Markus
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 Konrad
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);
Thanks,
Praveen Talari
>
> Konrad
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
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
© 2016 - 2026 Red Hat, Inc.