Re: [PATCH] of: fix race when matching drivers

[Grant Likely]

On Wed, May 18, 2011 at 9:27 AM, Milton Miller <miltonm@xxxxxxx> wrote:
> If two drivers are probing devices at the same time, both will write
> their match table result to the dev->of_match cache at the same time.
>
> Only write the result if the device matches.
>
> In a thread titled "SBus devices sometimes detected, sometimes not",
> Meelis reported his SBus hme was not detected about 50% of the time.
> From the debug suggested by Grant it was obvious another driver matched
> some devices between the call to match the hme and the hme discovery
> failling.
>
> Reported-by: Meelis Roos <mroos@xxxxxxxx>
> Signed-off-by: Milton Miller <miltonm@xxxxxxx>
> ---
>
> Grant, I really think this of_match cache in the device node is bad a
> bad tradeoff, and am willing to submit patches to remove it for 2.6.40.
> It is only used by about 26 drivers and all use it once during probe
> to fill out their driver data.  It comes at the cost of a long for
> every struct device in every system.

Ah, bugger.  I had /thought/ that matching and probing were kept
together with a mutex.  So, yes, this is bad and the of_match needs to
be removed.  Thanks for volunteering to submit the patch.  It should
be backported to 2.6.39 too.

> I'll even offer to throw in a patch to cache the parsing of the
> compatible property to speed up of_device_is_compatible if needed.

That would be useful.  :-)

I'll pick up your patch right now and fire it off to Linus.

g.

>
>
>
> Index: work.git/include/linux/of_device.h
> ===================================================================
> --- work.git.orig/include/linux/of_device.h     2011-05-18 09:57:01.014386816 -0500
> +++ work.git/include/linux/of_device.h  2011-05-18 09:58:27.537431575 -0500
> @@ -21,8 +21,15 @@ extern void of_device_make_bus_id(struct
>  static inline int of_driver_match_device(struct device *dev,
>                                         const struct device_driver *drv)
>  {
> -       dev->of_match = of_match_device(drv->of_match_table, dev);
> -       return dev->of_match != NULL;
> +       const struct of_device_id *match;
> +
> +       match = of_match_device(drv->of_match_table, dev);
> +       if (match) {
> +               dev->of_match = of_match_device(drv->of_match_table, dev);
> +               return 1;
> +       }
> +
> +       return 0;
>  }
>
>  extern struct platform_device *of_dev_get(struct platform_device *dev);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/