Re: [PATCH v2 2/2] dell-wmi: Process only one event on devices with interface version 0

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: Text/Plain, Size: 3731 bytes --]

On Tuesday 12 January 2016 12:14:39 Michał Kępień wrote:
> > BIOS/ACPI on devices with WMI interface version 0 does not clear
> > buffer before filling it. So next time when BIOS/ACPI send WMI
> > event which is smaller as previous then it contains garbage in
> > buffer from previous event.
> > 
> > BIOS/ACPI on devices with WMI interface version 1 clears buffer and
> > sometimes send more events in buffer at one call.
> > 
> > Since commit 83fc44c32ad8 ("dell-wmi: Update code for processing
> > WMI events") dell-wmi process all events in buffer (and not just
> > first).
> > 
> > So to prevent reading garbage from buffer we will process only
> > first one event on devices with WMI interface version 0.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/platform/x86/dell-wmi.c |   16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/platform/x86/dell-wmi.c
> > b/drivers/platform/x86/dell-wmi.c index 1ad7a7b..5db9efb 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -237,6 +237,22 @@ static void dell_wmi_notify(u32 value, void
> > *context)
> > 
> >  	buffer_end = buffer_entry + buffer_size;
> > 
> > +	/*
> > +	 * BIOS/ACPI on devices with WMI interface version 0 does not
> > clear +	 * buffer before filling it. So next time when BIOS/ACPI
> > send WMI event +	 * which is smaller as previous then it contains
> > garbage in buffer from +	 * previous event.
> > +	 *
> > +	 * BIOS/ACPI on devices with WMI interface version 1 clears
> > buffer and +	 * sometimes send more events in buffer at one call.
> > +	 *
> > +	 * So to prevent reading garbage from buffer we will process only
> > first +	 * one event on devices with WMI interface version 0.
> > +	 */
> > +	if (dell_wmi_interface_version == 0 && buffer_entry < buffer_end)
> > +		if (buffer_end > buffer_entry + buffer_entry[0] + 1)
> > +			buffer_end = buffer_entry + buffer_entry[0] + 1;
> 
> Wouldn't it be a bit more clear if we clamped buffer_size before
> setting buffer_end?  E.g. like this:
> 
> 	if (buffer_size == 0)
> 		return;
> 
> 	if (dell_wmi_interface_version == 0 &&
> 	    buffer_size > buffer_entry[0] + 1)
> 		buffer_size = buffer_entry[0] + 1;
> 
> 	buffer_end = buffer_entry + buffer_size;

Before return adds correct cleanup part and code will be same as my 
original patch.

So if more people think that your code is cleaner I'm OK with replacing 
it.

> If I understand correctly, the second check on the first line added
> by your patch prevents a bad dereference when accesing
> buffer_entry[0].

Yes. Same check (buffer_entry < buffer_end) is used in next whole loop, 
so I uses it in my patch too...

> The only case when that may happen is when
> buffer_size is 0,

In this one case, yes. But you can see that buffer_entry variable is 
changing (increasing pointer offset), it means that it points to some 
entry in buffer.

> which means we got notified with rubbish anyway,
> so we can just return (perhaps with a log message, which I omitted
> above).
> 
> One more minor nit: you should probably decide between "first" and
> "one" as the phrase "only first one event" (found both in the commit
> message and in the code comment) sounds incorrect to me.

Feel free to correct commit message, I'm not very good in english...

It should mean something like this... in buffer received by bios can be 
more events. That while loop iterate over events. And this my patch on 
machines with wmi version 0 will process only *one* event. And that 
event is *first* in buffer.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]