[PATCH v1 0/3] Improve SPI target mode support and error handling

[PATCH v1 0/3] Improve SPI target mode support and error handling

[Praveen Talari]

Enhance the Qualcomm GENI SPI driver's target mode support and improve 
error handling for serial engine operations.

The current implementation has issues with incorrect controller allocation
for target mode, missing abort sequence execution during error recovery,
and lack of graceful abort mechanism for target operations. These problems
can lead to serial engine undefined behavior and improper resource
cleanup.

Fix controller allocation to use proper target APIs, ensure abort sequence
always executes for error recovery, and add target abort support for 
graceful transfer cancellation.

Praveen Talari (3):
  spi: geni-qcom: Improve target mode allocation by using proper
    allocation functions
  spi: geni-qcom: Fix abort sequence execution for serial engine errors
  spi: geni-qcom: Add target abort support

 drivers/spi/spi-geni-qcom.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)


base-commit: e3b32dcb9f23e3c3927ef3eec6a5842a988fb574
-- 
2.34.1

[PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Praveen Talari]

The current implementation always allocates a host controller and sets the
target flag later when the "spi-slave" device tree property is present.
This approach is suboptimal as it doesn't utilize the dedicated allocation
functions designed for target mode.

Use devm_spi_alloc_target() when "spi-slave" device tree property is
present, otherwise use devm_spi_alloc_host(). This replaces the previous
approach of always allocating a host controller and setting target flag
later.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 0e5fd9df1a8f..f5d05025b196 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
 	struct clk *clk;
 	struct device *dev = &pdev->dev;
 
+	if (device_property_read_bool(dev, "spi-slave"))
+		spi = devm_spi_alloc_target(dev, sizeof(*mas));
+	else
+		spi = devm_spi_alloc_host(dev, sizeof(*mas));
+
+	if (!spi)
+		return -ENOMEM;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
@@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	spi = devm_spi_alloc_host(dev, sizeof(*mas));
-	if (!spi)
-		return -ENOMEM;
-
 	platform_set_drvdata(pdev, spi);
 	mas = spi_controller_get_devdata(spi);
 	mas->irq = irq;
@@ -1087,9 +1091,6 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	if (device_property_read_bool(&pdev->dev, "spi-slave"))
-		spi->target = true;
-
 	/* Set the bus quota to a reasonable value for register access */
 	mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ);
 	mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
-- 
2.34.1

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

[Praveen Talari]

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

[PATCH v1 3/3] spi: geni-qcom: Add target abort support

[Praveen Talari]

SPI target mode currently lacks a mechanism to gracefully abort ongoing
transfers when the client or core needs to cancel active transactions.

Implement spi_geni_target_abort() to handle aborting SPI target
operations when the client and core want to cancel ongoing transfers.
This provides a mechanism for graceful termination of active SPI
transactions in target mode.

Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
 drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index e5320e2fb834..231fd31de048 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -1009,6 +1009,17 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int spi_geni_target_abort(struct spi_controller *spi)
