There is a possibility that SPI controller can get into loop due to indefinite
RDR match failures. Hence put a limit to failures and stop the sequencer.

Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
---
 hw/ssi/pnv_spi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index 41beb559c6..d605fa8b46 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -20,6 +20,7 @@
 #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
 #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
 #define PNV_SPI_FIFO_SIZE 16
+#define RDR_MATCH_FAILURE_LIMIT 16
 
 /*
  * Macro from include/hw/ppc/fdt.h
@@ -838,21 +839,31 @@ static void operation_sequencer(PnvSpi *s)
              */
             if (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1) {
                 bool rdr_matched = false;
+                static int fail_count;
                 rdr_matched = does_rdr_match(s);
                 if (rdr_matched) {
                     trace_pnv_spi_RDR_match("success");
+                    fail_count = 0;
                     /* A match occurred, increment the sequencer index. */
                     seq_index++;
                     s->status = SETFIELD(SPI_STS_SEQ_FSM, s->status,
                                     SEQ_STATE_INDEX_INCREMENT);
                 } else {
                     trace_pnv_spi_RDR_match("failed");
+                    fail_count++;
                     /*
                      * Branch the sequencer to the index coded into the op
                      * code.
                      */
                     seq_index = PNV_SPI_OPCODE_LO_NIBBLE(opcode);
                 }
+                if (fail_count >= RDR_MATCH_FAILURE_LIMIT) {
+                    qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RDR match failure"
+                                  " limit crossed %d times hence requesting "
+                                  "sequencer to stop.\n",
+                                  RDR_MATCH_FAILURE_LIMIT);
+                    stop = true;
+                }
                 /*
                  * Regardless of where the branch ended up we want the
                  * sequencer to continue shifting so we have to clear
-- 
2.39.5

On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
> There is a possibility that SPI controller can get into loop due to indefinite
> RDR match failures. Hence put a limit to failures and stop the sequencer.
>
> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
> ---
>  hw/ssi/pnv_spi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index 41beb559c6..d605fa8b46 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -20,6 +20,7 @@
>  #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>  #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>  #define PNV_SPI_FIFO_SIZE 16
> +#define RDR_MATCH_FAILURE_LIMIT 16
>  
>  /*
>   * Macro from include/hw/ppc/fdt.h
> @@ -838,21 +839,31 @@ static void operation_sequencer(PnvSpi *s)
>               */
>              if (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1) {
>                  bool rdr_matched = false;
> +                static int fail_count;

This will be shared by SPI instances, is that okay or should it be
in PnvSpi?

Other than that, looks good.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

On 27-02-2025 07:26, Nicholas Piggin wrote:
> On Sat Jan 4, 2025 at 2:18 AM AEST, Chalapathi V wrote:
>> There is a possibility that SPI controller can get into loop due to indefinite
>> RDR match failures. Hence put a limit to failures and stop the sequencer.
>>
>> Signed-off-by: Chalapathi V <chalapathi.v@linux.ibm.com>
>> ---
>>   hw/ssi/pnv_spi.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
>> index 41beb559c6..d605fa8b46 100644
>> --- a/hw/ssi/pnv_spi.c
>> +++ b/hw/ssi/pnv_spi.c
>> @@ -20,6 +20,7 @@
>>   #define PNV_SPI_OPCODE_LO_NIBBLE(x) (x & 0x0F)
>>   #define PNV_SPI_MASKED_OPCODE(x) (x & 0xF0)
>>   #define PNV_SPI_FIFO_SIZE 16
>> +#define RDR_MATCH_FAILURE_LIMIT 16
>>   
>>   /*
>>    * Macro from include/hw/ppc/fdt.h
>> @@ -838,21 +839,31 @@ static void operation_sequencer(PnvSpi *s)
>>                */
>>               if (GETFIELD(SPI_STS_RDR_FULL, s->status) == 1) {
>>                   bool rdr_matched = false;
>> +                static int fail_count;
> This will be shared by SPI instances, is that okay or should it be
> in PnvSpi?
>
> Other than that, looks good.
This should be in PnvSpi. Will update in V6. Thank You.
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>