RFC: default ioremap_*() variant defintions
The 0-day build bot detected a build issue on a patch not upstream yet that
makes a driver use iorempa_uc(), this call is now upstream but we have no
drivers yet using it, the patch in question makes the atyfb framebuffer driver
use it. The build issue was the lack of the ioremap_uc() call being implemented
on some non-x86 architectures. I *thought* I had added boiler plate code to map
the ioremap_uc() call to ioremap_nocache() for archs that do not already define
their own iorempa_uc() call, but upon further investigation it seems that was
not the case but found that this may be a bit different issue altogether.
The way include/asm-generic/io.h works for ioremap() calls and its variants is:
#ifndef CONFIG_MMU
#ifndef ioremap
#define ioremap ioremap
static inline void __iomem *ioremap(phys_addr_t offset, size_t size)
{
return (void __iomem *)(unsigned long)offset;
}
#endif
...
#define iounmap iounmap
static inline void iounmap(void __iomem *addr)
{
}
#endif
#endif /* CONFIG_MMU */
That's the gist of it, but the catch here is the ioremap_*() variants and where
they are defined. The first variant is ioremap_nocache() and then all other
variants by default map to this one. We've been stuffing the variant definitions
within the #ifndef CONFIG_MMU and I don't think we should be as otherwise each
and everyone's archs will have to add their own variant default map to the
default ioremap_nocache() or whatever. That's exaclty what we have to day, and
from what I gather the purpose of the variant boiler plate is lost. I think
we should keep the ioremap() and ioreunmap() tucked under the CONFIG_MMU
but not the variants. For instance to address the build issue for ioremap_uc()
we either define ioremap_uc() for all archs or do something like this:
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index f56094cfdeff..6e5e80d5dd0c 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -769,14 +769,6 @@ static inline void __iomem *ioremap_nocache(phys_addr_t
offset, size_t size)
}
#endif
-#ifndef ioremap_uc
-#define ioremap_uc ioremap_uc
-static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size)
-{
- return ioremap_nocache(offset, size);
-}
-#endif
-
#ifndef ioremap_wc
#define ioremap_wc ioremap_wc
static inline void __iomem *ioremap_wc(phys_addr_t offset, size_t size)
@@ -802,6 +794,14 @@ static inline void iounmap(void __iomem *addr)
#endif
#endif /* CONFIG_MMU */
+#ifndef ioremap_uc
+#define ioremap_uc ioremap_uc
+static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size)
+{
+ return ioremap_nocache(offset, size);
+}
+#endif
+
#ifdef CONFIG_HAS_IOPORT_MAP
#ifndef CONFIG_GENERIC_IOMAP
#ifndef ioport_map
This builds on x86 and other archs now and I can verify that the default
boilerplate code is not used on x86. One small caveat:
I have no idea why its not picking up asm-generic ioremap_uc() for x86,
although this is the right thing to do the guard should not work as we never
define ioremap_uc() as a macro but we do as an exported symbol. The way
archs do their ioremap calls is they define externs and then they include
asm-generic to pick up the things they don't define. In the extern calls
for ioremap_uc() we never add a define there. Adding a simple one line
#define right after the extern declaration to itself should suffice to
fix paranoia but curious why this does work as-is right now.
I'd like review and consensus from other architecture folks if this is
the Right Thing To Do (TM) and if it is decide how we want to fix this.
For instance my preference would be to just add this fix as-is after
we've figured out the guard thing above, and then define these variants
in the documentation on the asm-generic file as safe to not have, and
safe to map to the default ioremap call. If you don't have a safe call
like this we should set out different expectations, for instance having
it depend on Kconfig symbol and then drivers depend on it, archs then
implement the symbols and can HAVE_ARCH_FOO. If everyone agrees with
this then there is quite a bit of cleanup possible as tons of archs do
tons of their own variant mapping definitions.
Luis
___
Re: RFC: default ioremap_*() variant defintions
On Sat, Jul 04, 2015 at 06:54:40AM -0700, Dan Williams wrote: > On Fri, Jul 3, 2015 at 11:17 AM, Luis R. Rodriguez wrote: > > I have no idea why its not picking up asm-generic ioremap_uc() for x86, > > x86 does not use "include/asm-generic/io.h". That's a helper-include > for archs that want to use it, but it's incomplete, see the patch > referenced below... Ah it includes iomap.h not io.h odd... > > although this is the right thing to do the guard should not work as we never > > define ioremap_uc() as a macro but we do as an exported symbol. The way > > archs do their ioremap calls is they define externs and then they include > > asm-generic to pick up the things they don't define. In the extern calls > > for ioremap_uc() we never add a define there. Adding a simple one line > > #define right after the extern declaration to itself should suffice to > > fix paranoia but curious why this does work as-is right now. > > > > I'd like review and consensus from other architecture folks if this is > > the Right Thing To Do (TM) and if it is decide how we want to fix this. > > For instance my preference would be to just add this fix as-is after > > we've figured out the guard thing above, and then define these variants > > in the documentation on the asm-generic file as safe to not have, and > > safe to map to the default ioremap call. If you don't have a safe call > > like this we should set out different expectations, for instance having > > it depend on Kconfig symbol and then drivers depend on it, archs then > > implement the symbols and can HAVE_ARCH_FOO. If everyone agrees with > > this then there is quite a bit of cleanup possible as tons of archs do > > tons of their own variant mapping definitions. > > We're also discussing this issue in the patch here: > > https://lkml.org/lkml/2015/6/22/98 > "[PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases": Great, you've done all the exact work I expected we needed to do ;) > Russell mentioned wanting to fix up ioremap_wt() for ARM [1], after > which I would look to re-spin this patch with the interface proposed > by Christoph [2]. > > [1]: https://lkml.org/lkml/2015/7/1/125 > [2]: https://lkml.org/lkml/2015/6/22/391 OK thanks I guess I'll jump in there as I have some feedback. Luis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: RFC: default ioremap_*() variant defintions
On Fri, Jul 03, 2015 at 08:09:24PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 03, 2015 at 08:17:09PM +0200, Luis R. Rodriguez wrote:
> > The 0-day build bot detected a build issue on a patch not upstream yet that
> > makes a driver use iorempa_uc(), this call is now upstream but we have no
> > drivers yet using it, the patch in question makes the atyfb framebuffer
> > driver use it. The build issue was the lack of the ioremap_uc() call being
> > implemented on some non-x86 architectures.
>
> You have to be careful here. We've been through this with ioremap_wt()
> which is incorrect for ARM as things stand at the moment (I have patches
> which adds documentation on this issue, and fixes ioremap_wt().)
Thanks, it seems that going in for v4.2 is that right?
> The question is whether the allocated memory may have unaligned accesses
> performed on it - either intentionally, or unintentionally (eg, by GCC
> inlining memcpy().)
>
> If the answer to that question is yes or possibly yes, neither ioremap()
> nor ioremap_nocache() (which is, unfortunately, the same as ioremap() due
> to various documentation telling people to use it for devices) can be used
> for the mapping on ARM.
There is a difference between unaligned accesses being an issue for an
architecture (not HAVE_EFFICIENT_UNALIGNED_ACCESS) on a mapping area Vs having
a requirement for all device drivers to not have unaligned accesses for mapped
memory, it seems we want the later anyway... however we obviously don't have a
scraper to go and vet all drivers / check a driver it seems. Hrm, I wonder
if we can use grammar checks for this. Anyway, OK got it!
> So we can't go around adding new ioremap() variants and hoping that
> ioremap_nocache() is a safe default everywhere. It isn't.
Sure, OK.
> > I *thought* I had added boiler plate code to map
> > the ioremap_uc() call to ioremap_nocache() for archs that do not already
> > define
> > their own iorempa_uc() call, but upon further investigation it seems that
> > was
> > not the case but found that this may be a bit different issue altogether.
> >
> > The way include/asm-generic/io.h works for ioremap() calls and its variants
> > is:
> >
> > #ifndef CONFIG_MMU
> >
> > #ifndef ioremap
> >
> > #define ioremap ioremap
> >
> > static inline void __iomem *ioremap(phys_addr_t offset, size_t size)
> >
> > {
> >
> > return (void __iomem *)(unsigned long)offset;
> >
> > }
> >
> > #endif
> > ...
> > #define iounmap iounmap
> >
> >
> >
> > static inline void iounmap(void __iomem *addr)
> >
> > {
> >
> > }
> >
> > #endif
> >
> > #endif /* CONFIG_MMU */
>
> > That's the gist of it, but the catch here is the ioremap_*() variants and
> > where
> > they are defined. The first variant is ioremap_nocache() and then all other
> > variants by default map to this one. We've been stuffing the variant
> > definitions
> > within the #ifndef CONFIG_MMU and I don't think we should be as otherwise
> > each
> > and everyone's archs will have to add their own variant default map to the
> > default ioremap_nocache() or whatever. That's exaclty what we have to day,
> > and
> > from what I gather the purpose of the variant boiler plate is lost. I think
> > we should keep the ioremap() and ioreunmap() tucked under the CONFIG_MMU
> > but not the variants. For instance to address the build issue for
> > ioremap_uc()
> > we either define ioremap_uc() for all archs or do something like this:
>
> Surely, if architectures can support the different variants, then they
> should implement them?
Sure, the asm-generic file seemed to allude to that some ioremaps are safe
to have defaults for, if we want to address this as a problem it must
be addressed there, this is
Re: [RFC] fs: add userspace critical mounts event support
On Wed, Oct 05, 2016 at 09:46:33PM +0200, Luis R. Rodriguez wrote:
> On Wed, Oct 05, 2016 at 11:08:06AM -0700, Linus Torvalds wrote:
> > On Wed, Oct 5, 2016 at 11:00 AM, Luis R. Rodriguez
> > wrote:
> > > On Tue, Sep 13, 2016 at 09:38:17PM -0500, Rob Landley wrote:
> > >
> > >> I did some shuffling around of those code to make initmpfs work, does
> > >> anybody know why initramfs extraction _before_ we initialize drivers
> > >> would be a bad thing?
> > >
> > > No, but it seems sensible to me, if its done before do_initcalls()
> > > that should resolve the race for initramfs users
> >
> > initramfs should already be set up before drivers are.
>
> Actually you are right, the issue would only be for old initrd, for initramfs
> we populate that via rootfs_initcall(populate_rootfs), so as long as drivers
> in question use an init level beyond rootfs's we're good there.
>
> > Exactly what is it that has trouble right now?
>
> It would seem then that the only current stated race possible should
> then be non-initramfs users.
Or:
a) initramfs users that include a driver but do not include the firmware
into initramfs
b) driver is built-in but firmware is not in initrafms (Johannes reports
this causes driver failure on intel wireless for instance, and I guess
you need to reload)
> One example if very large firmware for
> remote-proc, whereby an initramfs is just not practical or desirable.
This issue still stands. At Plumbers Johannes Berg did indicate to me
he had a simple elegant solution in mind. He suggested that since the
usermode helper was available, he had added support to be able to
differentiate async firmware request calls form sync requests, and that
userspace should not return an error *iff* the request made was async and
it can determine we're initramfs. The semantics issue is the same though,
is there a generic way to determine we're initramfs ? What if we move
multiple levels? Anyway -- provided we could figure this out, userspace
would simply yield and wait until the real rootfs is met. Upon pivot_root()
the assumption is all previous udev events pending would be re-triggered
and finally udev could finally confirm/deny if the firmware was present.
This would *also* allow you to stuff your firmware whever, however big
it was. This however relied on the userspace firmware loading support,
it turns out that (I think because of an incorrect negative backlash
back in the day over blaming this over booting issues due to the timeout
whereas the real issue was the kmod timeout was affecting our long
standing serial init()/probe()) the systemd userspace firmware laoding
support was removed from systemd udev in 2014 by Kay via commit
be2ea723b1d023b3d ("udev: remove userspace firmware loading support").
Systemd might *still* be able to provide a solution here, however I will
note Johannes was asking for *all* async firmware requests to always
rely on the kernel syfs UMH fallback -- this suggestion is against the
direction we've been taking to eventually compartamentalize the kernel UMH
code, so whatever we decide to do, lets please take a breather and seriously
address this properly *with* systemd folks.
A side race discussed at Plumbers worth mentioning given its related to the
UMH was inspired by Jiri's talk on the abuse of the freezer for kthreads --
and his suggestion to use freeze_super(). Currently the UMH lock is used
for the UMH but as I have noted in Daniel Wagner's recent patches to
give some love to this code and further compartamentalize the UMH --
the UMH lock was originally added to help avoid drivers use the firmware
API on resume, given the races. The firmware cache solution implemented by
Ming Lei years ago helped address this, whereby devm helpers are used
based on the requested firmware and prior to suspend we cache all required
firmware for drivers so that upon resume calls would work without the
effective race present. This mitigated the actual races / issues with
drivers, but they must not use the firmware API on suspend/resume. Since
this solution *kills* all pending UMH caller on suspend obviously this
means on suspend using request_firmware*() API and expecting it to work
is brutally dumb as we will eventually kill any pending requests. This
is a long winded way to say that if you rely on the UMH for firmware
you must figure out your own proactive firmware cache solution and
must definitely not request firmware on suspend. Two things then:
1) I've been brainstorming with Daniel how to use freeze_super() to
replace the now invalid UMH lock -- its purpose only helps races
on boot, for the fallback case to the UMH. But note most distributions
disable forcing it always on, so these days we *only* rely on the UMH
as a fallback if the driver explicitly requested i
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Nov 8, 2016 at 2:47 PM, Luis R. Rodriguez wrote:
> Whatever the outcome of this discussion is -- Johannes seemed to *want*
> to further use the UMH by default on *all* async alls... even if the
> driver did not explicitly requested it -- I'm concerned about this given
> all the above and the existing flip/flop on systemd for it. Whatever
> we try to dream up here, please consider all the above as well.
One addition to this: the current API does not always require the UMH
firmware fallback, for most distributions that do not enable
CONFIG_FW_LOADER_USER_HELPER_FALLBACK but do enable
CONFIG_FW_LOADER_USER_HELPER we only require the UMH firmware fallback
*iff* the driver explicitly requests it. For kernels with
CONFIG_FW_LOADER_USER_HELPER_FALLBACK enabled we *always* use the UMH
fallback. By fallback note that this means its used only if the first
direct filesystem request failed. For further details on complexities
of the UMH refer to two ongoing threads [0] [1] about it.
Johannes, you seemed to note you added some uevent classifier for
async requests, I checked and it seems this was with commit
e9045f9178f3e ("firmware class: export nowait to userspace") was this
the change you were referring to ? Even with all these complexities
annotated, do you still believe we need the UMH always for all async
calls ?
[0] https://lkml.kernel.org/r/20161109211741.gi13...@wotan.suse.de
[1] https://lkml.kernel.org/r/20161109220210.gj13...@wotan.suse.de
Luis
Re: [RFC] fs: add userspace critical mounts event support
On Wed, Nov 9, 2016 at 3:21 AM, Andy Lutomirski wrote: > On Wed, Nov 9, 2016 at 1:13 AM, Daniel Wagner > wrote: >> [CC: added Harald] >> >> As Harald pointed out over a beer yesterday evening, there is at least >> one more reason why UMH isn't obsolete. The ordering of the firmware loading >> might be of important. Say you want to greet the user with a splash screen >> really early on, the graphic card firmware should be loaded first. Also the >> automotive world has this fancy requirement that rear camera must be on the >> screen within 2 seconds. So controlling the firmware loading order is of >> importance (e.g. also do not overcommit the I/O bandwith not so important >> firmwares). A user space helper is able to prioritize the request >> accordingly the use case. > > That seems like a valid problem, but I don't think that UMH adequately > solves it. Sure, loading firmware in the right order avoids a >2sec > delay due to firmware loading, but what happens when you have a slow > USB device that *doesn't* need firmware plugged in to your car's shiny > USB port when you start the car? > > It seems to me that this use case requires explicit control over > device probing and, if that gets added, you get your firmware ordering > for free (just probe the important devices first). Agreed, we could prioritize requests but actually these days we don't queue requests we simply go through them serially. We could add later a priority aspect for async requests but its not clear this is needed yet. As Andy notes most of these requirements could be solved through alternative means. One is loading your module really early so stuffing it into for instance initramfs and if using systmed for instance listing it on a say /etc/modules-load.d/load.conf file, otherwise priority of loading the module is set through the actual default init level associated for the module, if using module_init() that's just device_initcall() and these are all probed serially. If your driver is built-in and needs to be after rootfs_initcall() we don't provide finer granularity for ordering other than just device_initcall() and late_initcall(). In the future we could extend this easily but I really do want a valid use case which we can't handle with today's infrastructure. If this is a special driver and we are certain it must load really early and first than others for *all* system cases when the driver is needed, it may be worth looking into this as a new category. If so let me know and I think we can work on a solution. If you have systemd though I think using something like /etc/modules-load.d/load.conf should suffice. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Nov 29, 2016 at 10:10:56PM +0100, Tom Gundersen wrote: > On Tue, Nov 15, 2016 at 10:28 AM, Johannes Berg > wrote: > > My argument basically goes like this: > > > > First, given good drivers (i.e. using request_firmware_nowait()) > > putting firmware even for a built-in driver into initramfs or not > > should be a system integrator decision. If they don't need the device > > that early, it should be possible for them to delay it. Or, perhaps, if > > the firmware is too big, etc. I'm sure we can all come up with more > > examples of why you'd want to do it one way or another. > > This is how I understood the the situation, but I never quite bought > it. What is wrong with the kernel saying "you must put your module and > your firmware together"? Sure, people may want to do things > differently, but what is the real blocker? 0) Firmware upgrades are possible 1) Some firmware is optional 2) Firmware licenses may often not be GPLv2 compatible 3) Some firmwares may be stupid large (remote-proc) as such neither built-in firmware nor using the firmware in initramfs is reasonable. But note that Johannes' main point was that today only a few properly constructed drivers use async fw request, and furthermore given the lack of a deterministic final rootfs signal his proposal was to address the lack of semantics available between kernel and userspcae available for this with a firmware kobject uevent fallback helper. This fallback kobject uevent helper would not reply firmly against files not found until it knows all rootfs firmware paths are ready. > Fundamentally, it seems to me that if a module needs firmware, it > makes no sense to make the module available before the firmware. I'm > probably missing something though :) You are right but just consider all the above. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Wed, Nov 09, 2016 at 03:21:07AM -0800, Andy Lutomirski wrote: > On Wed, Nov 9, 2016 at 1:13 AM, Daniel Wagner > wrote: > > [CC: added Harald] > > > > As Harald pointed out over a beer yesterday evening, there is at least > > one more reason why UMH isn't obsolete. The ordering of the firmware loading > > might be of important. Say you want to greet the user with a splash screen > > really early on, the graphic card firmware should be loaded first. Also the > > automotive world has this fancy requirement that rear camera must be on the > > screen within 2 seconds. So controlling the firmware loading order is of > > importance (e.g. also do not overcommit the I/O bandwith not so important > > firmwares). A user space helper is able to prioritize the request > > accordingly the use case. > > That seems like a valid problem, but I don't think that UMH adequately > solves it. Sure, loading firmware in the right order avoids a >2sec > delay due to firmware loading, but what happens when you have a slow > USB device that *doesn't* need firmware plugged in to your car's shiny > USB port when you start the car? > > It seems to me that this use case requires explicit control over > device probing and, if that gets added, you get your firmware ordering > for free (just probe the important devices first). In theory this is correct, the problem comes with the flexibility we have created with pivot_root() and friends (another is mount on /lib/firmware) which enables system integrators to pick and choose the "real rootfs" to be a few layers away from the first fs picked up by the kernel. In providing this flexibility we did not envision nor have devised signals to enable a deterministic lookup due to the requirements such lookups might have -- in this case the requirements are that direct fs is ready and kosher all the paths possible for firmware are ready. As you can imagine first race is not only an issue for firmware but a generic issue. The generic race on the fs lookup requires a fs availability event, and addressing fs suspend. I'll note that the race on init is addressed today *only* by the firmware UMH (its UMH is kobject uevent and optionally a custom binary) by using the UMH lock. During a cleanup by Daniel recently I realized it was bogus to use the UMH of the UMH was not used, turns out this would still expose the direct FS lookup to a race though. This begs the question if the UMH lock either be removed / shared with the other kernel UMHs or a generic solution provided for direct fs lookup with some requirements specified. This is all a mess so I've documented each component and issues / ideas we've discussed so far separately, the firmware UMH (which we should probably rebrand to firmware kobject uevent helper to avoid confusion) [0], the real kernel usermode helper [1], the new common kernel file loader [2] [0] https://kernelnewbies.org/KernelProjects/firmware-class-enhancements [1] https://kernelnewbies.org/KernelProjects/usermode-helper-enhancements [2] https://kernelnewbies.org/KernelProjects/common-kernel-loader Luis
powerpc allyesconfig / allmodconfig linux-next next-20160729 - next-20160729 build failures
Are linux-next builds being tested for powerpc with allyesconfig and
allmodconfig ? I have some changes I'm making and while debugging my
build issues I decided to give a clean build a shot and see linux-next
next-20160729 up to next-20160729 all have build failures without my
changes. I get:
/opt/gcc-4.9.0-nolibc/powerpc64-linux/bin/powerpc64-linux-ld:
drivers/built-in.o: .opd is not a regular array of opd entries
MODPOST vmlinux.o
GEN .version
CHK include/generated/compile.h
UPD include/generated/compile.h
CC init/version.o
LD init/built-in.o
/opt/gcc-4.9.0-nolibc/powerpc64-linux/bin/powerpc64-linux-ld:
drivers/built-in.o: .opd is not a regular array of opd entries
drivers/built-in.o: In function `.ipw2100_up':
ipw2100.c:(.text+0x1ff9c90): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.round_jiffies_relative' defined
in .text section in kernel/built-in.o
drivers/built-in.o: In function `.ipw2100_reset_adapter':
ipw2100.c:(.text+0x1ffa500): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `._raw_spin_lock_irqsave' defined
in .spinlock.text section in kernel/built-in.o
drivers/built-in.o: In function `.ipw2100_irq_tasklet':
ipw2100.c:(.text+0x1ffa7cc): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `._raw_spin_lock_irqsave' defined
in .spinlock.text section in kernel/built-in.o
ipw2100.c:(.text+0x1ffb6c8): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.printk' defined in
.text.unlikely section in kernel/built-in.o
ipw2100.c:(.text+0x1ffb6d8): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.printk' defined in
.text.unlikely section in kernel/built-in.o
ipw2100.c:(.text+0x1ffb740): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.printk' defined in
.text.unlikely section in kernel/built-in.o
ipw2100.c:(.text+0x1ffb750): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.printk' defined in
.text.unlikely section in kernel/built-in.o
ipw2100.c:(.text+0x1ffb7ec): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.debug_dma_unmap_page' defined in
.text section in lib/built-in.o
ipw2100.c:(.text+0x1ffb88c): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.__dev_kfree_skb_any' defined in
.text section in net/built-in.o
ipw2100.c:(.text+0x1ffb8b8): relocation truncated to fit:
R_PPC64_REL24 (stub) against symbol `.printk' defined in
.text.unlikely section in kernel/built-in.o
ipw2100.c:(.text+0x1ffb8f4): additional relocation overflows omitted
from the output
scripts/link-vmlinux.sh: line 52: 14580 Segmentation fault (core
dumped) ${LD} ${LDFLAGS} ${LDFLAGS_vmlinux} -o ${2} -T ${lds}
${KBUILD_VMLINUX_INIT} --start-group ${KBUILD_VMLINUX_MAIN}
--end-group ${1}
make: *** [Makefile:952: vmlinux] Error 139
Luis
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
linker tables on powerpc - build issues
I've run into a few compilation issues with linker tables support [0] [1] on only a few architectures: blackfin - compiler issue it seems, I have a work around now in place arm - some alignment issue - still need to iron this out powerpc - issue with including on The issue with powerpc can be replicated easily with the patch below, and compilation fails even on a 'make defconfig' configuration, the issues are recurring include header ordering issues. I've given this some tries to fix but am still a bit bewildered how to best do this without affecting non-powerpc compilations. The patch below replicates the changes in question, it does not include the linker table work at all, it just includes instead of to reduce and provide an example of the issues observed. The list of errors are also pretty endless... so was hoping some power folks might be able to take a glance if possible. If you have any ideas, please let me know. [0] https://lkml.kernel.org/r/1469222687-1600-1-git-send-email-mcg...@kernel.org [1] https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20160729-linker-table-v4-blackfin2 diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h index 9a287e0ac8b1..68e46825b0f8 100644 --- a/arch/powerpc/include/asm/jump_label.h +++ b/arch/powerpc/include/asm/jump_label.h @@ -9,6 +9,7 @@ * as published by the Free Software Foundation; either version * 2 of the License, or (at your option) any later version. */ +#include #ifndef __ASSEMBLY__ #include diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h index 7dc006b58369..929decb62d9c 100644 --- a/arch/powerpc/include/asm/sections.h +++ b/arch/powerpc/include/asm/sections.h @@ -1,11 +1,14 @@ #ifndef _ASM_POWERPC_SECTIONS_H #define _ASM_POWERPC_SECTIONS_H -#ifdef __KERNEL__ +#if defined(__KERNEL__) && !defined(__ASSEMBLER__) && !defined(__ASSEMBLY__) #include #include +#endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) && !defined(__ASSEMBLY__) */ + #include +#if defined(__KERNEL__) && !defined(__ASSEMBLER__) && !defined(__ASSEMBLY__) #ifdef __powerpc64__ extern char __start_interrupts[]; @@ -77,5 +80,5 @@ static inline void *dereference_function_descriptor(void *ptr) #endif -#endif /* __KERNEL__ */ +#endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) && !defined(__ASSEMBLY__) */ #endif /* _ASM_POWERPC_SECTIONS_H */ diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h index af0254c09424..06bceee909da 100644 --- a/include/asm-generic/sections.h +++ b/include/asm-generic/sections.h @@ -1,6 +1,8 @@ #ifndef _ASM_GENERIC_SECTIONS_H_ #define _ASM_GENERIC_SECTIONS_H_ +#if defined(__KERNEL__) && !defined(__ASSEMBLER__) && !defined(__ASSEMBLY__) + /* References to section boundaries */ #include @@ -128,4 +130,6 @@ static inline bool init_section_intersects(void *virt, size_t size) return memory_intersects(__init_begin, __init_end, virt, size); } +#endif /* defined(__KERNEL__) && !defined(__ASSEMBLER__) && !defined(__ASSEMBLY__) */ + #endif /* _ASM_GENERIC_SECTIONS_H_ */ Luis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: powerpc allyesconfig / allmodconfig linux-next next-20160729 - next-20160729 build failures
On Tue, Aug 02, 2016 at 02:58:39PM -0700, Guenter Roeck wrote: > On Tue, Aug 02, 2016 at 01:07:09PM -0700, Luis R. Rodriguez wrote: > > Are linux-next builds being tested for powerpc with allyesconfig and > > allmodconfig ? I have some changes I'm making and while debugging my > > build issues I decided to give a clean build a shot and see linux-next > > next-20160729 up to next-20160729 all have build failures without my > > changes. I get: > > > > /opt/gcc-4.9.0-nolibc/powerpc64-linux/bin/powerpc64-linux-ld: > > drivers/built-in.o: .opd is not a regular array of opd entries > > MODPOST vmlinux.o > > GEN .version > > CHK include/generated/compile.h > > UPD include/generated/compile.h > > CC init/version.o > > LD init/built-in.o > > /opt/gcc-4.9.0-nolibc/powerpc64-linux/bin/powerpc64-linux-ld: > > drivers/built-in.o: .opd is not a regular array of opd entries > > drivers/built-in.o: In function `.ipw2100_up': > > ipw2100.c:(.text+0x1ff9c90): relocation truncated to fit: > > "relocation truncated to fit" errors are typical for ppc:allyesconfig. Thanks for the confirmation. For how long is it known this is broken? Does anyone care and fix these ? Or is this best effort? > allmodconfig should work, though. OK thanks. Luis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe
On Wed, Aug 24, 2016 at 08:55:55AM +0200, Daniel Vetter wrote: > On Fri, Jun 17, 2016 at 12:54 AM, Luis R. Rodriguez wrote: > > Thou shalt not make firmware calls early on init or probe. <-- snip --> > > There are 4 offenders at this time: > > > > mcgrof@ergon ~/linux-next (git::20160609)$ export > > COCCI=scripts/coccinelle/api/request_firmware.cocci > > mcgrof@ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report > > > > drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its > > init routine on line 321. > > drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on > > its probe routine on line 136. > > drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its > > probe routine on line 796. > > drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on > > its probe routine on line 1246. > > Plus all gpu drivers which need firmware. And yes we must load them at > probe Do you have an upstream driver in mind that does this ? Is it on device drier module probe or a DRM subsystem specific probe call of some sort ? > because people are generally pissed when they boot their machine > and the screen goes black. On top of that a lot of people want their > gpu drivers to be built-in, but can't ship the firmware blobs in the > kernel image because gpl. Yep, there's a bit a contradiction there ... Can they use initramfs for this ? Also just curious -- as with other subsystems, is it possible to load a generic driver first, say vesa, and then a more enhanced one later ? I am not saying this is ideal or am I suggesting this, I'd just like to know the feasibility of this. > I think what would work is loading the different subsystems of the > driver in parallel (we already do that largely) Init level stuff is actually pretty synchronous, and in fact both init and probe are called serially. There are a few subsystems which have been doing things a bit differently, but these are exceptions. When you say we already do this largely, can you describe a bit more precisely what you mean ? >, and then if one > firmware blob isn't there yet simply stall that async worker until it > shows up. Is this an existing framework or do you mean if we add something generic to do this async loading of subsystems ? > But the answers I've gotten thus far from request_firmware() > folks (well at least Greg) is don't do that ... Well in this patch set I'm adding myself as a MAINTAINER and I've been extending the firmware API recently to help with a few new features I want, I've been wanting to hear more feedback from other's needs and I had actually not gotten much -- except only recently with the usermode helper and reasons why some folks thought they could not use direct firmware loading from the fs. I'm keen to hear or more use cases and needs specially if they have to do with improving boot time and asynchronous boot. > Is that problem still somewhere on the radar? Not mine. > Atm there's various > wait_for_rootfs hacks out there floating in vendor/product trees. This one I've heard about recently, and I suggested two possible solutions, with a preference to a simply notification of when rootfs is available from userspace. > "Avoid at all costs" sounds like upstream prefers to not care about > android/cros in those case (yes I know most arm socs don't need > firmware, which would make it a problem fo just be a subset of all > devices). In my days of dealing with Android I learned most folks did not frankly care too much about upstream-first model. That means things were reactive. That's a business mind set and that's fine. However for upstream we want what is best and to discuss. I haven't seen proposals so, so long as we just hear of "hacks" that some folks do in vendor/product trees, what can we do ? In so far as async probe is concerned -- that is now upstream. http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html In so far as modules are concerned -- this should work without issue now, and if there is an issue its very likely a bug in the subsystem. As I noted in the post, built-in support requires more love. A simple option for you to test this is to test the two debug patches at the end there and boot. Alternatively inits can just peg the async request for all modules. Should be an easy change, just hadn't had a change to do it yet. Maybe its time. I'm also trying to make more async functionality possible early in boot with dependencies annotated somehow, and have a bit of work to help with this (refer to recent linker tables patches) already which may even be possible to used to facelift our old kernel init levels -- but as I've studied this I've also observed others working on very similar problems, nothing is quite taking a large picture of this and trying to generalize this. Its why I proposed it as a topic for KS. So .. I agree, let's avoid the hacks. Patches welcomed. Luis
Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe
Summoning Felix for the embedded aspect on initramfs below.
Jörg might be interested in the async facilities you speak of as well.
On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
> On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez wrote:
> > On Wed, Aug 24, 2016 at 08:55:55AM +0200, Daniel Vetter wrote:
> >> On Fri, Jun 17, 2016 at 12:54 AM, Luis R. Rodriguez
> >> wrote:
> >> > Thou shalt not make firmware calls early on init or probe.
> >
> > <-- snip -->
> >
> >> > There are 4 offenders at this time:
> >> >
> >> > mcgrof@ergon ~/linux-next (git::20160609)$ export
> >> > COCCI=scripts/coccinelle/api/request_firmware.cocci
> >> > mcgrof@ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report
> >> >
> >> > drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on
> >> > its init routine on line 321.
> >> > drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call
> >> > on its probe routine on line 136.
> >> > drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on
> >> > its probe routine on line 796.
> >> > drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call
> >> > on its probe routine on line 1246.
> >>
> >> Plus all gpu drivers which need firmware. And yes we must load them at
> >> probe
> >
> > Do you have an upstream driver in mind that does this ? Is it on device
> > drier module probe or a DRM subsystem specific probe call of some sort ?
>
> i915 is the one I care about for obvious reasons ;-) It's all from the
> pci device probe function, but nested really deeply.
The above SmPL grammar should capture well nested calls of calls within probe,
so curious why it didn't pick up i915. Let's see.
i915_pci_probe() --> i915_driver_load() -->
i915_load_modeset_init() --> (drivers/gpu/drm/i915/i915_drv.c)
a) intel_csr_ucode_init() (drivers/gpu/drm/i915/intel_csr.c)
...
b) intel_guc_init() (drivers/gpu/drm/i915/intel_guc_loader.c)
The two firmwares come from:
a) intel_csr_ucode_init() --> schedule_work(&dev_priv->csr.work);
csr_load_work_fn() --> request_firmware()
b) intel_guc_init() --> guc_fw_fetch() request_firmware()
---
a) is not dealt with given the grammar does not include scheduled work,
however using work to offload loading firmware seems reasonable here.
b) This should have been picked up, but its not. Upon closer inspection
the grammar currently expects the module init and probe to be on the
same file. Loosening this:
diff --git
a/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
b/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
index cf180c59e042..e19e6d3dfc0f 100644
--- a/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
+++ b/scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci
@@ -49,7 +49,7 @@ identifier init;
module_init(init);
-@ has_probe depends on defines_module_init@
+@ has_probe @
identifier drv_calls, drv_probe;
type bus_driver;
identifier probe_op =~ "(probe)";
@@ -59,7 +59,7 @@ bus_driver drv_calls = {
.probe_op = drv_probe,
};
-@hascall depends on !after_start && defines_module_init@
+@hascall depends on !after_start @
position p;
@@
I get a lot more complaints but still -- i915 b) case is not yet covered:
./drivers/bluetooth/ath3k.c: ERROR: driver call request firmware call on its
probe routine on line 546.
./drivers/bluetooth/bcm203x.c: ERROR: driver call request firmware call on its
probe routine on line 193.
./drivers/bluetooth/bcm203x.c: ERROR: driver call request firmware call on its
probe routine on line 218.
./drivers/bluetooth/bfusb.c: ERROR: driver call request firmware call on its
probe routine on line 655.
./drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its
init routine on line 321.
./drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on
its probe routine on line 136.
./drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on
its probe routine on line 1246.
./sound/soc/codecs/wm2000.c: ERROR: driver call request firmware call on its
probe routine on line 893.
./sound/soc/sh/siu_dai.c: ERROR: driver call request firmware call on its probe
routine on line 747.
./drivers/net/wireless/intersil/orinoco/orinoco_usb.c: ERROR: driver call
request firmware call on its probe routine on line 1661.
./sound/soc/intel/common/sst-acpi.c: ERROR: driver call request firmware call
on its probe routine on line 161.
./drivers/input/touchscreen/goodix.c: ERROR: driver call request firmware call
on its prob
Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe
On Thu, Aug 25, 2016 at 10:10:52PM +0200, Daniel Vetter wrote: > Cutting down since a lot of this is probably better discussed at > ks/lpc. Aside, if you want to check out Chris Wilson's work on our new > depency handling, it's called kfence. > > https://lkml.org/lkml/2016/7/17/37 Thanks more reading.. :) > On Thu, Aug 25, 2016 at 9:41 PM, Luis R. Rodriguez wrote: > >> > So .. I agree, let's avoid the hacks. Patches welcomed. > >> > >> Hm, this is a definite change of tack - back when I discussed this > >> with Greg about 2 ks ago it sounded like "don't do this". The only > >> thing we need is some way to wait for rootfs before we do the > >> request_firmware. Everything else we handle already in the kernel. > > > > OK so lets just get this userspace event done, and we're set. > > Ah great. As long as that special wait-for-rootfs-pls firmware request > is still allowed, i915&gfx folks will be happy. We will call it from > probe though ;-) but all from our own workers. We should strive for this to be transparent to drivers, ie, this safeguard of looking for firmware should be something handled by the kernel as otherwise we're just forcing a race condition given the review in the last thread. Luis
Re: [PATCH v3 2/5] firmware: annotate thou shalt not request fw on init or probe
On Wed, Aug 24, 2016 at 10:17:38AM +0200, Gabriel Paubert wrote: > On Tue, Aug 23, 2016 at 05:45:04PM -0700, mcg...@kernel.org wrote: > > [snip] > > --- > > Documentation/firmware_class/README| 20 > > drivers/base/Kconfig | 2 +- > > .../request_firmware-avoid-init-probe-init.cocci | 130 > > + > > 3 files changed, 151 insertions(+), 1 deletion(-) > > create mode 100644 > > scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci > > > > diff --git a/Documentation/firmware_class/README > > b/Documentation/firmware_class/README > > index cafdca8b3b15..056d1cb9d365 100644 > > --- a/Documentation/firmware_class/README > > +++ b/Documentation/firmware_class/README > > @@ -93,6 +93,26 @@ > > user contexts to request firmware asynchronously, but can't be called > > in atomic contexts. > > > > +Requirements: > > += > > + > > +You should avoid at all costs requesting firmware on both init and probe > > paths > > +of your device driver. Reason for this is the complexity needed to ensure a > > +firmware will be available for a driver early in boot through different > > +build configurations. Consider built-in drivers needing firmware early, or > > +consider a driver assuming it will only get firmware after pivot_root(). > > + > > +Drivers that really need firmware early should use stuff the firmware in > > Minor grammatical nit: s/use// Fixed, thanks. Luis
Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe
On Thu, Aug 25, 2016 at 09:41:33PM +0200, Luis R. Rodriguez wrote: > On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote: > > On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez > > wrote: > > Some users want a fully running gfx stack 2s after power-on. There's > > not even enough time to load an uefi or vga driver first. i915 > > directly initializes the gpu from power-on state on those. > > I see.. thanks. > > > >> I think what would work is loading the different subsystems of the > > >> driver in parallel (we already do that largely) > > > > > > Init level stuff is actually pretty synchronous, and in fact both > > > init and probe are called serially. There are a few subsystems which > > > have been doing things a bit differently, but these are exceptions. > > > > > > When you say we already do this largely, can you describe a bit more > > > precisely what you mean ? > > > > Oh, this isn't subsystems as in linux device/driver model, but > > different parts within the driver. We fire up a bunch of struct work > > to get various bits done asynchronously. > > Thanks for the clarification. <-- snip --> > > One of the proposals which would have worked for us died a while back: > > > > https://lkml.org/lkml/2014/6/15/47 > > Interesting, the problem stated here, which comes from the fact (as clarified > above) the rootfs comes up only *after* we run do_initcalls() as such there > are > possible theoretical races even with request_firmware_nowait() -- as such that > justifies even more the SmPL grammar rule to include even > request_firmware_nowait() > on probe / init as this patch has. > > Anyway yeah that patch has good intentions but its wrong for a slew of > reasons. > The problem is the same as discussed with Bjorn recently. The solution > discussed is that since only userspace knows when the *real* rootfs is ready > (because of slew of reasons) the best we can do is have an event issued by > userspace to inform us of that. OK I have something I think we can build upon for this. Will send RFC. Luis
[RFC] fs: add userspace critical mounts event support
kernel_read_file_from_path() can try to read a file from
the system's filesystem. This is typically done for firmware
for instance, which lives in /lib/firmware. One issue with
this is that the kernel cannot know for sure when the real
final /lib/firmare/ is ready, and even if you use initramfs
drivers are currently initialized *first* prior to the initramfs
kicking off. During init we run through all init calls first
(do_initcalls()) and finally the initramfs is processed via
prepare_namespace():
do_basic_setup() {
...
driver_init();
...
do_initcalls();
...
}
kernel_init_freeable() {
...
do_basic_setup();
...
if (sys_access((const char __user *) ramdisk_execute_command, 0) != 0) {
ramdisk_execute_command = NULL;
prepare_namespace();
}
}
This leaves a possible race between loading drivers and any uses
of kernel_read_file_from_path(). Because pivot_root() can be used,
this allows userspace further possibilities in terms of defining
when a kernel critical filesystem should be ready by.
We define kernel critical filesystems as filesystems which the
kernel needs for kernel_read_file_from_path(). Since only userspace
can know when kernel critical filesystems are mounted and ready,
let userspace notify the kernel of this, and enable a new kernel
configuration which lets the kernel wait for this event before
enabling reads from kernel_read_file_from_path().
A default timeout of 10s is used for now. You can override this
through the kernel-parameters using critical_mounts_timeout_ms=T
where T is in ms. cat /sys/kernel/critical_mounts_timeout_ms the
current system value.
When userspace is ready it can simply:
echo 1 > /sys/kernel/critical_mounts_ready
Signed-off-by: Luis R. Rodriguez
---
Note, this still leaves the puzzle of the fact that initramfs may carry
some firmware, and some drivers may be OK in using firmware from there,
the wait stuff would just get in the way. To address this I think can
perhaps instead check *one* for the file, and if its present immediately
give it back, we'd only resort to the wait in cases of failure.
Another thing -- other than firmware we have:
security/integrity/ima/ima_fs.c:rc = kernel_read_file_from_path(path,
&data, &size, 0, READING_POLICY);
sound/oss/sound_firmware.h: err = kernel_read_file_from_path((char *)fn,
(void **)fp, &size,
What paths are these? So we can document the current uses in the Kconfig
at least.
Thoughts ?
Documentation/kernel-parameters.txt | 6 +++
drivers/base/Kconfig| 48 +++
fs/exec.c | 3 ++
include/linux/fs.h | 8
kernel/ksysfs.c | 77 +
5 files changed, 142 insertions(+)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 8ccacc44622a..1af89faa9fc9 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -849,6 +849,12 @@ bytes respectively. Such letter suffixes can also be
entirely omitted.
It will be ignored when crashkernel=X,high is not used
or memory reserved is below 4G.
+ critical_mounts_timeout_ms=T[KNL] timeout in ms
+ Format:
+ Use this to override the kernel's default timeout for
+ waiting for critical system mount points to become
+ available.
+
cryptomgr.notests
[KNL] Disable crypto self-tests
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 12b4f5551501..21576c0a4898 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -25,6 +25,54 @@ config UEVENT_HELPER_PATH
via /proc/sys/kernel/hotplug or via /sys/kernel/uevent_helper
later at runtime.
+config CRITICAL_MOUNTS_WAIT
+ bool "Enable waiting for critical-filesystems-ready notification"
+ default n
+ help
+ Kernel subsystems and device drivers often need to read files
+ from the filesystem, however in doing this races are possible at
+ bootup -- the subsystem requesting the file might look for it in /
+ early in boot, but if we haven't yet mounted the real root
+ filesystem we'll just tell the subsystem the file is not present and
+ it will fail. Furthermore what path to the filesystem is used varies
+ depending on the subsystem. To help the kernel we provide the option
+ to let the kernel wait for all critical filesystems to mounted and
+ ready before letting the kernel start trying to read files from the
+ systems' filesystem. Since pivot_root() can be used and therefore a
+ system might be configured to change its / filesystem at bootup as
+ many times as it wishes, only userspace can realy know ex
Re: [RFC] fs: add userspace critical mounts event support
On Sat, Sep 03, 2016 at 11:10:02AM -0700, Dmitry Torokhov wrote: > On Sat, Sep 3, 2016 at 11:01 AM, Linus Torvalds > wrote: > > On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov > > wrote: > >> > >> Unfortunately module loading and availability of firmware is very > >> loosely coupled. > > > > The whole "let's add a new magical proc entry To be fair it was using a generic sysfs entry. > > to say that all > > filesystems are mounted" is all about the user space coupling them. > > I was thinking if we kernel could post "conditions" (maybe simple strings) > that it waits for, and userspace could unlock these "conditions". One of them > might be "firmware available". Dmitry, you seem to be suggesting a generic kernel-userspace "registry" for intertwined dependencies that the kernel needs and only userspace can provide, is that right? If so it sounds overly complicated to resolve this. Do we have other uses outside of kernel_read_file_from_path() you had in mind for this? > > I'm just saying that if user space must know about when firmware is > > ready to be loaded anyway, just do it right. Not with some hacky "you > > can now do random things" flag. Note, this isn't just about firmware since we now have a generic API to read files from the kernel, so kernel_read_file_from_path(). Firmware is now just *one* user case. Also a complexity here is this is not just for modules but also for built-in drivers as well. And you want the option to update the files without the driver. The proposed solution provides a generic broad syfs entry for letting userspace inform the kernel when files from the filesystems where it would typically read from (I'm calling them critical filesystems for lack of a better term) can be accessible. Something more specific requires a bit more thought given this is not anymore about just firmware, must also address built-in drivers, and allow for updates. > > But by having user space actually say > > "put this module together with this firmware". Keep in mind this isn't about just firmware anymore, and we have to consider built-in drivers as well. But if we were to just consider firmware... for the sake of following with examples. How would this registry work? We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or something like it to help annotate the the driver was only functional with the firmware, punt things to kmod to deal with the requirements. kmod would only load the driver if the firmware is present as well, otherwise it could just return a new -ENOFIRMWARE and try to defer module loading for a later time, it would load the driver once and if the firmware becomes available... This all seems rather hacky. For built-in drivers.. the vmlinux would have the associated firmware reqs, it would still need a way to let userspace know when such firmware is ready. What kernel <-> userspace API would we want to use for this ? Or as you note we could just prevent such drivers to be built-in. I'd be happy with this, however I don't think the distributions using non-modular kernels will be. Now whatever we come up with recall this isn't just about firmware anymore. Which is why I ended up bundling all these requirements up into one generic "kernel reads file from filesystem" requirement. I can see the syfs approach being considered hacky -- but I gladly welcome an actual alternative suggestion. > > If you just put the two pieces together, then the module "will just work". > > > > And quite frankly, that sounds like a much better maintenance practice > > anyway. If some module doesn't work without the firmware, then dammit, > > the two *should* go together. Putting them in two different places > > would be *INSANE*. > > Quite often it does until it does not. Most of the touch controllers > work just fine until some event (abrupt cutting of power for example) > where nvram gets corrupted and they come up in bootloader mode. It is > just an example. You want to also opt-in for updates, you don't want to require re-building a driver for a firmware fix, for instance. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Sep 06, 2016 at 11:32:05AM -0700, Linus Torvalds wrote: > On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson > wrote: > > > > Linus, I reversed the order of your questions/answers to fit my answer > > better. > > Nobody has actually answered the "why don't we just tie the firmware This is not about firmware anymore. The fact that we were reading files from the filesystem in different ways called for a generic API, that was done by Mimi with the new generic kernel_read_file_from_path(), the fact that there were race concerns with firmware means such races are also in theory possible outside of firmware. > and module together" question. In terms of the firmware, licensing is one reason I'm aware of. Another reason is updates to firmware files may not implicate a driver update -- its pointless to rebuild a kernel if all you need is a firmware update. For both modules and built-in we already have the option to bundle firmware into initramfs, and CONFIG_EXTRA_FIRMWARE, some folks do not want to use these. Bjorn noted a few reasons why. Here's the full list of reasons I've heard why folks shy away from these: o Licensing o You still want the ability to do updates o The size of the files can be huge (remoteproc is talking about 10 MiB files) Do we want to bring the firmware closer together than what we allow for modules? If so it may make sense to use a modpost command to try to bundle module and firmware together, the build system could use the MODULE_FIRMWARE() for that. It does mean you now have a build dependency for linux-firmware on the kernel. Do we want that? > Really. If the driver doesn't work without the firmware, then why the > hell is it separated from it in the first place? This I agree with, although you still have to consider you may want to enable updates for firmware without a driver update. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Sep 06, 2016 at 02:50:51PM -0700, Linus Torvalds wrote: > On Tue, Sep 6, 2016 at 2:11 PM, Bjorn Andersson > wrote: > > On Tue 06 Sep 11:32 PDT 2016, Linus Torvalds wrote: > > > >> On Tue, Sep 6, 2016 at 10:46 AM, Bjorn Andersson > >> Nobody has actually answered the "why don't we just tie the firmware > >> and module together" question. > > > > The answer to this depends on the details of the suggestion; but > > generally there's a much stronger bond between the kernel and the driver > > than between the driver and the firmware in my cases. > > I call BS. > > Let me be very clear. I'm not applying that shit-for-brains stupid > patch, and will not be pulling it unless somebody tricks me into it. Great thanks. Please note that the only reason I proposed this was to get the ball rolling in terms of finding a solution to the problem for why some folks claim they *need* the firmware usermode helper. Granted, upstream we only have 2 explicit users left, I'm told some out-of-tree users still need and use the usermode helper. They claim that without it there is the race between /lib/firmware being ready and driver asking for the firmware. I was told there were quite a bit of out-of-tree hacks to address this without using the usermode helper, the goal of this patch was to create the discussion needed to a proper resolution to this. Given that upon inspection -- I've determined that even if you stuff the firmware into initramfs you may still run into the race (the commit log of this patch explains that) and that using initramfs was the alternative solution we expected folks to use -- the only current deterministic sure way a driver can depend on a firmware and avoid the race is to use CONFIG_EXTRA_FIRMWARE. This is an issue. > Because all these arguments make no sense at all. > > If the driver doesn't work without the firmware, then anybody who > distributes the driver binary without a firmware is just > *fundamentally* doing something insane. Some companies only redistribute firmware binaries to specific entities due to avoid expanding to whom a patent grant is given to. That is, not every company writing drivers or pushing out binary drivers is willing to dish out the firmware as per the terms in linux-firmware. > You may do it for *development* purposes, but doing so for actual *use* would > be entirely pointless. To be fair we haven't been explicit about our requirements for firmware_class and expectations about what we expect for firmware for driver in Linux. This has all been rather loose. Furthermore the race issues we have found in firmware_class have only come about through introspection, and I've been slowly documenting all this tribal knowledge. > See my point? If a distribution is distributing the driver without the > firmware, then what the hell is the point of such a thing? Agreed. > But even if you decide to do that for some odd reason, the patch is > still just stupid. Instead of adding some crazy infrastructure for > "now I've mounted everything", you could equally well just > > (a) make the driver fail the module load if it cannot find a firmware binary Not sure if its clear but: 0) this is not just about firmware anymore 1) there is a race between using kernel_read_file_from_path() and having the filesystem that should be present on be ready; 2) Only *userspace* can know for sure what the real valid filesystem for files read from kernel_read_file_from_path() can be ready, so only userspace can tell the kernel if a read from kernel_read_file_from_path() is at a certain point in time valid. > (b) after user space has mounted everything, just do "insmod -a" > again (or insmod just that driver). I'm happy to document this as the resolution... I have a feeling some folks will not like it. We also have built-in drivers to consider, what do we advise for that? Keep in mind only CONFIG_EXTRA_FIRMWARE is deterministically safe. > See? The point is, this "generic" hacky interface is just stupid. It's > not adding any value. If you add user space "I'm ready now" points > anyway, you might as well make those points do the right thing and > just load the module that is now loadable. This is not about firmware anymore though, and we need to address built-in. > We could mark such "late loading" modules explicitly if people want > to, so that you can automate the whole thing about delaying the > loading in user space. Now *that* could actually help, for instance add another late init call which would be called after initramfs and friends -- perhaps way after prepare_namespace() -- by then we would ensure userspace has all critical fs mounted. The problem though is we'd still need a way for userspace to tell the kernel that all critical fs are mounted as only userspace can know this for sure. When is that done? How would the kernel know? We do have PROBE_PREFER_ASYNCHRONOUS, but that does not provide any guarantees over ready filesystems. > At no point does it mak
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Sep 06, 2016 at 03:28:47PM -0700, Bjorn Andersson wrote: > On Tue 06 Sep 14:52 PDT 2016, Luis R. Rodriguez wrote: > > > We already have MODULE_FIRMWARE(), we could have MODULE_FIRMWARE_REQ() or > > something like it to help annotate the the driver was only functional with > > the > > firmware, punt things to kmod to deal with the requirements. > > That implies that a single driver will only use a single version of the > firmware. Today firmware requests are done manually by the driver, in the future it should be possible to specify just an array of firmwares and on that list specify which firmware is optional, and perhaps if you need at last one. Then you treat optional firmware upgrades as optional -- and only treat fatal conditions as such. Today drivers manage all this on their own. This is something we can later do as we have the flexible firmware API in place, but for now -- you are right. There is no clear way to extract the semantics of what firmware requirements really are in an easy way. > There are cases where we want a single driver to load firmware > depending on e.g. hardware revisions, Dynamic firmware names -- indeed. Good point. > or previous firmware version and > there are cases where we want to load firmware based on requested > use-cases. True. Luis
[PATCH v4 2/5] firmware: annotate thou shalt not request fw on init or probe
Thou shalt not make firmware calls early on init or probe. systemd already ripped support out for the usermode helper a while ago, there are still users that require the usermode helper, however systemd's use of the usermode helper exacerbated a long lasting issue of the fact that we have many drivers that load firmware on init or probe. Independent of the usermode helper loading firmware on init or probe is a bad idea for a few reasons. When the firmware is read directly from the filesystem by the kernel, if the driver is built-in to the kernel the firmware may not yet be available, for such uses one could use initramfs and stuff the firmware inside, or one also use CONFIG_EXTRA_FIRMWARE; however not all distributions use this, as such generally one cannot count on this. There is another corner cases to consider, since we are accessing the firmware directly folks cannot expect new found firmware on a filesystem after we switch off from an initramfs with pivot_root(). Supporting such situations is possible today but fixing it for good is really complex due to the large amount of variablity in the boot up process. Instead just document the expectations properly and add a grammar rule to enable folks to check / validate / police if drivers are using the request firmware API early on init or probe. The SmPL rule used to check for the probe routine is loose and is currently defined through a regexp, that can easily be extended to any other known bus probe routine names. It also uses the new Python iteration support which allows us to backtrack from a request_firmware*() call back to a possible probe or init, iteratively. Without iteration we would only be able to get reports for callers who directly use the request_firmware*() API on the initial probe or init routine. There are 4 offenders at this time: mcgrof@ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci mcgrof@ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321. drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136. drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796. drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246. I checked and verified all these are valid reports. This list also matches the same list as in 20150805, so we have fortunately not gotten worse. Let's keep it that way and also fix these drivers. v3: Clarify that if initramfs or builtin_fw cannot be used the race between driver probe and /lib/firmware/ being ready must be addressed. v2: changes from v1 [0]: o This now supports iteration, this extends our coverage on the report o Update documentation and commit log to accept the fate of not being able to remove the usermode helper. [0] https://lkml.kernel.org/r/1440811107-861-1-git-send-email-mcg...@do-not-panic.com Cc: Alessandro Rubini Cc: Kevin Cernekee Cc: Daniel Vetter Cc: Mimi Zohar Cc: David Woodhouse Cc: Kees Cook Cc: Dmitry Torokhov Cc: Ming Lei Cc: Jonathan Corbet Cc: Julia Lawall Cc: Gilles Muller Cc: Nicolas Palix Cc: Thierry Martinez Cc: Michal Marek Cc: co...@systeme.lip6.fr Cc: Alessandro Rubini Cc: Kevin Cernekee Cc: Greg Kroah-Hartman Cc: Jiri Slaby Cc: linux-ser...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-ser...@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Signed-off-by: Luis R. Rodriguez --- Documentation/firmware_class/README| 22 drivers/base/Kconfig | 2 +- .../request_firmware-avoid-init-probe-init.cocci | 130 + 3 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README index cafdca8b3b15..20aca5901785 100644 --- a/Documentation/firmware_class/README +++ b/Documentation/firmware_class/README @@ -93,6 +93,28 @@ user contexts to request firmware asynchronously, but can't be called in atomic contexts. +Requirements: += + +You should avoid at all costs requesting firmware on both init and probe paths +of your device driver. Reason for this is loading firmware and using it can +often delay boot and we can have races early in boot, between a device's probe +and having the real /lib/firmware ready. + +Drivers that really need firmware early should stuff the firmware in +initramfs or consider using CONFIG_EXTRA_FIRMWARE. Using initramfs is much +more portable to more distributions as not all distributions wish to enable +CONFIG_EXTRA_FIRMWARE. Should a driver require the firmware being built-in +it should depend on CONFIG_EXTRA_FIRMWARE. There is no current annotat
Re: [RFC] fs: add userspace critical mounts event support
On Sat, Sep 24, 2016 at 10:41:46AM -0700, Dmitry Torokhov wrote: > On Fri, Sep 23, 2016 at 6:37 PM, Herbert, Marc wrote: > > On 03/09/2016 11:10, Dmitry Torokhov wrote: > >> I was thinking if we kernel could post > >> "conditions" (maybe simple stings) that it waits for, and userspace > >> could unlock these "conditions". One of them might be "firmware > >> available". > > > > On idea offered by Josh Triplett that seems to overlap with this one > > is to have something similar to the (deprecated) userhelper with > > *per-blob* requests and notifications except for one major difference: > > userspace would not anymore be in charge of *providing* the blob but > > would instead only *signal* when a given blob becomes available and is > > either found or found missing. Then the kernel loads the blob _by > > itself_; unlike the userhelper. No new “critical filesystem” concept > > and a *per-blob basis*, allowing any variation of blob locations > > across any number of initramfs and filesystems. > > > > Really, I do not quite understand why people have issues with usermode > helper/uevents. One reason is you'd have to implement your own cache for suspend/resume. > It used to work reasonably well (if you were using > request_firmware_nowait()), as the kernel would post the request and > then, when userspace was ready[^Hier], uevents would be processed and > firmware would be loaded. We had a timeout of 60(?) seconds by > default, but that would be adjusted as systems needed. The issue with the timeout was kernel developers *assumed* module init and probe were detached, and saying 'thou shall not load firmware on probe' seems actually like a more radical change than just saying 'thou shall load firmware on init'. I'll note that as it stands its the right thing to complain about these users only because we lack the semantics to ensure correctness if used on init or probe. The timeout incurred huge latencies for optional firmwares, and while we had a new API added to avoid the wait on optional firmware, that obviously still leaved the races as possible. We now have async probe which *does* enable some original misconceptions by kernel developers, but by now other issues have also been found on the usermode helper, the cache was one, another one was a recent discusion over the user of the UMH lock with the assumption this was providing a sort of safeguard on early boot use -- it does not, for the same exact reasons why a UMH lock does not suffice to avoid all possible rootfs races. For this later issue refer to a recent discussion in review with Daniel Wagner's patches. > Unfortunately it all broke when udev started insisting [1] on > servicing some uevents in strict sequence, which resulted in boot > stalls. That was not the only issue... another implicit issue was that you are reducing the number of possible supported number of devices Linux supports per module by the timeout, it would depend on the combine time it takes to both init and probe. Some drivers are super complex and even if you *don't* have firmware requirements and say burn the firmware onto a device we found that *probe* alone was taking a long long time on some device drivers -- check out cxgb4 driver, where one device actually ends up loading about 4 subdevices underneath it. Yes that's a mess and the driver needs a major rewrite to address this in a clean way but that takes time. Its no trivial pursuit. The umh timeout then would not be implicated anymore *but* since systemd implemented the timeout in general for kmod loading it did mean system was limiting them Linux drivers and how much devices a driver can support depending on this timeout value. At SUSE we solved this by lifting this timeout for kmod workers for now. A long term goal here, which could help, is also to just detach init and probes, so we give to system what it originally thought. Summary of this all is here: http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html I have some code that starts to enable some of this on systemd/kmod but it still needs some more testing before I post. > Maybe the ultimate answer is to write a firmware loading > daemon that would also listen to netlink events and do properly what > udev refused to be doing? Meh, in the wireless subsystem we devised our own file loader, check CRDA. That worked for us since we needed to optionally enable digital RSA signed file checking, but long term our experience is that this is pointless. So we're going to phase that out in favor of using the firmware API for the file loading of this file, and support then digital signatures on the firmware. I am not sure how/why a firmware loading daemon would be a better idea now. What Marc describes that Josh proposed with signals for userspcae seems more aligned with what we likely need -- but note that since we now use a shared common API for kernel reads from a path via kernel_read_file_from_path() we'd probably want something like a notifier for any ker
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Oct 4, 2016 at 5:12 PM, Linus Torvalds wrote: > On Tue, Oct 4, 2016 at 5:00 PM, Luis R. Rodriguez wrote: >> >> I am not sure how/why a firmware loading daemon would be a better >> idea now. What Marc describes that Josh proposed with signals for >> userspcae seems more aligned with what we likely need > > Quite frankly, I doubt you want a signal. > > You will want to have some way to specify where the firmware files > are. Right now we have "fw_path[]" which is hardcoded except for the > first entry that can be set as a module parameter. But you'd probably > want to expand on that, which implies some /sys or /proc interface. > > And once you do that, wouldn't it make more sense to just make the > "update the firmware path /proc/sys/kernel/fw_path file" make things > re-search for firmware? We can, but re-searching for firmware assumes we cache pending firmware, we currently don't, we just either process sync or async firmware requests. > In other words, the interface has to be something *sensible*. Not some > idiotic ad-hoc "send a signal" (of which that stupid original patch > was just a very odd example). Note that the races are beyond firmware, so all kernel_read_file_from_path() users, as such re-using such old /sys/ interafeces for firmware will not suffice to cover all ground now for the same race for other possible users. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Oct 04, 2016 at 05:32:22PM -0700, Linus Torvalds wrote: > On Tue, Oct 4, 2016 at 5:24 PM, Luis R. Rodriguez wrote: > > > > Note that the races are beyond firmware, so all > > kernel_read_file_from_path() users, as such re-using such old /sys/ > > interafeces for firmware will not suffice to cover all ground now for > > the same race for other possible users. > > Blah blah blah. > > The reason I've hated this whole discussion is that it's full of > "let's re-architect everything", and then it has these horribly warty > interfaces. To be clear, kernel_read_file_from_path() was an agreed upon strategy about 1 year ago at the Linux Security summit as we found different kernel implementations for the same exact task, reading files from the filesystem -- my point here was simply that acknowledging that the race on early init and driver's init / probe for firmware is implicating that the race is *also* possible for the other kernel-read-from-fs points. Its not clear to me what your grudge here is other than the proposal for a solution in this patch is not what we want. > It's classic second-system syndrome. > > Just do *one* thing, and do it well. Don't change anything else. Don't > force existign drivers to use new interfaces. Don't over-architect, > and don't do stupid interfaces. If there is a race for the other users and we want to avoid wrapping a solution for it to the other callers without doing any vetting for correctness then so be it, but to disregard completely seems error-prone. I accept that thinking about such other users may complicate a solution for firmware and if you prefer we just separate the race solution for both that's fine. > If user-space mounts a new filesystem (or just unpacks files from a > tar-file that has firmware images in it, for chissake), that is not > some magical "critical mount event". The whole concept is just stupid. > Is it a "mount event" when the user downloads a new firmware image > from the internet? > > HELL NO. We've gotten passed that the original implementation proposed is not what we want, let's move on. > But what is equally stupid is to then dismiss simple models because > some totally unrelated "beyond firmware" issue. I have not heard back from the other stakeholders using kernel_read_file_from_path() and possible races for them. You seem to suggest to ignore those possible theoretical races in the name of a simple solution for firmware. Fine. > Anything that is "beyond firmware" shouldn't even be discussed, for > chrissake! It has nothing what-so-ever to do with firmware loading. If > there ends up being some common helper functions, and shared code, > that *still* doesn't make it so. My point was to raise the flag of the possible races on the other call sites where we read files directly from the kernel, that's all, if we agree we really don't care for that fine. > Basic rules of thumb: > > (a) don't over-design > > (b) don't have stupid illogical interfaces > > (c) don't conflate different issues just because you think they may > have shared code. > > (4) be consistent. Don't make up new interfaces, and most certainly > do *NOT* dismiss something just because it's what we have done before. > > That's it. OK.. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Tue, Sep 13, 2016 at 09:38:17PM -0500, Rob Landley wrote: > On 09/02/2016 07:20 PM, Luis R. Rodriguez wrote: > > kernel_read_file_from_path() can try to read a file from > > the system's filesystem. This is typically done for firmware > > for instance, which lives in /lib/firmware. One issue with > > this is that the kernel cannot know for sure when the real > > final /lib/firmare/ is ready, and even if you use initramfs > > drivers are currently initialized *first* prior to the initramfs > > kicking off. > > Why? do_initcalls() is called prior to prepare_namespace(), other than that we have no strict rules over where the real rootfs should be, and since we have pivot_root() its up to userspace to decide when/how the real rootfs goes. This and the fact that its also up to userspace to design what files to place in initramfs of further rootfs -- only userspace will know for sure when all firmware for all drivers is really ready. > > During init we run through all init calls first > > (do_initcalls()) and finally the initramfs is processed via > > prepare_namespace(): > > What's the downside of moving initramfs cpio extraction earlier in the boot? That would help users of initrafms, some folks seem to not want to use initramfs, one of such users are that of the large firmwares for remote-proc (Documentation/remoteproc.txt), we're talking about over 200 MiB for some firmware for example. > I did some shuffling around of those code to make initmpfs work, does > anybody know why initramfs extraction _before_ we initialize drivers > would be a bad thing? No, but it seems sensible to me, if its done before do_initcalls() that should resolve the race for initramfs users but -- so long as the drivers that need firmware early are dumped into initramfs. We have no assurances/warnings for this, but we can add such things if we want them. This would not resolve the race for non-initramfs users / pivot_root() changes. Luis
Re: [RFC] fs: add userspace critical mounts event support
On Wed, Oct 05, 2016 at 11:08:06AM -0700, Linus Torvalds wrote: > On Wed, Oct 5, 2016 at 11:00 AM, Luis R. Rodriguez wrote: > > On Tue, Sep 13, 2016 at 09:38:17PM -0500, Rob Landley wrote: > > > >> I did some shuffling around of those code to make initmpfs work, does > >> anybody know why initramfs extraction _before_ we initialize drivers > >> would be a bad thing? > > > > No, but it seems sensible to me, if its done before do_initcalls() > > that should resolve the race for initramfs users > > initramfs should already be set up before drivers are. Actually you are right, the issue would only be for old initrd, for initramfs we populate that via rootfs_initcall(populate_rootfs), so as long as drivers in question use an init level beyond rootfs's we're good there. > Exactly what is it that has trouble right now? It would seem then that the only current stated race possible should then be non-initramfs users. One example if very large firmware for remote-proc, whereby an initramfs is just not practical or desirable. > The gating issue for initramfs is that technically the filesystem > setup needs to be done, which means that it currently ends up being > populated _fairly_ late in the initcall series, but certainly before > drivers. But since initramfs really only needs very limited filesystem > functionality, I assume Rob had few problems with just moving it > earlier. > > Still, what kind of ordering issues did people have? What is it that > needs to load files even before driver init? Some crazy subsystem? No, I think this is just about non-initramfs users now, if we disregard old initrd users. Bjorn, Marc, correct me if I'm wrong, as I think its so far you both who have seemed to run into race issues and have then ended up trying to look for hacks to address this race or considered using the usermode helper (which we're trying to minimize users for). Daniel seems to note a lot of video drivers use firmware on probe as well so there's a potential issue for those users if they don't use initramfs. Luis
[PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe
Thou shalt not make firmware calls early on init or probe. systemd already ripped support out for the usermode helper a while ago, there are still users that require the usermode helper, however systemd's use of the usermode helper exacerbated a long lasting issue of the fact that we have many drivers that load firmware on init or probe. Independent of the usermode helper loading firmware on init or probe is a bad idea for a few reasons. When the firmware is read directly from the filesystem by the kernel, if the driver is built-in to the kernel the firmware may not yet be available, for such uses one could use initramfs and stuff the firmware inside, or one also use CONFIG_EXTRA_FIRMWARE; however not all distributions use this, as such generally one cannot count on this. There is another corner cases to consider, since we are accessing the firmware directly folks cannot expect new found firmware on a filesystem after we switch off from an initramfs with pivot_root(). Supporting such situations is possible today but fixing it for good is really complex due to the large amount of variablity in the boot up process. Instead just document the expectations properly and add a grammar rule to enable folks to check / validate / police if drivers are using the request firmware API early on init or probe. The SmPL rule used to check for the probe routine is loose and is currently defined through a regexp, that can easily be extended to any other known bus probe routine names. It also uses the new Python iteration support which allows us to backtrack from a request_firmware*() call back to a possible probe or init, iteratively. Without iteration we would only be able to get reports for callers who directly use the request_firmware*() API on the initial probe or init routine. There are 4 offenders at this time: mcgrof@ergon ~/linux-next (git::20160609)$ export COCCI=scripts/coccinelle/api/request_firmware.cocci mcgrof@ergon ~/linux-next (git::20160609)$ make coccicheck MODE=report drivers/fmc/fmc-fakedev.c: ERROR: driver call request firmware call on its init routine on line 321. drivers/fmc/fmc-write-eeprom.c: ERROR: driver call request firmware call on its probe routine on line 136. drivers/tty/serial/rp2.c: ERROR: driver call request firmware call on its probe routine on line 796. drivers/tty/serial/ucc_uart.c: ERROR: driver call request firmware call on its probe routine on line 1246. I checked and verified all these are valid reports. This list also matches the same list as in 20150805, so we have fortunately not gotten worse. Let's keep it that way and also fix these drivers. v2: changes from v1 [0]: o This now supports iteration, this extends our coverage on the report o Update documentation and commit log to accept the fate of not being able to remove the usermode helper. [0] https://lkml.kernel.org/r/1440811107-861-1-git-send-email-mcg...@do-not-panic.com Cc: Alessandro Rubini Cc: Kevin Cernekee Cc: Daniel Vetter Cc: Mimi Zohar Cc: David Woodhouse Cc: Kees Cook Cc: Dmitry Torokhov Cc: Ming Lei Cc: Jonathan Corbet Cc: Julia Lawall Cc: Gilles Muller Cc: Nicolas Palix Cc: Thierry Martinez Cc: Michal Marek Cc: co...@systeme.lip6.fr Cc: Alessandro Rubini Cc: Kevin Cernekee Cc: Greg Kroah-Hartman Cc: Jiri Slaby Cc: linux-ser...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-ser...@vger.kernel.org Cc: linuxppc-dev@lists.ozlabs.org Signed-off-by: Luis R. Rodriguez --- Documentation/firmware_class/README| 20 drivers/base/Kconfig | 2 +- .../request_firmware-avoid-init-probe-init.cocci | 130 + 3 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 scripts/coccinelle/api/request_firmware-avoid-init-probe-init.cocci diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README index cafdca8b3b15..056d1cb9d365 100644 --- a/Documentation/firmware_class/README +++ b/Documentation/firmware_class/README @@ -93,6 +93,26 @@ user contexts to request firmware asynchronously, but can't be called in atomic contexts. +Requirements: += + +You should avoid at all costs requesting firmware on both init and probe paths +of your device driver. Reason for this is the complexity needed to ensure a +firmware will be available for a driver early in boot through different +build configurations. Consider built-in drivers needing firmware early, or +consider a driver assuming it will only get firmware after pivot_root(). + +Drivers that really need firmware early should use stuff the firmware in +initramfs or consider using CONFIG_EXTRA_FIRMWARE. Using initramfs is much +more portable to more distributions as not all distributions wish to enable +CONFIG_EXTRA_FIRMWARE. Should a driver require the firmware being built-in +it should depend on CONFIG_EXTRA_FIRMWARE. There is no current annotation for +requiring a firmware on initramfs.
Re: [PATCH 00/15] change default_llseek action
On Wed, Sep 15, 2010 at 2:39 AM, Stephen Rothwell wrote: > Hi Arnd, > > On Tue, 14 Sep 2010 22:22:28 +0200 Arnd Bergmann wrote: >> >> Stephen, please add >> git+ssh://master.kernel.org/pub/scm/linux/kernel/git/arnd/bkl.git llseek > > Added from today. > > Thanks for adding your subsystem tree as a participant of linux-next. As > you may know, this is not a judgment of your code. The purpose of > linux-next is for integration testing and to lower the impact of > conflicts between subsystems in the next merge window. > > You will need to ensure that the patches/commits in your tree/series have > been: > * submitted under GPL v2 (or later) and include the Contributor's > Signed-off-by, I should note this should say the code should be GPL-compatible, it doesn't need to be GPLv2 (or later). Furthermore the contributors of the subsystem respect the individual licenses of the files through the Developers Certificate of Origin, which tells the developers what the meaning of Signed-off-by means. Luis ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] pegasos_eth.c: Fix compile error over MV643XX_ defines
This commit made an incorrect assumption:
--
Author: Lennert Buytenhek <[EMAIL PROTECTED]>
Date: Fri Oct 19 04:10:10 2007 +0200
mv643xx_eth: Move ethernet register definitions into private header
Move the mv643xx's ethernet-related register definitions from
include/linux/mv643xx.h into drivers/net/mv643xx_eth.h, since
they aren't of any use outside the ethernet driver.
Signed-off-by: Lennert Buytenhek <[EMAIL PROTECTED]>
Acked-by: Tzachi Perelstein <[EMAIL PROTECTED]>
Signed-off-by: Dale Farnsworth <[EMAIL PROTECTED]>
--
arch/powerpc/platforms/chrp/pegasos_eth.c made use of a 3 defines there.
[EMAIL PROTECTED]:~/devel/wireless-2.6$ git-describe
v2.6.24-rc1-138-g0119130
This patch fixes this by internalizing 3 defines onto pegasos which are
simply no longer available elsewhere. Without this your compile will fail
whenever you enable 'Common Hardware Reference Platform (CHRP) based machines',
(CONFIG_PPC_CHRP) as the Makefile for chrp adds it always:
obj-y += setup.o time.o pegasos_eth.o pci.o
If these defines are platform specific then they should later just be added
back to include/linux/mv643xx.h.
Compile error:
CC arch/powerpc/platforms/chrp/pegasos_eth.o
arch/powerpc/platforms/chrp/pegasos_eth.c: In function 'Enable_SRAM':
arch/powerpc/platforms/chrp/pegasos_eth.c:150: error: 'MV643XX_ETH_BAR_4'
undeclared (first use in this function)
arch/powerpc/platforms/chrp/pegasos_eth.c:150: error: (Each undeclared
identifier is reported only once
arch/powerpc/platforms/chrp/pegasos_eth.c:150: error: for each function it
appears in.)
arch/powerpc/platforms/chrp/pegasos_eth.c:152: error:
'MV643XX_ETH_SIZE_REG_4' undeclared (first use in this function)
arch/powerpc/platforms/chrp/pegasos_eth.c:154: error:
'MV643XX_ETH_BASE_ADDR_ENABLE_REG' undeclared (first use in this function)
make[2]: *** [arch/powerpc/platforms/chrp/pegasos_eth.o] Error 1
make[1]: *** [arch/powerpc/platforms/chrp] Error 2
make: *** [arch/powerpc/platforms] Error 2
Signed-off-by: Luis R. Rodriguez <[EMAIL PROTECTED]>
---
arch/powerpc/platforms/chrp/pegasos_eth.c | 11 +++
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/platforms/chrp/pegasos_eth.c
b/arch/powerpc/platforms/chrp/pegasos_eth.c
index 5bcc58d..1fc9e8c 100644
--- a/arch/powerpc/platforms/chrp/pegasos_eth.c
+++ b/arch/powerpc/platforms/chrp/pegasos_eth.c
@@ -24,6 +24,9 @@
#define PEGASOS2_SRAM_BASE_ETH0(PEGASOS2_SRAM_BASE)
#define PEGASOS2_SRAM_BASE_ETH1
(PEGASOS2_SRAM_BASE_ETH0 + (PEGASOS2_SRAM_SIZE / 2) )
+#define PEGASOS2_ETH_BAR_4 0x2220
+#define PEGASOS2_ETH_SIZE_REG_40x2224
+#define PEGASOS2_ETH_BASE_ADDR_ENABLE_REG 0x2290
#define PEGASOS2_SRAM_RXRING_SIZE (PEGASOS2_SRAM_SIZE/4)
#define PEGASOS2_SRAM_TXRING_SIZE (PEGASOS2_SRAM_SIZE/4)
@@ -147,13 +150,13 @@ static int Enable_SRAM(void)
ALong = 0x02;
ALong |= PEGASOS2_SRAM_BASE & 0x;
- MV_WRITE(MV643XX_ETH_BAR_4, ALong);
+ MV_WRITE(PEGASOS2_ETH_BAR_4, ALong);
- MV_WRITE(MV643XX_ETH_SIZE_REG_4, (PEGASOS2_SRAM_SIZE-1) & 0x);
+ MV_WRITE(PEGASOS2_ETH_SIZE_REG_4, (PEGASOS2_SRAM_SIZE-1) & 0x);
- MV_READ(MV643XX_ETH_BASE_ADDR_ENABLE_REG, ALong);
+ MV_READ(PEGASOS2_ETH_BASE_ADDR_ENABLE_REG, ALong);
ALong &= ~(1 << 4);
- MV_WRITE(MV643XX_ETH_BASE_ADDR_ENABLE_REG, ALong);
+ MV_WRITE(PEGASOS2_ETH_BASE_ADDR_ENABLE_REG, ALong);
#ifdef BE_VERBOSE
printk("Pegasos II/Marvell MV64361: register unmapped\n");
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pegasos_eth.c: Fix compile error over MV643XX_ defines
On 10/29/07, Dale Farnsworth <[EMAIL PROTECTED]> wrote: > On Mon, Oct 29, 2007 at 05:27:29PM -0400, Luis R. Rodriguez wrote: > > This commit made an incorrect assumption: > > -- > > Author: Lennert Buytenhek <[EMAIL PROTECTED]> > > Date: Fri Oct 19 04:10:10 2007 +0200 > > > > mv643xx_eth: Move ethernet register definitions into private header > > > > Move the mv643xx's ethernet-related register definitions from > > include/linux/mv643xx.h into drivers/net/mv643xx_eth.h, since > > they aren't of any use outside the ethernet driver. > > > > Signed-off-by: Lennert Buytenhek <[EMAIL PROTECTED]> > > Acked-by: Tzachi Perelstein <[EMAIL PROTECTED]> > > Signed-off-by: Dale Farnsworth <[EMAIL PROTECTED]> > > -- > > > > arch/powerpc/platforms/chrp/pegasos_eth.c made use of a 3 defines there. > > > > [EMAIL PROTECTED]:~/devel/wireless-2.6$ git-describe > > > > v2.6.24-rc1-138-g0119130 > > > > This patch fixes this by internalizing 3 defines onto pegasos which are > > simply no longer available elsewhere. Without this your compile will fail > > That compile failure was fixed in commit > 30e69bf4cce16d4c2dcfd629a60fcd8e1aba9fee by Al Viro. > > However, as I examine that commit, I see that it defines offsets from > the eth block in the chip, rather than the full chip registeri block > as the Pegasos 2 code expects. So, I think it fixes the compile > failure, but leaves the Pegasos 2 broken. > > Luis, do you have Pegasos 2 hardware? Can you (or anyone) verify that > the following patch is needed for the Pegasos 2? Nope, sorry. Luis ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
