re: net: dsa: sja1105: use detected device id instead of DT one on mismatch

re: net: dsa: sja1105: use detected device id instead of DT one on mismatch

[Colin Ian King]

Hi,

Static analysis with Coverity has detected a potential issue with the
following commit:

commit 0b0e299720bb99428892a23ecbd2b4b7f61ccf6d
Author: Vladimir Oltean <olteanv@gmail.com>
Date:   Mon Aug 3 19:48:23 2020 +0300

    net: dsa: sja1105: use detected device id instead of DT one on mismatch

The analysis is as follows:

Array compared against 0 (NO_EFFECT)array_null: Comparing an array to
null is not useful: match->compatible, since the test will always
evaluate as true.

    Was match->compatible formerly declared as a pointer?

3418        for (match = sja1105_dt_ids; match->compatible; match++) {
3419                const struct sja1105_info *info = match->data;
3420

I'm not sure what the original intention was, so I was unable to fix
this hence I'm sending this report as I think it needs addressing.

Colin

Re: net: dsa: sja1105: use detected device id instead of DT one on mismatch

[Vladimir Oltean]

On Thu, Aug 06, 2020 at 05:27:11PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has detected a potential issue with the
> following commit:
> 
> commit 0b0e299720bb99428892a23ecbd2b4b7f61ccf6d
> Author: Vladimir Oltean <olteanv@gmail.com>
> Date:   Mon Aug 3 19:48:23 2020 +0300
> 
>     net: dsa: sja1105: use detected device id instead of DT one on mismatch
> 
> The analysis is as follows:
> 
> Array compared against 0 (NO_EFFECT)array_null: Comparing an array to
> null is not useful: match->compatible, since the test will always
> evaluate as true.
> 
>     Was match->compatible formerly declared as a pointer?
> 
> 3418        for (match = sja1105_dt_ids; match->compatible; match++) {
> 3419                const struct sja1105_info *info = match->data;
> 3420
> 
> I'm not sure what the original intention was, so I was unable to fix
> this hence I'm sending this report as I think it needs addressing.
> 
> Colin

The intention was to loop through sja1105_dt_ids and stop at the
sentinel:

static const struct of_device_id sja1105_dt_ids[] = {
	{ .compatible = "nxp,sja1105e", .data = &sja1105e_info },
	{ .compatible = "nxp,sja1105t", .data = &sja1105t_info },
	{ .compatible = "nxp,sja1105p", .data = &sja1105p_info },
	{ .compatible = "nxp,sja1105q", .data = &sja1105q_info },
	{ .compatible = "nxp,sja1105r", .data = &sja1105r_info },
	{ .compatible = "nxp,sja1105s", .data = &sja1105s_info },
	{ /* sentinel */ },
};

I should have looked at the definition of struct of_device_id:

/*
 * Struct used for matching a device
 */
struct of_device_id {
	char	name[32];
	char	type[32];
	char	compatible[128];
	const void *data;
};

Honestly, I had thought it's "const char *compatible" rather than "char
compatible[128]". I'm still not 100% clear why it isn't doing just that,
though, I think it has to do with some weird usage patterns such as this
one in UIO:

static struct of_device_id uio_of_genirq_match[] = {
	{ /* This is filled with module_parm */ },
	{ /* Sentinel */ },
};
MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
module_param_string(of_id, uio_of_genirq_match[0].compatible, 128, 0);
MODULE_PARM_DESC(of_id, "Openfirmware id of the device to be handled by uio");

So I had 2 options for this patch: either break the loop on
match->compatible, or on match->data. And it looks like I made the wrong
one.

Thanks,
-Vladimif