[PATCH v2] ftgmac100: fix dblac write test

[PATCH v2] ftgmac100: fix dblac write test

[erik-smit]

The test of the write of the dblac register was testing the old value
instead of the new value. This would accept the write of an invalid value
but subsequently refuse any following valid writes.

Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
---
Changes since v1:

Changed %ld to HWADDR_PRIx to fix building on mingw

 hw/net/ftgmac100.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
index 043ba61b86..b69e3dd14e 100644
--- a/hw/net/ftgmac100.c
+++ b/hw/net/ftgmac100.c
@@ -810,16 +810,18 @@ static void ftgmac100_write(void *opaque, hwaddr addr,
         s->phydata = value & 0xffff;
         break;
     case FTGMAC100_DBLAC: /* DMA Burst Length and Arbitration Control */
-        if (FTGMAC100_DBLAC_TXDES_SIZE(s->dblac) < sizeof(FTGMAC100Desc)) {
+        if (FTGMAC100_DBLAC_TXDES_SIZE(value) < sizeof(FTGMAC100Desc)) {
             qemu_log_mask(LOG_GUEST_ERROR,
-                          "%s: transmit descriptor too small : %d bytes\n",
-                          __func__, FTGMAC100_DBLAC_TXDES_SIZE(s->dblac));
+                          "%s: transmit descriptor too small: %" HWADDR_PRIx
+                          " bytes\n", __func__,
+                          FTGMAC100_DBLAC_TXDES_SIZE(value));
             break;
         }
-        if (FTGMAC100_DBLAC_RXDES_SIZE(s->dblac) < sizeof(FTGMAC100Desc)) {
+        if (FTGMAC100_DBLAC_RXDES_SIZE(value) < sizeof(FTGMAC100Desc)) {
             qemu_log_mask(LOG_GUEST_ERROR,
-                          "%s: receive descriptor too small : %d bytes\n",
-                          __func__, FTGMAC100_DBLAC_RXDES_SIZE(s->dblac));
+                          "%s: receive descriptor too small : %" HWADDR_PRIx
+                          " bytes\n", __func__,
+                          FTGMAC100_DBLAC_RXDES_SIZE(value));
             break;
         }
         s->dblac = value;
-- 
2.25.1

Re: [PATCH v2] ftgmac100: fix dblac write test

[Andrew Jeffery]

On Sun, 28 Jun 2020, at 23:56, erik-smit wrote:
> The test of the write of the dblac register was testing the old value
> instead of the new value. This would accept the write of an invalid value
> but subsequently refuse any following valid writes.
> 
> Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
> ---
> Changes since v1:
> 
> Changed %ld to HWADDR_PRIx to fix building on mingw

Bit of a nitpick, but the type of the value argument is uint64_t, so shouldn't 
the result of the expression captured by FTGMAC100_DBLAC_TXDES_SIZE() and 
FTGMAC100_DBLAC_RXDES_SIZE() be printed with a straight PRIx64 rather than 
HWADDR_PRIx?

Otherwise the change seems sensible, so:

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Re: [PATCH v2] ftgmac100: fix dblac write test

[Erik Smit]

Hi Andrew,

On Mon, 6 Jul 2020 at 03:59, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Sun, 28 Jun 2020, at 23:56, erik-smit wrote:
> > The test of the write of the dblac register was testing the old value
> > instead of the new value. This would accept the write of an invalid value
> > but subsequently refuse any following valid writes.
> >
> > Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
> > ---
> > Changes since v1:
> >
> > Changed %ld to HWADDR_PRIx to fix building on mingw
>
> Bit of a nitpick, but the type of the value argument is uint64_t, so shouldn't
> the result of the expression captured by FTGMAC100_DBLAC_TXDES_SIZE() and
> FTGMAC100_DBLAC_RXDES_SIZE() be printed with a straight PRIx64 rather than
> HWADDR_PRIx?

You are correct. I didn't understand the meaning of the PRI macros and
just grabbed the other PRI macro I saw getting used in the file.

-- 
Best regards,

Erik Smit

Re: [PATCH v2] ftgmac100: fix dblac write test

[Cédric Le Goater]

On 7/7/20 9:42 AM, Erik Smit wrote:
> Hi Andrew,
> 
> On Mon, 6 Jul 2020 at 03:59, Andrew Jeffery <andrew@aj.id.au> wrote:
>> On Sun, 28 Jun 2020, at 23:56, erik-smit wrote:
>>> The test of the write of the dblac register was testing the old value
>>> instead of the new value. This would accept the write of an invalid value
>>> but subsequently refuse any following valid writes.
>>>
>>> Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
>>> ---
>>> Changes since v1:
>>>
>>> Changed %ld to HWADDR_PRIx to fix building on mingw
>>
>> Bit of a nitpick, but the type of the value argument is uint64_t, so shouldn't
>> the result of the expression captured by FTGMAC100_DBLAC_TXDES_SIZE() and
>> FTGMAC100_DBLAC_RXDES_SIZE() be printed with a straight PRIx64 rather than
>> HWADDR_PRIx?
> 
> You are correct. I didn't understand the meaning of the PRI macros and
> just grabbed the other PRI macro I saw getting used in the file.

With that fixed,

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

Re: [PATCH v2] ftgmac100: fix dblac write test

[Jason Wang]

On 2020/7/13 下午8:06, Cédric Le Goater wrote:
> On 7/7/20 9:42 AM, Erik Smit wrote:
>> Hi Andrew,
>>
>> On Mon, 6 Jul 2020 at 03:59, Andrew Jeffery <andrew@aj.id.au> wrote:
>>> On Sun, 28 Jun 2020, at 23:56, erik-smit wrote:
>>>> The test of the write of the dblac register was testing the old value
>>>> instead of the new value. This would accept the write of an invalid value
>>>> but subsequently refuse any following valid writes.
>>>>
>>>> Signed-off-by: erik-smit <erik.lucas.smit@gmail.com>
>>>> ---
>>>> Changes since v1:
>>>>
>>>> Changed %ld to HWADDR_PRIx to fix building on mingw
>>> Bit of a nitpick, but the type of the value argument is uint64_t, so shouldn't
>>> the result of the expression captured by FTGMAC100_DBLAC_TXDES_SIZE() and
>>> FTGMAC100_DBLAC_RXDES_SIZE() be printed with a straight PRIx64 rather than
>>> HWADDR_PRIx?
>> You are correct. I didn't understand the meaning of the PRI macros and
>> just grabbed the other PRI macro I saw getting used in the file.
> With that fixed,
>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>
> Thanks,
>
> C.


Applied by switching to use PRIx64.

Thanks