linux-kernel - RE: [PATCH V1] da9052: ONKEY: use correct register bit for key status

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]

Date:	Tue, 11 Feb 2014 16:57:47 +0000
From:	"Opensource [Anthony Olech]" <anthony.olech.opensource@...semi.com>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	"Opensource [Anthony Olech]" <anthony.olech.opensource@...semi.com>
CC:	Paul Gortmaker <paul.gortmaker@...driver.com>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"David Dajun Chen" <david.chen@...semi.com>
Subject: RE: [PATCH V1] da9052: ONKEY: use correct register bit for key
 status

> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torokhov@...il.com]
> Sent: 11 February 2014 16:29
..[snip]..
> >  static void da9052_onkey_query(struct da9052_onkey *onkey)  {
> > -	int key_stat;
> > +	int ret, key_stat;
> >
> > -	key_stat = da9052_reg_read(onkey->da9052,
> DA9052_EVENT_B_REG);
> > -	if (key_stat < 0) {
> > +	ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
> > +	if (ret < 0) {
> >  		dev_err(onkey->da9052->dev,
> > -			"Failed to read onkey event %d\n", key_stat);
> > +			"Failed to read onkey event err=%d\n", ret);
> > +		key_stat = false;
> 
> Why do you need this assignment? Also, key_stat is integer, why are we
> using boolean values for it?
> 
> >  	} else {
> >  		/*
> >  		 * Since interrupt for deassertion of ONKEY pin is not
> >  		 * generated, onkey event state determines the onkey
> >  		 * button state.
> >  		 */
> > -		key_stat &= DA9052_EVENTB_ENONKEY;
> > +		if (ret & DA9052_STATUSA_NONKEY)
> > +			key_stat = false;
> > +		else
> > +			key_stat = true;
> 
> It seems to me that the relevant changes are replacement of
> DA9052_EVENT_B_REG -> DA9052_STATUS_A_REG in da9052_reg_read()
> and doing:
> 
> 		key_stat &= DA9052_STATUSA_NONKEY;
> 		input_report_key(onkey->input, KEY_POWER, !key_stat);
> 
> Right?

Right as far as the register and the bit within it.

However, should the register read fail due to the PMIC not responding on the i2c bus then the previous code would just loop round the work queue.
By explicitly using key_stat as a boolean it makes the code very clear that in the case of failure a 'false' value is set. As it was the negative value of the error code would be treated as true, since -EFAULT being non-zero will be treated as true.

Tony Olech

> Thanks.
> Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/