* Re: net: dsa: sja1105: use detected device id instead of DT one on mismatch
2020-08-06 16:27 net: dsa: sja1105: use detected device id instead of DT one on mismatch Colin Ian King
@ 2020-08-06 19:25 ` Vladimir Oltean
0 siblings, 0 replies; 2+ messages in thread
From: Vladimir Oltean @ 2020-08-06 19:25 UTC (permalink / raw)
To: Colin Ian King
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Jakub Kicinski, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
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
^ permalink raw reply [flat|nested] 2+ messages in thread