linux-kernel - Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAAtXAHetS9354mN5LAneMBk07U23hq2eZfqOxoXapG384=+EzQ@mail.gmail.com>
Date:	Wed, 11 Mar 2015 15:00:17 -0700
From:	Moritz Fischer <moritz.fischer@...us.com>
To:	Paul Bolle <pebolle@...cali.nl>
Cc:	Sebastian Reichel <sre@...nel.org>,
	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

Paul,

thanks for your feedback. While developing this I looked at other drivers in
the tree and many of your comments would apply to them, too?

On Wed, Mar 11, 2015 at 2:31 AM, Paul Bolle <pebolle@...cali

>> +config POWER_RESET_SYSCON_POWEROFF
>> +     bool "Generic SYSCON regmap poweroff driver"
>
> This adds a bool symbol.

Is that an issue or is it just to substantiate the rest of your comments below?

>> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
>
> So this objectfile can never be part of a module.

Same comment would apply for power/reset/qnap-poweroff.c, right?

>> +#include <linux/module.h>
>
> Is that include needed?

Wouldn't that be needed if MODULE_DEVICE_TABLE etc, is used?


>> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
>
> This will be preprocessed away.

In my experiments it didn't work without this, are you sure it's not needed?
>
>> +module_platform_driver(syscon_poweroff_driver);
>
> I think the built-in equivalent of this would be adding a wrapper that
> only does
>     platform_driver_register(&syscon_poweroff_driver);

I couldn't find other examples in the tree doing this. Any pointers?

>> +MODULE_LICENSE("GPL v2");
>
> You probably meant
>     MODULE_LICENSE("GPL");

Sorry about that. Will fix.
>
>> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@...us.com>");
>> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
>> +MODULE_ALIAS("platform:syscon-poweroff");
>
> But these four macros will all be effectively preprocessed away, anyhow.

I'll remove them.

Thanks,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/