Re: [Qemu-devel] [PATCH 2/4] Endian fix an assertion in usb-msd

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2012-03-09 at 13:46 +0100, Gerd Hoffmann wrote:
> On 03/08/12 01:41, David Gibson wrote:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> > -    assert(s->csw.sig == 0x53425355);
> > +    assert(s->csw.sig == cpu_to_le32(0x53425355));
> 
> Correct but incomplete.  residue is sent in host endian instead of
> little endian too.  It is zero most of the time though, so it easily
> goes unnoticed.
> 
> Can you try the attached patch instead?

I dislike the patch as it does an in-place swap of the structure. I tend
to prefer a given structure to have a known endian, never change in
place. For example it would make it harder to ever implement endian
annotations like the kernel does (to have sparse find the bugs for us).

As such I would rather fix the access locations instead. This basically
means adding this small patch on top of my previous one (I haven't
spotted any other error) :

diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index c18e76d..d5d7f74 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -193,7 +193,7 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p)
     int len;
 
     DPRINTF("Command status %d tag 0x%x, len %zd\n",
-            s->csw.status, s->csw.tag, p->iov.size);
+            s->csw.status, le32_to_cpu(s->csw.tag), p->iov.size);
 
     assert(s->csw.sig == cpu_to_le32(0x53425355));
     len = MIN(sizeof(s->csw), p->iov.size);
@@ -233,7 +233,7 @@ static void usb_msd_command_complete(SCSIRequest *req, uint32_t status, size_t r
 
     s->csw.sig = cpu_to_le32(0x53425355);
     s->csw.tag = cpu_to_le32(req->tag);
-    s->csw.residue = s->residue;
+    s->csw.residue = cpu_to_le32(s->residue);
     s->csw.status = status != 0;
 
     if (s->packet) {