linux-kernel - Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface

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: <20110317181203.GL9597@angua.secretlab.ca>
Date:	Thu, 17 Mar 2011 12:12:03 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Ed W <lists@...dgooses.com>
Cc:	Andres Salomon <dilinger@...ued.net>, rpurdie@...ys.net,
	linux-geode@...ts.infradead.org, const@...as.ru,
	linux-kernel@...r.kernel.org
Subject: Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver
 using gpio interface

On Thu, Mar 17, 2011 at 05:24:55PM +0000, Ed W wrote:
> On 17/03/2011 16:08, Grant Likely wrote:
> > Actually, it looks like with your changes this isn't even a driver
> > anymore.  It is merely code to register a device on a specific
> > platform.  Is there any other alix-specific initialization code in the
> > kernel?  If so, you should consider relocating the device registration
> > with the rest of the alix setup code.
> 
> Agreed.  I confess that I don't understand the linux driver structure
> enough to shift the code further though
> 
> What I observe is that there is a lot of arch specific setup for ARM,
> etc, however, this is not currently done at all for x86 (which is Alix),
> so at the moment this would seem to sit slightly awkwardly with current
> x86 arch code?
> 
> Instead I found leds-net5501.c, which is for a very similar platform to
> the Alix (not quite similar enough that I could combine the files) and I
> used that as my prototype for this driver.
> 
> I think given that we already have a similar driver in the leds area
> which does platform alike setup, this gives some justification for doing
> the same with the Alix leds?

No, that's bad practise.  You should have one Alix setup file,
probably in arch/x86/platform, that takes care of all Alix specific
setup registrations.  Right now that only consists of gpio leds, but
it could potentially grow.  Note that this is only for on-time setup
and registrations.  Actual device drivers still belong under drivers/

> Additionally if we ever find we need Alix
> specific setup code then the code is ready to be used as is by the
> platform code?
> 
> 
> >>> -module_init(alix_led_init);
> >>> -module_exit(alix_led_exit);
> >>> +arch_initcall(alix_init);
> >>
> >> Why is this arch_initcall rather than module_init?   If possible, it
> >> would be good to have an unload hook as well.
> > 
> > Yes, unless you've got specific ordering constraints this should
> > definitely be module_init().
> 
> I'm out of my depth here.  I would be very happy to resubmit either way?
> 
> However, is there not a potential ordering issue if leds-alix2 is loaded
> *before* leds-gpio? Is this not the reason for making it an arch_initcall?

Nope.  It does not matter if the driver gets registered first or the
device.

> Also the same code is used in leds-5501.c - would you like me to submit
> a patch to change that also (if you confirm it should become a
> module_init call?).

Unless you've got the hardware to test, probably not.

> Thanks for final confirmation on this and I will quickly resubmit the patch?

Thanks

g.

--
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/