linux-kernel - Re: [PATCH v2 3/4] mtd: devices: elm: Low power transition support

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: <20130213124303.GM17852@n2100.arm.linux.org.uk>
Date:	Wed, 13 Feb 2013 12:43:03 +0000
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	"Philip, Avinash" <avinashphilip@...com>
Cc:	"Hilman, Kevin" <khilman@...com>,
	"tony@...mide.com" <tony@...mide.com>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"artem.bityutskiy@...ux.intel.com" <artem.bityutskiy@...ux.intel.com>,
	"Mohammed, Afzal" <afzal@...com>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"Nori, Sekhar" <nsekhar@...com>,
	"Hebbar, Gururaja" <gururaja.hebbar@...com>,
	"Hiremath, Vaibhav" <hvaibhav@...com>
Subject: Re: [PATCH v2 3/4] mtd: devices: elm: Low power transition support

On Wed, Feb 13, 2013 at 11:42:01AM +0000, Philip, Avinash wrote:
> On Sat, Feb 09, 2013 at 15:52:44, Russell King - ARM Linux wrote:
> > On Thu, Feb 07, 2013 at 06:06:57PM +0530, Philip Avinash wrote:
> > > +static int elm_suspend(struct device *dev)
> > > +{
> > > +	struct elm_info *info = dev_get_drvdata(dev);
> > > +	wait_queue_head_t wq;
> > > +	DECLARE_WAITQUEUE(wait, current);
> > > +
> > > +	init_waitqueue_head(&wq);
> > > +	while (1) {
> > > +		/* Make sure that ELM not running */
> > > +		if (info->idle) {
> > > +			add_wait_queue(&wq, &wait);
> > > +			schedule();
> > > +			remove_wait_queue(&wq, &wait);
> > > +		} else {
> > > +			break;
> > > +		}
> > > +	}
> > 
> > The above code looks really wrong - it will just spin endlessly with the
> > waitqueues doing nothing useful.  What are you trying to do here?
> 
> The intention of waitqeue is to make the suspend process really wait till
> the ELM module finishes current activity. Although this type of protection
> already achieved in mtd layer using nand_suspend(), this one is particularly
> required for ELM module to really make sure that *any pending* corrections to
> finish really before gone to suspend.

I don't think you understand what's going on with the above, and why the
above is completely ineffective.

1. Your wait queue head is declared on stack, and there's no references
   to it outside of this function.  So _nothing_ can activate the wait
   queue.
2. You're not changing the current thread's status away from TASK_RUNNING,
   so schedule() will either return immediately or it will schedule another
   task if it's time for this one to be preempted.

In other words, the above can be rewritten as:

	while (info->idle)
		schedule();

and it will still have the same effect.

Now, if you want to be kinder to the system, then you should use a
wait queue properly.  Put the waitqueue head in struct elm_info.  Use
wait_event() here.  And call wake_up() where you set info->idle to
false.
--
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/