Re: [PATCH] dell-wmi: Stop storing pointers to DMI tables

[Jean Delvare <jdelvare@suse.de>]

On Mon, 18 Jan 2016 10:09:46 -0800, Andy Lutomirski wrote:
> On Mon, Jan 18, 2016 at 7:44 AM, Jean Delvare <jdelvare@suse.de> wrote:
> > On Sun,  3 Jan 2016 06:52:28 -0800, Andy Lutomirski wrote:
> >> +     if (results->err || results->keymap)
> >> +             return;         /* We already found the hotkey table. */
> >
> > Can this actually happen?
> 
> Yes, I think, if Dell ships a laptop with two tables of type 0xB2.
> There's no return code that says "I'm done", so I can't just stop
> walking the DMI data after finding what I'm looking for.

This may be another thing to consider when redesigning dmi_walk. Maybe
we should let the callback function notify when processing should stop.
If nothing else it should make things slightly faster by avoiding
callbacks for the remaining entries. And it may allow for cleaner
handling of corner cases.

> (...)
> I think the length check is correct, but the hotkey_num calculation is
> wrong.  The table is 84 bytes on my system, which makes perfect sense:
> 6 bytes of header and 78 == 13*6 bytes of entries.  But 84-4 is *not*
> a multiple of 6.

For the record, I don't have a Dell laptop myself but I have a DMI
table dump from a Latitude E6410 and the type 178 record length is 96
bytes (4 + 23 * 4.)

> > (...)
> > I think it would make sense to fix dmi_walk() so that it lets the
> > decoding function return error codes. This would avoid the
> > convoluted error code handling. Not sure why I didn't do that
> > originally :(
> 
> I think that would make sense as a followup.  It'll probably have to
> change the callback's signature, though.

Indeed. That won't be a nice and easy change, but it can still be done.

-- 
Jean Delvare
SUSE L3 Support