linux-kernel - Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse keymap and processing WMI events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160609112721.GA2461@eudyptula.hq.kempniu.pl>
Date:	Thu, 9 Jun 2016 13:27:21 +0200
From:	Michał Kępień <kernel@...pniu.pl>
To:	Darren Hart <dvhart@...radead.org>
Cc:	Pali Rohár <pali.rohar@...il.com>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	Gabriele Mazzotta <gabriele.mzt@...il.com>,
	Mario Limonciello <mario_limonciello@...l.com>,
	Andy Lutomirski <luto@...nel.org>,
	Alex Hung <alex.hung@...onical.com>,
	platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] dell-wmi: Rework code for generating sparse
 keymap and processing WMI events

> > -		case 0x10:
> > -			/* Keys pressed */
> > +		case 0x0010:
> > +			/* Sequence of keys pressed */
> >  			for (i = 2; i < len; ++i)
> > -				dell_wmi_process_key(buffer_entry[i]);
> > +				dell_wmi_process_key(0x0010, buffer_entry[i]);
> >  			break;
> > -		case 0x11:
> > -			for (i = 2; i < len; ++i) {
> > -				switch (buffer_entry[i]) {
> > -				case 0xfff0:
> > -					/* Battery unplugged */
> > -					pr_debug("Battery unplugged\n");
> > -					break;
> > -				case 0xfff1:
> > -					/* Battery inserted */
> > -					pr_debug("Battery inserted\n");
> > -					break;
> > -				case 0x01e1:
> > -				case 0x02ea:
> > -				case 0x02eb:
> > -				case 0x02ec:
> > -				case 0x02f6:
> > -					/* Keyboard backlight level changed */
> > -					pr_debug("Keyboard backlight level "
> > -						 "changed\n");
> > -					break;
> > -				default:
> > -					/* Unknown event */
> > -					pr_info("Unknown WMI event type 0x11: "
> > -						"0x%x\n", (int)buffer_entry[i]);
> > -					break;
> > -				}
> > -			}
> > +		case 0x0011:
> > +			/* Sequence of events occurred */
> > +			for (i = 2; i < len; ++i)
> > +				dell_wmi_process_key(0x0011, buffer_entry[i]);
> 
> Since this is identical to case 0x010, let's avoid the duplication of code and
> handle this with a fall-through, like:
> 
> 		case 0x0010:
> 		case 0x0011:
> 			/* Sequence of events occurred */
> 			for (i = 2; i < len; ++i)
> 				dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);

I believe it was Pali's intention to make a distinction between keys
being pressed (0x0010) and other events occuring (0x0011), so perhaps a
comment after the first case label could be useful?

    case 0x0010:
        /* Sequence of keys pressed; fall through */
    case 0x0011:
        /* Sequence of events occurred */
        for (i = 2; i < len; ++i)
            dell_wmi_process_key(buffer_entry[1], buffer_entry[i]);

I'll leave it to Pali to decide, I'm just throwing in my two cents.

-- 
Best regards,
Michał Kępień