On Fri, Nov 02, 2012 at 04:12:21PM +0100, Lars-Peter Clausen wrote:

On 11/02/2012 02:38 PM, Josh Cartwright wrote:

quoted

On Fri, Nov 02, 2012 at 10:33:44AM +0100, Lars-Peter Clausen wrote:

quoted

On 10/31/2012 07:58 PM, Josh Cartwright wrote:

[...]

quoted

quoted

quoted

+static void __init zynq_periph_clk_setup(struct device_node *np)
+{
+	struct zynq_periph_clk *periph;
+	const char *parent_names[3];
+	struct clk_init_data init;
+	struct clk *clk;
+	int err;
+	u32 reg;
+	int i;
+
+	err = of_property_read_u32(np, "reg", &reg);
+	WARN_ON(err);

Shouldn't the function abort if a error happens somewhere? Continuing here
will lead to undefined behavior. Same is probably true for the other WARN_ONs.

The way I see it is: the kernel is will be left in a bad state in the
case of any failure, regardless of if we bail out or continue.  AFAICT,
there is no clean way to recover from a failure this early.

Given that, it seems simpler (albeit marginally so) just to continue; so
that's what I chose to do.  I'm not opposed to bailing out, just not
convinced it does anything for us.

The issue with this approach is that, while you get a warning, unexpected
seemingly unrelated side-effects may happen later on. E.g. if no reg
property for the clock is specified the reg variable will be uninitialized
and contain whatever was on the stack before. The clock will be registered
nonetheless and the boot process continues. Now if the clock is enabled a
bit in a random register will be modified, which could result in strange and
abnormal behavior, which can be very hard to track down.

Okay.....but any reasonable person would start their debugging quest at
the source of the WARN_ON.  If someone sees the WARN_ON message but
stupidly chooses to ignore it, they deserves to spend the time trying to
track down abnormal behavior, so I'm still not convinced.

  Josh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121102/4744adbf/attachment.sig>