linux-kernel - Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87mu3ia2zn.fsf@nanos.tec.linutronix.de>
Date:   Wed, 29 Jul 2020 15:02:20 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Freddy Hsin <freddy.hsin@...iatek.com>,
        linux-mediatek@...ts.infradead.or,
        linux-arm-kernel@...ts.infradead.org,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        Paul Cercueil <paul@...pouillou.net>,
        "Ben Dooks \(Codethink\)" <ben.dooks@...ethink.co.uk>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Saravana Kannan <saravanak@...gle.com>,
        linux-kernel@...r.kernel.org, chang-an.chen@...iatek.com,
        Baolin Wang <baolin.wang7@...il.com>,
        wsd_upstream@...iatek.com, kuohong.wang@...iatek.com,
        stanley.chu@...iatek.com, Freddy Hsin <freddy.hsin@...iatek.com>
Subject: Re: [PATCH v1 2/2] timer: mt6873: porting Mediatek timer driver to loadable module

Freddy,

Freddy Hsin <freddy.hsin@...iatek.com> writes:

again, please be more careful with subject lines. git log $FILE will
give you a hint. 

> porting Mediatek timer driver to loadable module

Repeating the sentence in the subject line is not giving any
information. Also changelogs want to tell the WHY and not the WHAT. This
also lacks any information why this is actually safe when booting such a
system w/o this particular driver built in. What is early boot - up to
module load - using as clocksource and timer?

> diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> index 9de7515..5504569 100644
> --- a/drivers/clocksource/mmio.c
> +++ b/drivers/clocksource/mmio.c
> @@ -21,6 +21,7 @@ u64 clocksource_mmio_readl_up(struct clocksource *c)
>  {
>  	return (u64)readl_relaxed(to_mmio_clksrc(c)->reg);
>  }
> +EXPORT_SYMBOL(clocksource_mmio_readl_up);

Again EXPORT_SYMBOL_GPL() and this wants to be a seperate patch. It has
absolutely no business with the mediatek timer changes. 
  
>  u64 clocksource_mmio_readl_down(struct clocksource *c)
>  {
> @@ -46,7 +47,7 @@ u64 clocksource_mmio_readw_down(struct clocksource *c)
>   * @bits:	Number of valid bits
>   * @read:	One of clocksource_mmio_read*() above
>   */
> -int __init clocksource_mmio_init(void __iomem *base, const char *name,
> +int clocksource_mmio_init(void __iomem *base, const char *name,
>
>  	unsigned long hz, int rating, unsigned bits,
>  	u64 (*read)(struct clocksource *))
>  {
> @@ -68,3 +69,4 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
>  
>  	return clocksource_register_hz(&cs->clksrc, hz);
>  }
> +EXPORT_SYMBOL(clocksource_mmio_init);

See above.

> diff --git a/drivers/clocksource/timer-mediatek.c b/drivers/clocksource/timer-mediatek.c
> index 9318edc..5c89b6b 100644
> --- a/drivers/clocksource/timer-mediatek.c
> +++ b/drivers/clocksource/timer-mediatek.c
> @@ -13,6 +13,9 @@
>  #include <linux/clocksource.h>
>  #include <linux/interrupt.h>
>  #include <linux/irqreturn.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
>  #include <linux/sched_clock.h>
>  #include <linux/slab.h>
>  #include "timer-of.h"
> @@ -309,5 +312,41 @@ static int __init mtk_gpt_init(struct device_node *node)
>  
>  	return 0;
>  }
> +
> +#ifdef MODULE
> +static int mtk_timer_probe(struct platform_device *pdev)
> +{
> +	int (*timer_init)(struct device_node *node);
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	timer_init = of_device_get_match_data(&pdev->dev);
> +	return timer_init(np);
> +}
> +
> +static const struct of_device_id mtk_timer_match_table[] = {
> +	{
> +		.compatible = "mediatek,mt6577-timer",
> +		.data = mtk_gpt_init,
> +	},
> +	{
> +		.compatible = "mediatek,mt6765-timer",
> +		.data = mtk_syst_init,
> +	},
> +	{}
> +};
> +
> +static struct platform_driver mtk_timer_driver = {
> +	.probe = mtk_timer_probe,
> +	.driver = {
> +		.name = "mtk-timer",
> +		.of_match_table = mtk_timer_match_table,
> +	},
> +};
> +MODULE_DESCRIPTION("MEDIATEK Module timer driver");
> +MODULE_LICENSE("GPL v2");
> +
> +module_platform_driver(mtk_timer_driver);
> +#else
>  TIMER_OF_DECLARE(mtk_mt6577, "mediatek,mt6577-timer", mtk_gpt_init);
>  TIMER_OF_DECLARE(mtk_mt6765, "mediatek,mt6765-timer", mtk_syst_init);
> +#endif

Sorry no. This is not going to happen.

The above probe, match table and platform driver structs plus the module*
thingies are going to be repeated in every single driver which is going
to support module build. Tons of boilerplate copied over and over
again.

We had exactly the same before TIMER_OF_DECLARE() came around, so pretty
please this want's to be some smart macro which handles all of this
automatically.

Thanks,

        tglx