linux-kernel - Re: [PATCH 07/07] ONKEY: OnKey module for DA9052/53 PMIC v1

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] [day] [month] [year] [list]

Date:	Wed, 1 Feb 2012 09:50:43 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Ashish Jangam <ashish.jangam@...tcummins.com>
Cc:	linux-kernel@...r.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCH 07/07] ONKEY: OnKey module for DA9052/53 PMIC v1

On Wed, Feb 01, 2012 at 01:58:55PM +0530, Ashish Jangam wrote:
> On Wed, 2012-02-01 at 13:30 +0530, Ashish Jangam wrote:

> > > +	ret = da9052_reg_read(onkey->da9052, DA9052_EVENT_B_REG);
> > > +	if (ret < 0) {
> > > +		dev_err(onkey->da9052->dev,
> > > +			"da9052_onkey_report_event da9052_reg_read error %d\n",
> > > +			ret);
> > > +		ret = 1;
> > > +	} else {
> > > +		ret = ret & DA9052_EVENTB_ENONKEY;
> > > +		input_report_key(onkey->input, KEY_POWER, ret);
> > > +		input_sync(onkey->input);
> > > +	}

> > > +	if (ret)
> > > +		schedule_delayed_work(&onkey->work, msecs_to_jiffies(50));

> > Why not just schedule the work directly?  The use of ret took a bit of
> > thinking about to follow.

> schedule_dealyed_work simulates the release of the onkey button since event for release
> is not generated and ret & DA9052_EVENTB_ENONKEY is used to determine the release

That doesn't seem to address the concern.  You're setting ret in exactly
one place and scheduling the work in exactly one place, why are these
two things split?
> of the onkey button.
> > > +	error = request_threaded_irq(onkey->da9052->irq_base + onkey->irq, NULL,
> > > +				     da9052_onkey_irq,

> > This looks buggy, the resource should have the IRQ you need directly in
> > it.  The MFD core can do this for the chip core driver when it registers
> > children.

> As irq_base may get determined at runtime this will require modification to the
> defined resource struct for each mfd child in the device init function of the mfd core.
> Not sure if this is fine.

No, it really won't require that.  Please read what I wrote above: the
MFD core can do this for you.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)