+{
+	if (!spi->cur_msg)
+		return 0;
+
+	handle_se_timeout(spi, spi->cur_msg);
+	spi_finalize_current_transfer(spi);
+
+	return 0;
+}
+
 static int spi_geni_probe(struct platform_device *pdev)
 {
 	int ret, irq;
@@ -1082,6 +1093,9 @@ static int spi_geni_probe(struct platform_device *pdev)
 	init_completion(&mas->rx_reset_done);
 	spin_lock_init(&mas->lock);
 
+	if (spi->target)
+		spi->target_abort = spi_geni_target_abort;
+
 	ret = geni_icc_get(&mas->se, NULL);
 	if (ret)
 		return ret;
-- 
2.34.1

Re: [PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Konrad Dybcio]

On 1/22/26 4:10 PM, Praveen Talari wrote:
> The current implementation always allocates a host controller and sets the
> target flag later when the "spi-slave" device tree property is present.
> This approach is suboptimal as it doesn't utilize the dedicated allocation
> functions designed for target mode.
> 
> Use devm_spi_alloc_target() when "spi-slave" device tree property is
> present, otherwise use devm_spi_alloc_host(). This replaces the previous
> approach of always allocating a host controller and setting target flag
> later.
> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 0e5fd9df1a8f..f5d05025b196 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	struct clk *clk;
>  	struct device *dev = &pdev->dev;
>  
> +	if (device_property_read_bool(dev, "spi-slave"))
> +		spi = devm_spi_alloc_target(dev, sizeof(*mas));
> +	else
> +		spi = devm_spi_alloc_host(dev, sizeof(*mas));
> +
> +	if (!spi)
> +		return -ENOMEM;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (IS_ERR(clk))
>  		return PTR_ERR(clk);
>  
> -	spi = devm_spi_alloc_host(dev, sizeof(*mas));
> -	if (!spi)
> -		return -ENOMEM;

Is there a reason you're moving this code to the top of the function?

the "main" change looks ok

Konrad

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

[Konrad Dybcio]

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

Re: [PATCH v1 3/3] spi: geni-qcom: Add target abort support

[Konrad Dybcio]

On 1/22/26 4:10 PM, Praveen Talari wrote:
> SPI target mode currently lacks a mechanism to gracefully abort ongoing
> transfers when the client or core needs to cancel active transactions.
> 
> Implement spi_geni_target_abort() to handle aborting SPI target
> operations when the client and core want to cancel ongoing transfers.
> This provides a mechanism for graceful termination of active SPI
> transactions in target mode.
> 
> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
> ---
>  drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index e5320e2fb834..231fd31de048 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -1009,6 +1009,17 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int spi_geni_target_abort(struct spi_controller *spi)
> +{
> +	if (!spi->cur_msg)
> +		return 0;
> +
> +	handle_se_timeout(spi, spi->cur_msg);

I can't help but notice this function never even dereferences this
argument

Konrad

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

[Praveen Talari]

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

Re: [PATCH v1 3/3] spi: geni-qcom: Add target abort support

[Praveen Talari]

Hi Konrad

On 1/27/2026 6:51 PM, Konrad Dybcio wrote:
> On 1/22/26 4:10 PM, Praveen Talari wrote:
>> SPI target mode currently lacks a mechanism to gracefully abort ongoing
>> transfers when the client or core needs to cancel active transactions.
>>
>> Implement spi_geni_target_abort() to handle aborting SPI target
>> operations when the client and core want to cancel ongoing transfers.
>> This provides a mechanism for graceful termination of active SPI
>> transactions in target mode.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>> index e5320e2fb834..231fd31de048 100644
>> --- a/drivers/spi/spi-geni-qcom.c
>> +++ b/drivers/spi/spi-geni-qcom.c
>> @@ -1009,6 +1009,17 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static int spi_geni_target_abort(struct spi_controller *spi)
>> +{
>> +	if (!spi->cur_msg)
>> +		return 0;
>> +
>> +	handle_se_timeout(spi, spi->cur_msg);
> 
> I can't help but notice this function never even dereferences this
> argument

Yes, you’re correct. Since the argument is never dereferenced, it is 
safe to pass either cur_msg or NULL here.

Thank,
Praveen

> 
> Konrad

Re: [PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Praveen Talari]

Hi Konrad

On 1/27/2026 6:45 PM, Konrad Dybcio wrote:
> On 1/22/26 4:10 PM, Praveen Talari wrote:
>> The current implementation always allocates a host controller and sets the
>> target flag later when the "spi-slave" device tree property is present.
>> This approach is suboptimal as it doesn't utilize the dedicated allocation
>> functions designed for target mode.
>>
>> Use devm_spi_alloc_target() when "spi-slave" device tree property is
>> present, otherwise use devm_spi_alloc_host(). This replaces the previous
>> approach of always allocating a host controller and setting target flag
>> later.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>> index 0e5fd9df1a8f..f5d05025b196 100644
>> --- a/drivers/spi/spi-geni-qcom.c
>> +++ b/drivers/spi/spi-geni-qcom.c
>> @@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
>>   	struct clk *clk;
>>   	struct device *dev = &pdev->dev;
>>   
>> +	if (device_property_read_bool(dev, "spi-slave"))
>> +		spi = devm_spi_alloc_target(dev, sizeof(*mas));
>> +	else
>> +		spi = devm_spi_alloc_host(dev, sizeof(*mas));
>> +
>> +	if (!spi)
>> +		return -ENOMEM;
>> +
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq < 0)
>>   		return irq;
>> @@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>>   	if (IS_ERR(clk))
>>   		return PTR_ERR(clk);
>>   
>> -	spi = devm_spi_alloc_host(dev, sizeof(*mas));
>> -	if (!spi)
>> -		return -ENOMEM;
> 
> Is there a reason you're moving this code to the top of the function?

When CONFIG_SPI_SLAVE is disabled, the call returns NULL; therefore, I 
placed this check at the start of the probe() function.

ref:
static inline struct spi_controller *devm_spi_alloc_target(struct device 
*dev, unsigned int size)
{
	if (!IS_ENABLED(CONFIG_SPI_SLAVE))
		return NULL;

	return __devm_spi_alloc_controller(dev, size, true);
}

Thanks,
Praveen
> 
> the "main" change looks ok
> 
> Konrad

Re: [PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Konrad Dybcio]

On 1/28/26 5:32 PM, Praveen Talari wrote:
> Hi Konrad
> 
> On 1/27/2026 6:45 PM, Konrad Dybcio wrote:
>> On 1/22/26 4:10 PM, Praveen Talari wrote:
>>> The current implementation always allocates a host controller and sets the
>>> target flag later when the "spi-slave" device tree property is present.
>>> This approach is suboptimal as it doesn't utilize the dedicated allocation
>>> functions designed for target mode.
>>>
>>> Use devm_spi_alloc_target() when "spi-slave" device tree property is
>>> present, otherwise use devm_spi_alloc_host(). This replaces the previous
>>> approach of always allocating a host controller and setting target flag
>>> later.
>>>
>>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>>> ---
>>>   drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
>>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>> index 0e5fd9df1a8f..f5d05025b196 100644
>>> --- a/drivers/spi/spi-geni-qcom.c
>>> +++ b/drivers/spi/spi-geni-qcom.c
>>> @@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>       struct clk *clk;
>>>       struct device *dev = &pdev->dev;
>>>   +    if (device_property_read_bool(dev, "spi-slave"))
>>> +        spi = devm_spi_alloc_target(dev, sizeof(*mas));
>>> +    else
>>> +        spi = devm_spi_alloc_host(dev, sizeof(*mas));
>>> +
>>> +    if (!spi)
>>> +        return -ENOMEM;
>>> +
>>>       irq = platform_get_irq(pdev, 0);
>>>       if (irq < 0)
>>>           return irq;
>>> @@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>       if (IS_ERR(clk))
>>>           return PTR_ERR(clk);
>>>   -    spi = devm_spi_alloc_host(dev, sizeof(*mas));
>>> -    if (!spi)
>>> -        return -ENOMEM;
>>
>> Is there a reason you're moving this code to the top of the function?
> 
> When CONFIG_SPI_SLAVE is disabled, the call returns NULL; therefore, I placed this check at the start of the probe() function.
> 
> ref:
> static inline struct spi_controller *devm_spi_alloc_target(struct device *dev, unsigned int size)
> {
>     if (!IS_ENABLED(CONFIG_SPI_SLAVE))
>         return NULL;
> 
>     return __devm_spi_alloc_controller(dev, size, true);
> }

That doesn't really matter since spi is not accessed beforehand
and it'd return a NULL if it failed to allocate either way

I'm not sure this is a concern nowadays with fw_devlink and
friends, but today the allocation happens after we get a clock
reference, which could throw an eprobe_defer, which I think would
cause the memory to be de-allocated again, wasting cycles

Konrad

Re: [PATCH v1 3/3] spi: geni-qcom: Add target abort support

[Konrad Dybcio]

On 1/28/26 5:28 PM, Praveen Talari wrote:
> Hi Konrad
> 
> On 1/27/2026 6:51 PM, Konrad Dybcio wrote:
>> On 1/22/26 4:10 PM, Praveen Talari wrote:
>>> SPI target mode currently lacks a mechanism to gracefully abort ongoing
>>> transfers when the client or core needs to cancel active transactions.
>>>
>>> Implement spi_geni_target_abort() to handle aborting SPI target
>>> operations when the client and core want to cancel ongoing transfers.
>>> This provides a mechanism for graceful termination of active SPI
>>> transactions in target mode.
>>>
>>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>>> ---
>>>   drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>> index e5320e2fb834..231fd31de048 100644
>>> --- a/drivers/spi/spi-geni-qcom.c
>>> +++ b/drivers/spi/spi-geni-qcom.c
>>> @@ -1009,6 +1009,17 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>>>       return IRQ_HANDLED;
>>>   }
>>>   +static int spi_geni_target_abort(struct spi_controller *spi)
>>> +{
>>> +    if (!spi->cur_msg)
>>> +        return 0;
>>> +
>>> +    handle_se_timeout(spi, spi->cur_msg);
>>
>> I can't help but notice this function never even dereferences this
>> argument
> 
> Yes, you’re correct. Since the argument is never dereferenced, it is safe to pass either cur_msg or NULL here.

Would you like to send a patch removing the unused argument?

Konrad

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

[Konrad Dybcio]

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

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

[Markus Elfring]

> 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

Re: [PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Praveen Talari]

Hi Konrad,

On 1/29/2026 5:12 PM, Konrad Dybcio wrote:
> On 1/28/26 5:32 PM, Praveen Talari wrote:
>> Hi Konrad
>>
>> On 1/27/2026 6:45 PM, Konrad Dybcio wrote:
>>> On 1/22/26 4:10 PM, Praveen Talari wrote:
>>>> The current implementation always allocates a host controller and sets the
>>>> target flag later when the "spi-slave" device tree property is present.
>>>> This approach is suboptimal as it doesn't utilize the dedicated allocation
>>>> functions designed for target mode.
>>>>
>>>> Use devm_spi_alloc_target() when "spi-slave" device tree property is
>>>> present, otherwise use devm_spi_alloc_host(). This replaces the previous
>>>> approach of always allocating a host controller and setting target flag
>>>> later.
>>>>
>>>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>>>> ---
>>>>    drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
>>>>    1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>>> index 0e5fd9df1a8f..f5d05025b196 100644
>>>> --- a/drivers/spi/spi-geni-qcom.c
>>>> +++ b/drivers/spi/spi-geni-qcom.c
>>>> @@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>>        struct clk *clk;
>>>>        struct device *dev = &pdev->dev;
>>>>    +    if (device_property_read_bool(dev, "spi-slave"))
>>>> +        spi = devm_spi_alloc_target(dev, sizeof(*mas));
>>>> +    else
>>>> +        spi = devm_spi_alloc_host(dev, sizeof(*mas));
>>>> +
>>>> +    if (!spi)
>>>> +        return -ENOMEM;
>>>> +
>>>>        irq = platform_get_irq(pdev, 0);
>>>>        if (irq < 0)
>>>>            return irq;
>>>> @@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>>        if (IS_ERR(clk))
>>>>            return PTR_ERR(clk);
>>>>    -    spi = devm_spi_alloc_host(dev, sizeof(*mas));
>>>> -    if (!spi)
>>>> -        return -ENOMEM;
>>>
>>> Is there a reason you're moving this code to the top of the function?
>>
>> When CONFIG_SPI_SLAVE is disabled, the call returns NULL; therefore, I placed this check at the start of the probe() function.
>>
>> ref:
>> static inline struct spi_controller *devm_spi_alloc_target(struct device *dev, unsigned int size)
>> {
>>      if (!IS_ENABLED(CONFIG_SPI_SLAVE))
>>          return NULL;
>>
>>      return __devm_spi_alloc_controller(dev, size, true);
>> }
> 
> That doesn't really matter since spi is not accessed beforehand
> and it'd return a NULL if it failed to allocate either way
I agree. I had also reviewed other SPI drivers as a reference for this 
implementation.

Do you want me to keep the change where earlier the host allocation was 
present, or is the current modification acceptable?

Please help on this.

Thanks,
Praveen
> 
> I'm not sure this is a concern nowadays with fw_devlink and
> friends, but today the allocation happens after we get a clock
> reference, which could throw an eprobe_defer, which I think would
> cause the memory to be de-allocated again, wasting cycles
> 
> Konrad

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

Re: [PATCH v1 3/3] spi: geni-qcom: Add target abort support

[Praveen Talari]

Hi Konrad,

On 1/29/2026 5:12 PM, Konrad Dybcio wrote:
> On 1/28/26 5:28 PM, Praveen Talari wrote:
>> Hi Konrad
>>
>> On 1/27/2026 6:51 PM, Konrad Dybcio wrote:
>>> On 1/22/26 4:10 PM, Praveen Talari wrote:
>>>> SPI target mode currently lacks a mechanism to gracefully abort ongoing
>>>> transfers when the client or core needs to cancel active transactions.
>>>>
>>>> Implement spi_geni_target_abort() to handle aborting SPI target
>>>> operations when the client and core want to cancel ongoing transfers.
>>>> This provides a mechanism for graceful termination of active SPI
>>>> transactions in target mode.
>>>>
>>>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>>>> ---
>>>>    drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++
>>>>    1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>>> index e5320e2fb834..231fd31de048 100644
>>>> --- a/drivers/spi/spi-geni-qcom.c
>>>> +++ b/drivers/spi/spi-geni-qcom.c
>>>> @@ -1009,6 +1009,17 @@ static irqreturn_t geni_spi_isr(int irq, void *data)
>>>>        return IRQ_HANDLED;
>>>>    }
>>>>    +static int spi_geni_target_abort(struct spi_controller *spi)
>>>> +{
>>>> +    if (!spi->cur_msg)
>>>> +        return 0;
>>>> +
>>>> +    handle_se_timeout(spi, spi->cur_msg);
>>>
>>> I can't help but notice this function never even dereferences this
>>> argument
>>
>> Yes, you’re correct. Since the argument is never dereferenced, it is safe to pass either cur_msg or NULL here.
> 
> Would you like to send a patch removing the unused argument?

Sure, will update new patch in next version.

Thanks,
Praveen Talari

> 
> Konrad

Re: [PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Konrad Dybcio]

On 1/29/26 4:45 PM, Praveen Talari wrote:
> Hi Konrad,
> 
> On 1/29/2026 5:12 PM, Konrad Dybcio wrote:
>> On 1/28/26 5:32 PM, Praveen Talari wrote:
>>> Hi Konrad
>>>
>>> On 1/27/2026 6:45 PM, Konrad Dybcio wrote:
>>>> On 1/22/26 4:10 PM, Praveen Talari wrote:
>>>>> The current implementation always allocates a host controller and sets the
>>>>> target flag later when the "spi-slave" device tree property is present.
>>>>> This approach is suboptimal as it doesn't utilize the dedicated allocation
>>>>> functions designed for target mode.
>>>>>
>>>>> Use devm_spi_alloc_target() when "spi-slave" device tree property is
>>>>> present, otherwise use devm_spi_alloc_host(). This replaces the previous
>>>>> approach of always allocating a host controller and setting target flag
>>>>> later.
>>>>>
>>>>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>>>>> ---
>>>>>    drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
>>>>>    1 file changed, 8 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
>>>>> index 0e5fd9df1a8f..f5d05025b196 100644
>>>>> --- a/drivers/spi/spi-geni-qcom.c
>>>>> +++ b/drivers/spi/spi-geni-qcom.c
>>>>> @@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>>>        struct clk *clk;
>>>>>        struct device *dev = &pdev->dev;
>>>>>    +    if (device_property_read_bool(dev, "spi-slave"))
>>>>> +        spi = devm_spi_alloc_target(dev, sizeof(*mas));
>>>>> +    else
>>>>> +        spi = devm_spi_alloc_host(dev, sizeof(*mas));
>>>>> +
>>>>> +    if (!spi)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>>        irq = platform_get_irq(pdev, 0);
>>>>>        if (irq < 0)
>>>>>            return irq;
>>>>> @@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
>>>>>        if (IS_ERR(clk))
>>>>>            return PTR_ERR(clk);
>>>>>    -    spi = devm_spi_alloc_host(dev, sizeof(*mas));
>>>>> -    if (!spi)
>>>>> -        return -ENOMEM;
>>>>
>>>> Is there a reason you're moving this code to the top of the function?
>>>
>>> When CONFIG_SPI_SLAVE is disabled, the call returns NULL; therefore, I placed this check at the start of the probe() function.
>>>
>>> ref:
>>> static inline struct spi_controller *devm_spi_alloc_target(struct device *dev, unsigned int size)
>>> {
>>>      if (!IS_ENABLED(CONFIG_SPI_SLAVE))
>>>          return NULL;
>>>
>>>      return __devm_spi_alloc_controller(dev, size, true);
>>> }
>>
>> That doesn't really matter since spi is not accessed beforehand
>> and it'd return a NULL if it failed to allocate either way
> I agree. I had also reviewed other SPI drivers as a reference for this implementation.
> 
> Do you want me to keep the change where earlier the host allocation was present, or is the current modification acceptable?

Please move it back

Konrad