[v1] hw/dma: Add error handling for loading descriptions failing

[PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

Posted by Fea.Wang 1 year, 8 months ago

When calling the loading a description function, it should be noticed
that the function may return a failure value. Breaking the loop is one
of the possible ways to handle it.

Signed-off-by: Fea.Wang <fea.wang@sifive.com>
---
 hw/dma/xilinx_axidma.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
index 4b475e5484..b8ab5a423d 100644
--- a/hw/dma/xilinx_axidma.c
+++ b/hw/dma/xilinx_axidma.c
@@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
     }
 
     while (1) {
-        stream_desc_load(s, s->regs[R_CURDESC]);
+        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
+            break;
+        }
 
         if (s->desc.status & SDESC_STATUS_COMPLETE) {
             s->regs[R_DMASR] |= DMASR_HALTED;
@@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
     }
 
     while (len) {
-        stream_desc_load(s, s->regs[R_CURDESC]);
+        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
+            break;
+        }
 
         if (s->desc.status & SDESC_STATUS_COMPLETE) {
             s->regs[R_DMASR] |= DMASR_HALTED;
-- 
2.34.1

Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

Posted by Frank Chang 1 year, 8 months ago

Reviewed-by: Frank Chang <frank.chang@sifive.com>

Fea.Wang <fea.wang@sifive.com> 於 2024年6月3日 週一 下午1:48寫道：
>
> When calling the loading a description function, it should be noticed
> that the function may return a failure value. Breaking the loop is one
> of the possible ways to handle it.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
> ---
>  hw/dma/xilinx_axidma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 4b475e5484..b8ab5a423d 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s, StreamSink *tx_data_dev,
>      }
>
>      while (1) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s, unsigned char *buf,
>      }
>
>      while (len) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> --
> 2.34.1
>
>

Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

Posted by Edgar E. Iglesias 1 year, 8 months ago

On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:

> When calling the loading a description function, it should be noticed
> that the function may return a failure value. Breaking the loop is one
> of the possible ways to handle it.
>
> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>


Looks good, a nitpick comment, I would either squash this with patch #1
or move the change to return of error code in stream_desc_load() to this
patch.




> ---
>  hw/dma/xilinx_axidma.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
> index 4b475e5484..b8ab5a423d 100644
> --- a/hw/dma/xilinx_axidma.c
> +++ b/hw/dma/xilinx_axidma.c
> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s,
> StreamSink *tx_data_dev,
>      }
>
>      while (1) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s,
> unsigned char *buf,
>      }
>
>      while (len) {
> -        stream_desc_load(s, s->regs[R_CURDESC]);
> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
> +            break;
> +        }
>
>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>              s->regs[R_DMASR] |= DMASR_HALTED;
> --
> 2.34.1
>
>

Re: [PATCH 2/4] hw/dma: Break the loop when loading descriptions fails

Posted by Fea Wang 1 year, 8 months ago

Hi Edgar,
thank you for the advice, I will squash them in the next version of the
patch series.

Sincerely,
Fea

On Mon, Jun 3, 2024 at 6:21 PM Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:

> On Mon, Jun 3, 2024 at 7:48 AM Fea.Wang <fea.wang@sifive.com> wrote:
>
>> When calling the loading a description function, it should be noticed
>> that the function may return a failure value. Breaking the loop is one
>> of the possible ways to handle it.
>>
>> Signed-off-by: Fea.Wang <fea.wang@sifive.com>
>>
>
>
> Looks good, a nitpick comment, I would either squash this with patch #1
> or move the change to return of error code in stream_desc_load() to this
> patch.
>
>
>
>
>> ---
>>  hw/dma/xilinx_axidma.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/dma/xilinx_axidma.c b/hw/dma/xilinx_axidma.c
>> index 4b475e5484..b8ab5a423d 100644
>> --- a/hw/dma/xilinx_axidma.c
>> +++ b/hw/dma/xilinx_axidma.c
>> @@ -291,7 +291,9 @@ static void stream_process_mem2s(struct Stream *s,
>> StreamSink *tx_data_dev,
>>      }
>>
>>      while (1) {
>> -        stream_desc_load(s, s->regs[R_CURDESC]);
>> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
>> +            break;
>> +        }
>>
>>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>>              s->regs[R_DMASR] |= DMASR_HALTED;
>> @@ -348,7 +350,9 @@ static size_t stream_process_s2mem(struct Stream *s,
>> unsigned char *buf,
>>      }
>>
>>      while (len) {
>> -        stream_desc_load(s, s->regs[R_CURDESC]);
>> +        if (MEMTX_OK != stream_desc_load(s, s->regs[R_CURDESC])) {
>> +            break;
>> +        }
>>
>>          if (s->desc.status & SDESC_STATUS_COMPLETE) {
>>              s->regs[R_DMASR] |= DMASR_HALTED;
>> --
>> 2.34.1
>>
>>