diff mbox

[2/4] Endian fix an assertion in usb-msd

Message ID 1331612562.3105.96.camel@pasglop
State New
Headers show

Commit Message

Benjamin Herrenschmidt March 13, 2012, 4:22 a.m. UTC 
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) :

Comments

Gerd Hoffmann March 13, 2012, 9:57 a.m. UTC | #1 
On 03/13/12 05:22, Benjamin Herrenschmidt wrote:
> 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) :

Squashed into the original patch and added to the usb patch queue.

cheers,
  Gerd
Benjamin Herrenschmidt March 13, 2012, 10:43 a.m. UTC | #2 
On Tue, 2012-03-13 at 10:57 +0100, Gerd Hoffmann wrote:
> On 03/13/12 05:22, Benjamin Herrenschmidt wrote:
> > 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) :
> 
> Squashed into the original patch and added to the usb patch queue.

Thanks !

Cheers,
Ben.
David Gibson March 13, 2012, 10:44 a.m. UTC | #3 
On Tue, Mar 13, 2012 at 10:57:37AM +0100, Gerd Hoffmann wrote:
> On 03/13/12 05:22, Benjamin Herrenschmidt wrote:
> > 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) :
> 
> Squashed into the original patch and added to the usb patch queue.

Excellent, thanks.
diff mbox

Patch

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) {