Hi Mark,


On 19 November 2016 at 04:12, Mark Rutland [off-list ref] wrote:

On Wed, Nov 16, 2016 at 09:49:04PM +0800, fu.wei at linaro.org wrote:

quoted

+#define for_each_platform_timer(_g) for (; _g; _g = next_platform_timer(_g))

This doesn't fit the usual for_each_* pattern, since _g has to be
manually initialised first. Either come up with a way of maknig this fit
the usual pattern, or get rid of this, and use:

        t = however_you_get_the_first_timer();

        if (!t)
                bailt_out_somehow();

        do {
                ...
        } while (t = next_platform_timer(t));

Thanks, will do

quoted

+/*
+ * Release the memory we have allocated in acpi_gtdt_init.
+ * This should be called, when the driver who called "acpi_gtdt_init" previously
+ * doesn't need the GTDT info anymore.
+ */
+void __init acpi_gtdt_release(void)
+{
+     kfree(timer_block);
+     kfree(watchdog);
+     timer_block = NULL;
+     watchdog = NULL;
+}

Why is this not simply in the error path of acpi_gtdt_init()?

quoted

+
+/*
+ * Get some basic info from GTDT table, and init the global variables above
+ * for all timers initialization of Generic Timer.
+ * This function does some validation on GTDT table.
+ */
+int __init acpi_gtdt_init(struct acpi_table_header *table)
+{

quoted

+     timer_block = kcalloc(timer_count,
+                           sizeof(struct acpi_gtdt_timer_block *),
+                           GFP_KERNEL);
+     if (!timer_block)
+             return -ENOMEM;
+
+     watchdog = kcalloc(timer_count, sizeof(struct acpi_gtdt_watchdog *),
+                        GFP_KERNEL);
+     if (!watchdog) {
+             kfree(timer_block);
+             timer_block = NULL;
+             return -ENOMEM;
+     }

Please have a common error path below, and branch to that when you need
to free these.

OK , will do

quoted

+error:
+     acpi_gtdt_release();
+     return -EINVAL;
+}

[...]

quoted

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 61a3d90..a1611d1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h

@@ -577,6 +577,13 @@ enum acpi_reconfig_event  {
 int acpi_reconfig_notifier_register(struct notifier_block *nb);
 int acpi_reconfig_notifier_unregister(struct notifier_block *nb);

+#ifdef CONFIG_ACPI_GTDT
+int acpi_gtdt_init(struct acpi_table_header *table);
+int acpi_gtdt_map_ppi(int type);
+bool acpi_gtdt_c3stop(int type);
+void acpi_gtdt_release(void);

Why do these need to be here?

What possible value is ther in exporting acpi_gtdt_release() !?

because I need to release these after mem timer init,
But like your comment in 14/15, if I fill in the entire arch_timer_mem
in one go, I don't need to export acpi_gtdt_release.

Will try this way in v17

Thanks,
Mark.

-- 
Best regards,

Fu Wei
Software Engineer
Red Hat