hw/i2c/aspeed_i2c.c: Add a check for dma_read

[PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Nabih Estefan 2 weeks, 3 days ago

If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get
stuck in an infinite retry loop. Add a check for the return value of
aspeed_i2c_dma_read that will break us out of said loop.

Signed-off-by: Nabih Estefan <nabihestefan@google.com>
---
 hw/i2c/aspeed_i2c.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 1b8ac561c3..3de72db652 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
         }
         while (bus->regs[reg_dma_len]) {
             uint8_t data;
-            aspeed_i2c_dma_read(bus, &data);
+            ret = aspeed_i2c_dma_read(bus, &data);
+            /* Check that we were able to read the DMA */
+            if (ret) {
+                break;
+            }
             trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
                                       bus->regs[reg_dma_len], data);
             ret = i2c_send(bus->bus, data);
-- 
2.52.0.457.g6b5491de43-goog

Re: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Michael Tokarev 1 day, 10 hours ago

On 1/21/26 00:11, Nabih Estefan wrote:
> If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get
> stuck in an infinite retry loop. Add a check for the return value of
> aspeed_i2c_dma_read that will break us out of said loop.
> 
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>

This looks like a qemu-stable material.

Please let me know if it isn't.

Thanks,

/mjt

> ---
>   hw/i2c/aspeed_i2c.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 1b8ac561c3..3de72db652 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
>           }
>           while (bus->regs[reg_dma_len]) {
>               uint8_t data;
> -            aspeed_i2c_dma_read(bus, &data);
> +            ret = aspeed_i2c_dma_read(bus, &data);
> +            /* Check that we were able to read the DMA */
> +            if (ret) {
> +                break;
> +            }
>               trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
>                                         bus->regs[reg_dma_len], data);
>               ret = i2c_send(bus->bus, data);

Re: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Cédric Le Goater 21 hours ago

On 2/5/26 22:03, Michael Tokarev wrote:
> On 1/21/26 00:11, Nabih Estefan wrote:
>> If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get
>> stuck in an infinite retry loop. Add a check for the return value of
>> aspeed_i2c_dma_read that will break us out of said loop.
>>
>> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> 
> This looks like a qemu-stable material.

yes.

Thanks,

C.


> 
> Please let me know if it isn't.
> 
> Thanks,
> 
> /mjt
> 
>> ---
>>   hw/i2c/aspeed_i2c.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index 1b8ac561c3..3de72db652 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
>>           }
>>           while (bus->regs[reg_dma_len]) {
>>               uint8_t data;
>> -            aspeed_i2c_dma_read(bus, &data);
>> +            ret = aspeed_i2c_dma_read(bus, &data);
>> +            /* Check that we were able to read the DMA */
>> +            if (ret) {
>> +                break;
>> +            }
>>               trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
>>                                         bus->regs[reg_dma_len], data);
>>               ret = i2c_send(bus->bus, data);
>

Re: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Cédric Le Goater 2 weeks, 2 days ago

On 1/20/26 22:11, Nabih Estefan wrote:
> If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get
> stuck in an infinite retry loop. Add a check for the return value of
> aspeed_i2c_dma_read that will break us out of said loop.


Have you ever encountered this kind of problem ?

> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>   hw/i2c/aspeed_i2c.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 1b8ac561c3..3de72db652 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
>           }
>           while (bus->regs[reg_dma_len]) {
>               uint8_t data;
> -            aspeed_i2c_dma_read(bus, &data);
> +            ret = aspeed_i2c_dma_read(bus, &data);
> +            /* Check that we were able to read the DMA */
> +            if (ret) {
> +                break;
> +            }
>               trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
>                                         bus->regs[reg_dma_len], data);
>               ret = i2c_send(bus->bus, data);


Reviewed-by: Cédric Le Goater <clg@redhat.com>


Thanks,

C.

Re: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Nabih Estefan 2 weeks, 2 days ago

On Tue, Jan 20, 2026 at 11:43 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 1/20/26 22:11, Nabih Estefan wrote:
> > If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get
> > stuck in an infinite retry loop. Add a check for the return value of
> > aspeed_i2c_dma_read that will break us out of said loop.
>
>
> Have you ever encountered this kind of problem ?

I did when testing the AST1700. In one of the patchsets we had the
DRAM memory initialized but not the address space (v5 has this fixed)
but it took me a while to figure out what was happening because
instead of failing a `i2cdump` or `i2cget` command, it would just
hang. Presumably it can only happen on misconfigured or failing
devices that have some address space issue, but since aspeed_i2c_dma
returns an error if its fails I figured checking it had no real
downside.

- Nabih

>
> > Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> > ---
> >   hw/i2c/aspeed_i2c.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > index 1b8ac561c3..3de72db652 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
> >           }
> >           while (bus->regs[reg_dma_len]) {
> >               uint8_t data;
> > -            aspeed_i2c_dma_read(bus, &data);
> > +            ret = aspeed_i2c_dma_read(bus, &data);
> > +            /* Check that we were able to read the DMA */
> > +            if (ret) {
> > +                break;
> > +            }
> >               trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
> >                                         bus->regs[reg_dma_len], data);
> >               ret = i2c_send(bus->bus, data);
>
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>
>
> Thanks,
>
> C.

RE: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Jamin Lin 2 weeks, 3 days ago

Hi Nabih,

> Subject: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read
> 
> If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get stuck
> in an infinite retry loop. Add a check for the return value of
> aspeed_i2c_dma_read that will break us out of said loop.
> 
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>
> ---
>  hw/i2c/aspeed_i2c.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> 1b8ac561c3..3de72db652 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
>          }
>          while (bus->regs[reg_dma_len]) {
>              uint8_t data;
> -            aspeed_i2c_dma_read(bus, &data);
> +            ret = aspeed_i2c_dma_read(bus, &data);
> +            /* Check that we were able to read the DMA */
> +            if (ret) {
> +                break;
> +            }
>              trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
>                                        bus->regs[reg_dma_len],
> data);
>              ret = i2c_send(bus->bus, data);
> --
> 2.52.0.457.g6b5491de43-goog


Reviewed-by Jamin Lin <jamin_lin@aspeedtech.com>

Thanks,
Jamin

Re: [PATCH] hw/i2c/aspeed_i2c.c: Add a check for dma_read

Posted by Cédric Le Goater 2 weeks, 3 days ago

On 1/20/26 22:11, Nabih Estefan wrote:
> If aspeed_i2c_dma_read fails in aspeed_i2c_bus_send currently, we get
> stuck in an infinite retry loop. Add a check for the return value of
> aspeed_i2c_dma_read that will break us out of said loop.
> 
> Signed-off-by: Nabih Estefan <nabihestefan@google.com>


Fixes: 545d6bef7097 ("aspeed/i2c: Add support for DMA transfers")

> ---
>   hw/i2c/aspeed_i2c.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 1b8ac561c3..3de72db652 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -272,7 +272,11 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
>           }
>           while (bus->regs[reg_dma_len]) {
>               uint8_t data;
> -            aspeed_i2c_dma_read(bus, &data);
> +            ret = aspeed_i2c_dma_read(bus, &data);
> +            /* Check that we were able to read the DMA */
> +            if (ret) {
> +                break;
> +            }
>               trace_aspeed_i2c_bus_send("DMA", bus->regs[reg_dma_len],
>                                         bus->regs[reg_dma_len], data);
>               ret = i2c_send(bus->bus, data);