Oops with TQM5200 on TQM5200

[Anatolij Gustschin]

Grant Likely wrote:
> On Thu, Mar 20, 2008 at 10:17 AM, Bartlomiej Sieka <tur at semihalf.com> wrote:
>> Anatolij Gustschin wrote:
>>  > Hello Wolfgang,
>>  >
>>  > Wolfgang Grandegger wrote:
>>  >
>>  >> I just tried Linux 2.6.25-rc6 on my TQM5200 module and got the attached
>>  >> oops. Are there any known patches fixing the problems?
>>  >
>>  > try the patch below for tqm5200.dts, rebuild dtb and boot
>>  > again. Not sure if it works for Linux 2.6.25-rc6, but for
>>  > 2.6.25-rc3 it does.
>>
>>  It helps 2.6.25-rc6 too - thanks Anatolij.
>>
>>
>>  >
>>  > Anatolij
>>  > --
>>  > diff --git a/arch/powerpc/boot/dts/tqm5200.dts
>>  > b/arch/powerpc/boot/dts/tqm5200.dts
>>  > index c86464f..7c23bb3 100644
>>  > --- a/arch/powerpc/boot/dts/tqm5200.dts
>>  > +++ b/arch/powerpc/boot/dts/tqm5200.dts
>>  > @@ -83,6 +83,7 @@
>>  >         };
>>  >
>>  >         dma-controller at 1200 {
>>  > +            device_type = "dma-controller";
>>
>>  This actually fixes the Oops.
> 
> Hmm, this sounds like a band-aid; the kernel shouldn't oops if this is
> missing from the device tree.  Fail, perhaps, but oops is worrisome.
> Would someone like to investigate?

results of some further investigation:

the "bestcomm-core" driver defines its of_match table as follows
static struct of_device_id mpc52xx_bcom_of_match[] = {
	{ .type = "dma-controller", .compatible = "fsl,mpc5200-bestcomm", },
	{ .type = "dma-controller", .compatible = "mpc5200-bestcomm", },
	{},
};

so while registering the driver the drivers probe function won't be
called because of_match_node shows unexpected behavior in the case
that device tree node lacks device_type = "dma-controller" definition.

here is the current of_match_node code for quick reference:
drivers/of/base.c:309
const struct of_device_id *of_match_node(const struct of_device_id *matches,
					 const struct device_node *node)
{
	while (matches->name[0] || matches->type[0] || matches->compatible[0]) {
		int match = 1;
		if (matches->name[0])
			match &= node->name
				&& !strcmp(matches->name, node->name);
		if (matches->type[0])
			match &= node->type
				&& !strcmp(matches->type, node->type);
		if (matches->compatible[0])
			match &= of_device_is_compatible(node,
						matches->compatible);
		if (match)
			return matches;
		matches++;
	}
	return NULL;
}

So, the bestcomm-core driver provided the type field, but the
device tree node lacks it. The "if (matches->type[0])" case is true
and "node->type && !strcmp(matches->type, node->type)" evaluates to
zero, so match flag is set to zero. Now there is still a chance that
compatible property will match but even if it is the case, match flag
remains zero:
 "match &= of_device_is_compatible(node, matches->compatible)" always
sets match flag to zero if match was zero previously. of_match_node
returns NULL, the bestcomm-core driver probe won't be called and thus
the drivers bcom_engine structure won't be allocated. Referencing this
structure later causes observed Oops.

Checking bcom_eng pointer for NULL before referencing data pointed
by it prevents oopsing, but fec driver still doesn't work (because
of the lost bestcomm match and resulted task allocation failure).
Actually the compatible property exists and should match and so
the fec driver shoud work.

I suggest removing .type = "dma-controller" from the bestcomm driver's
mpc52xx_bcom_of_match table to solve the problem.

What do you think?

Signed-off-by: Anatolij Gustschin <agust at denx.de>
---
diff --git a/arch/powerpc/sysdev/bestcomm/bestcomm.c b/arch/powerpc/sysdev/bestcomm/bestcomm.c
index f589999..137d830 100644
--- a/arch/powerpc/sysdev/bestcomm/bestcomm.c
+++ b/arch/powerpc/sysdev/bestcomm/bestcomm.c
@@ -484,8 +484,8 @@ mpc52xx_bcom_remove(struct of_device *op)
 }
 
 static struct of_device_id mpc52xx_bcom_of_match[] = {
-	{ .type = "dma-controller", .compatible = "fsl,mpc5200-bestcomm", },
-	{ .type = "dma-controller", .compatible = "mpc5200-bestcomm", },
+	{ .compatible = "fsl,mpc5200-bestcomm", },
+	{ .compatible = "mpc5200-bestcomm", },
 	{},
 };