linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Oliver Neukum <oneukum@suse.com>
To: Alexey Klimov <klimov.linux@gmail.com>
Cc: Yury Norov <yury.norov@gmail.com>,
	wim@iguana.be, Guenter Roeck <linux@roeck-us.net>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	USB list <linux-usb@vger.kernel.org>,
	linux-watchdog@vger.kernel.org
Subject: Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
Date: Wed, 16 Mar 2016 10:07:53 +0100	[thread overview]
Message-ID: <1458119273.6570.4.camel@suse.com> (raw)
In-Reply-To: <CALW4P++Leg_TA6FkMY=RuhWJ4524KZN_gRzHEk+rUNc-TV7ctA@mail.gmail.com>

On Wed, 2016-03-16 at 00:57 +0000, Alexey Klimov wrote:
> Hi Oliver,
> 
> On Thu, Mar 10, 2016 at 9:23 AM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Thu, 2016-03-10 at 02:29 +0000, Alexey Klimov wrote:

> >> +     streamlabs_wdt->buffer[3] = 0x80;
> >> +
> >> +     streamlabs_wdt->buffer[6] = (timeout_msec & 0xff) << 8;
> >> +     streamlabs_wdt->buffer[7] = (timeout_msec & 0xff00) >> 8;
> >
> > We have macros for such conversions. Please use them.
> 
> I screwed here. It should be:
>     buffer[6] = timeout_msec & 0xff;
>     buffer[7] = (timeout_msec >> 8) & 0xff;
> 
> However, are you talking about using swab16() function? Or migrating
> to cpu_to_le() and friends functions?
> 
> If it's acceptable way to make buffer with u16 type?
> It slightly decreases lines of code and no conversion is needed here
> that way. I can do just without swapping bytes:
>  buffer[3] = timeout_msec;
> and that's it.

NO!

You cannot make assumptions about the byte order of the host. This goes
over the wire. It must be in a defined order. You need to use the
cpu_to_* family of macros which will do the right thing. You just
open coded them.

Sorry to be very blunt here, but we absolutely must not make assumptions
about which hosts USB drivers can run on.

	HTH
		Oliver

      reply	other threads:[~2016-03-16  9:10 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-10  2:29 [PATCH] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
2016-03-10  3:54 ` Guenter Roeck
2016-03-15  1:02   ` Alexey Klimov
2016-03-15  2:24     ` Guenter Roeck
2016-03-16  1:11       ` Alexey Klimov
2016-03-10  4:08 ` kbuild test robot
2016-03-10  9:23 ` Oliver Neukum
2016-03-16  0:57   ` Alexey Klimov
2016-03-16  9:07     ` Oliver Neukum [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1458119273.6570.4.camel@suse.com \
    --to=oneukum@suse.com \
    --cc=klimov.linux@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=wim@iguana.be \
    --cc=yury.norov@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).