On Sun, Nov 09, 2014 at 04:21:33PM +0100, Pali Rohár wrote:
> On Wednesday 22 October 2014 12:51:17 Pali Rohár wrote:
> > On Tuesday 21 October 2014 23:32:12 Darren Hart wrote:
> > > On Tue, Oct 21, 2014 at 12:15:24AM +0200, Pali Rohár wrote:
Hi Pali,
...
> > > > @@ -158,44 +182,117 @@ static void dell_wmi_notify(u32
> > > > value, void *context)
> > > >
> > > > }
> > > >
> > > > obj = (union acpi_object *)response.pointer;
> > > >
> > > > + if (!obj) {
> > > > + pr_info("no response\n");
> > > > + return;
> > > > + }
> > >
> > > If you intend to print this, it should probably be a bit
> > > more informative. Is "info" the right level here? I would
> > > imagine either WARN if this was a bad thing, or DEBUG if
> > > this is more for debugging the driver.
> >
> > So what you (or somebody else) prefer? warn or debug?
I was leaving that up to you based on your interpretation of severity and why
you added this. If it was just for debug purporses, the I suggest debug. If this
will impact the user in an unexpected way, then WARN.
...
> > > > + if (!dell_new_hk_type) {
> > > > + if (buffer_size >= 3 && buffer_entry[1] == 0x0)
> > > > + dell_wmi_process_key(buffer_entry[2]);
> > > >
> > > > else if (buffer_size >= 2)
> > > >
> > > > - reported_key = (int)buffer_entry[1] & 0xffff;
> > > > - else {
> > > > + dell_wmi_process_key(buffer_entry[1]);
> > >
> > > Why can we drop the 0xffff mask now?
> >
> > Because it is useless (or correct me if not!). Variable
> > buffer_entry has type u16* so operation "AND 0xFFFF" on 16bit
> > integer do nothing.
Right, of course. Thanks,
> > > > + else
> > > >
> > > > pr_info("Received unknown WMI event\n");
> > > >
> > > > - kfree(obj);
> > > > - return;
> > > > + kfree(obj);
> > > > + return;
> > > > + }
> > > > +
> > > > + buffer_end = buffer_entry + buffer_size;
> > > > +
> > > > + while (buffer_entry < buffer_end) {
> > > > +
> > > > + len = buffer_entry[0];
> > > > + if (len == 0)
> > > > + break;
> > > > +
> > > > + len++;
> > > > +
> > >
> > > Why increment len here? Are you trying to avoid a "len + 1"
> > > in the comparisons below? If so, is using "len * 2" in the
> > > debug message below correct? Please clarify.
> >
> > in buffer_entry[0] (16 bit integer) is stored length of event
> > (in 16bit units) without first (length) value. And "%*ph"
> > takes size in bytes (u8). So length in bytes (u8) units is 2
> > * length in u16 units.
Right, got it - thanks.
> > > > + if (buffer_entry+len > buffer_end) {
> > >
> > > See coding style documentation on operators. Please run
> > > patches through checkpatch.
> >
> > checkpatch.pl does not show any problem for these lines.
I thought we checked for that. Please see Documentation/CodingStyle 3.1: Spaces
with regard to spacing around binary and ternary operators. (one space on each
side of).
> Darren: PING. See my comments and questions.
Thanks for the ping :)
So please choose a loglevel and correct the whitespace and we should be good.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
