Re: [Qemu-devel] [Qemu-trivial] [PATCH net v1 0/4] Cadence GEM patches

[Peter Crosthwaite]

On Sat, May 24, 2014 at 5:17 PM, Michael Tokarev <address@hidden> wrote:
> 24.05.2014 03:06, Peter Crosthwaite wrote:
>> Ping^2!
>>
>> I'll try trivial queue too :)
>
> Actually this looks like trivial material.
>
> I'll comment in one place, for all.
>
>
>>>> Peter Crosthwaite (4):
>>>>   net: cadence_gem: Fix Tx descriptor update
>
> This appears to be a bugfix, but with an interesting "incomplete"
> update:
>
> ...
> +            unsigned    desc_first[2];
> ...
>              cpu_physical_memory_read(s->tx_desc_addr,
> -                                     (uint8_t *)&desc[0], sizeof(desc));
> +                                     (uint8_t *)&desc_first[0], 
> sizeof(desc));
>
> sizeof(desc_first), not sizeof(desc).  Also can drop extra
> "readdressing".
>

Fixed.

>>>>   net: cadence_gem: Add Tx descriptor fetch printf
>
> +    DB_PRINT("read descriptor 0x%x\n", (unsigned)packet_desc_addr);
>
> packet_desc_addr is hwaddr, which is uint64_t, here it is being cast
> to unsigned,  Is it right?

Probably not.

  Other code in this file does the same,
> but it still does not mean it's right.  Maybe it is because it uses
> only the lower 32 bits of it, or that the high bits just aren't
> useful for debugging?
>

Well they are not useful because the IP is only 32-bit but converting
to HWADDR_PRIx because its probably the right thing to do anyway.

>>>>   net: cadence_gem: Fix top comment
>
> I applied this one.
>

Thanks.

Spinning V2. Thanks for review.

Regards,
Peter

>>>>   net: cadence_gem: Comment spelling sweep
>
> This look okay, except that it clashes a bit with the first.
>
> /mjt
>