RE: [PATCH 3/3] OMAP4: clockdomain: Add wkup/sleep dependency support

[Rajendra Nayak <rnayak@ti.com>]

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Friday, February 11, 2011 10:08 AM
> To: Rajendra Nayak
> Cc: linux-omap@vger.kernel.org; Kevin Hilman; Benoit Cousson;
linux-arm-kernel@lists.infradead.org
> Subject: RE: [PATCH 3/3] OMAP4: clockdomain: Add wkup/sleep dependency
support
>
> On Fri, 11 Feb 2011, Rajendra Nayak wrote:
>
> > My initial version actually did have a check for cd->clkdm_name
instead
> > of cd->clkdm, and then I ran into aborts when a clkdm, though
belonging
> > to the right chip version, failed lookup (in clkdm_init) and left the
> > cd->clkdm pointer NULL. This however was due to the fact that the
> > clkdm_name populated was'nt matching the actual name,
>
> So those aborts were due to clockdomain or clockdomain dependency data
> that had errors that caused it not to have referential integrity?

Yes, I specifically found it when my script updates were actually
generating some non-matching (and hence wrong) clkdm_names.
The aborts actually helped me fix it...

>
> > Would it make sense to add an additional check here to avoid
> > an abort in case of mismatches in clkdm_name populated and
> > lookup's failing in clkdm_init?
> >
> > Something like...
> >
> > 		If (cd->clkdm) {
> > 			|= 1 << cd->clkdm->dep_bit;
> > 			atomic_set(&cd->wkdep_usecount, 0);
> > 		}
>
> That is going to fail silently.  If I'm understanding the problem
> that you're referring to correctly, it seems to me that in these
> circumstances, we want to fail loudly.  Especially now that all that
data
> is supposed to be autogenerated.  It is a symptom of a more profound
> problem that the end user should never see, no?

... so you are right. Failing silently is going to make it more difficult
to identify and fix. Maybe a WARN in else?

	if (cd->clkdm) {
		...
	} else
		WARN()

>
>
> - Paul