diff mbox

wf111: Add mdev dependency

Message ID 1439277422-26111-1-git-send-email-cdhmanning@gmail.com
State Rejected
Headers show

Commit Message

Charles Manning Aug. 11, 2015, 7:17 a.m. UTC
wf111 really needs mdev

Signed-off-by: Charles Manning <cdhmanning@gmail.com>
---
 package/wf111/Config.in | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Baruch Siach Aug. 11, 2015, 11:40 a.m. UTC | #1
HI Charles,

On Tue, Aug 11, 2015 at 07:17:02PM +1200, Charles Manning wrote:
> wf111 really needs mdev

Please explain why.

baruch
Charles Manning Aug. 11, 2015, 8:11 p.m. UTC | #2
Are you saying I should explain here or in the commit notice?

Actually udev would work too.

The wf111 driver set uses hotpug to perform the firmware patching
(loading .xbv files).

From what I have experienced, fw patching is required when you first
fire up a new module and when you change modes (sta only vs ap).






On Tue, Aug 11, 2015 at 11:40 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> HI Charles,
>
> On Tue, Aug 11, 2015 at 07:17:02PM +1200, Charles Manning wrote:
>> wf111 really needs mdev
>
> Please explain why.
>
> baruch
>
> --
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
Baruch Siach Aug. 12, 2015, 4:38 a.m. UTC | #3
Hi Charles,

On Wed, Aug 12, 2015 at 08:11:08AM +1200, Charles Manning wrote:
> Are you saying I should explain here or in the commit notice?

In commit log.

> Actually udev would work too.

Recent kernels gained the ability to load firmware directly without any help 
form userspace, so this is probably another option.

> The wf111 driver set uses hotpug to perform the firmware patching
> (loading .xbv files).
> 
> From what I have experienced, fw patching is required when you first
> fire up a new module and when you change modes (sta only vs ap).

Since there are different ways to load the firmware we generally don't make 
firmware or driver packages build depend on any specific firmware loading 
mechanism.

baruch

> On Tue, Aug 11, 2015 at 11:40 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > On Tue, Aug 11, 2015 at 07:17:02PM +1200, Charles Manning wrote:
> >> wf111 really needs mdev
> >
> > Please explain why.
Charles Manning Aug. 12, 2015, 5:16 a.m. UTC | #4
>> On Tue, Aug 11, 2015 at 11:40 PM, Baruch Siach <baruch@tkos.co.il> wrote:
>> > On Tue, Aug 11, 2015 at 07:17:02PM +1200, Charles Manning wrote:
>> >> wf111 really needs mdev
>> >
>> > Please explain why.
>On Wed, Aug 12, 2015 at 4:38 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Charles,
>
> On Wed, Aug 12, 2015 at 08:11:08AM +1200, Charles Manning wrote:
>> Are you saying I should explain here or in the commit notice?
>
> In commit log.

Ok, I will resubmit...

>
>> Actually udev would work too.
>
> Recent kernels gained the ability to load firmware directly without any help
> form userspace, so this is probably another option.

Well not without rewriting the wf111 driver and the associated
prebuilt "helper" binaries it comes with...

The wf111 buildroot package unwraps a tarball from Bluegiga, does an
out of tree build of the kernel modules then copies the modules +
binaries into the file system tree.

This isn't "pure" or anything like that, but it gets the job done....

This is why there is quite a bit of "funkiness" to the wf111 package.

>
>> The wf111 driver set uses hotpug to perform the firmware patching
>> (loading .xbv files).
>>
>> From what I have experienced, fw patching is required when you first
>> fire up a new module and when you change modes (sta only vs ap).
>
> Since there are different ways to load the firmware we generally don't make
> firmware or driver packages build depend on any specific firmware loading
> mechanism.

In general you are right, but I think this is a special case.

wf111 driver + associated files  is built out of tree by a special package.

That package already has other dependencies that are nothing to do
with the kernel. eg.BR2_TOOLCHAIN_USES_GLIBC because of those prebuilt
binaries.

The prebuilt binaries (and therefore the whole wf111 package) does not
result in a functioning wf111 subsystem unless there is udev or mdev
(I've only tested with mdev)., therefore let's be helpful and add the
dependency.

Regards

Charles
Baruch Siach Aug. 12, 2015, 5:38 a.m. UTC | #5
Hi Charles,

On Wed, Aug 12, 2015 at 05:16:35PM +1200, Charles Manning wrote:
> >> On Tue, Aug 11, 2015 at 11:40 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> >> > On Tue, Aug 11, 2015 at 07:17:02PM +1200, Charles Manning wrote:
> >> >> wf111 really needs mdev
> >> Actually udev would work too.
> >
> > Recent kernels gained the ability to load firmware directly without any help
> > form userspace, so this is probably another option.
> 
> Well not without rewriting the wf111 driver and the associated
> prebuilt "helper" binaries it comes with...
> 
> The wf111 buildroot package unwraps a tarball from Bluegiga, does an
> out of tree build of the kernel modules then copies the modules +
> binaries into the file system tree.
> 
> This isn't "pure" or anything like that, but it gets the job done....
> 
> This is why there is quite a bit of "funkiness" to the wf111 package.

All that should be mentioned in the commit log.

> >> The wf111 driver set uses hotpug to perform the firmware patching
> >> (loading .xbv files).
> >>
> >> From what I have experienced, fw patching is required when you first
> >> fire up a new module and when you change modes (sta only vs ap).
> >
> > Since there are different ways to load the firmware we generally don't make
> > firmware or driver packages build depend on any specific firmware loading
> > mechanism.
> 
> In general you are right, but I think this is a special case.
> 
> wf111 driver + associated files  is built out of tree by a special package.
> 
> That package already has other dependencies that are nothing to do
> with the kernel. eg.BR2_TOOLCHAIN_USES_GLIBC because of those prebuilt
> binaries.
> 
> The prebuilt binaries (and therefore the whole wf111 package) does not
> result in a functioning wf111 subsystem unless there is udev or mdev
> (I've only tested with mdev)., therefore let's be helpful and add the
> dependency.

I'm still not convinced. We use dependencies to express build time or run time 
dependencies. I think the glibc dependency is appropriate, but a hotplug 
mechanism is too broad.

Let's see what others think.

baruch
Charles Manning Aug. 12, 2015, 5:56 a.m. UTC | #6
On Wed, Aug 12, 2015 at 5:38 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Charles,
>
> On Wed, Aug 12, 2015 at 05:16:35PM +1200, Charles Manning wrote:
>> >> On Tue, Aug 11, 2015 at 11:40 PM, Baruch Siach <baruch@tkos.co.il> wrote:
>> >> > On Tue, Aug 11, 2015 at 07:17:02PM +1200, Charles Manning wrote:
>> >> >> wf111 really needs mdev
>> >> Actually udev would work too.
>> >
>> > Recent kernels gained the ability to load firmware directly without any help
>> > form userspace, so this is probably another option.
>>
>> Well not without rewriting the wf111 driver and the associated
>> prebuilt "helper" binaries it comes with...
>>
>> The wf111 buildroot package unwraps a tarball from Bluegiga, does an
>> out of tree build of the kernel modules then copies the modules +
>> binaries into the file system tree.
>>
>> This isn't "pure" or anything like that, but it gets the job done....
>>
>> This is why there is quite a bit of "funkiness" to the wf111 package.
>
> All that should be mentioned in the commit log.
>
>> >> The wf111 driver set uses hotpug to perform the firmware patching
>> >> (loading .xbv files).
>> >>
>> >> From what I have experienced, fw patching is required when you first
>> >> fire up a new module and when you change modes (sta only vs ap).
>> >
>> > Since there are different ways to load the firmware we generally don't make
>> > firmware or driver packages build depend on any specific firmware loading
>> > mechanism.
>>
>> In general you are right, but I think this is a special case.
>>
>> wf111 driver + associated files  is built out of tree by a special package.
>>
>> That package already has other dependencies that are nothing to do
>> with the kernel. eg.BR2_TOOLCHAIN_USES_GLIBC because of those prebuilt
>> binaries.
>>
>> The prebuilt binaries (and therefore the whole wf111 package) does not
>> result in a functioning wf111 subsystem unless there is udev or mdev
>> (I've only tested with mdev)., therefore let's be helpful and add the
>> dependency.
>
> I'm still not convinced. We use dependencies to express build time or run time
> dependencies. I think the glibc dependency is appropriate, but a hotplug
> mechanism is too broad.

I am a bit confused. If a binary needs hotplug as a runtime
dependency, then surely we put that in as a dependency. If not then
what's the purpose of dependencies?

The reason I discovered this dependency is that I had a system which
would not load a new fw and I got the WF111 trouble shooting guide.
(Login needed to access pdf). That says on page 14 Step 4.2 that if
you do not have udev or mdev operational then the fw upgrade will not
work.

I then added BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV and the module worked.

Perhaps there is a better choice for determining this than
BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_xxx

We can just leave the dependency out, like it is now, and it won't
work, but that seems rather pointless.

>
> Let's see what others think.

I am hoping Antoine will weigh in as he put the wf111 package together
and will have the most familiarity with this.

Regards

Charles
Floris Bos Aug. 12, 2015, 10:29 a.m. UTC | #7
On 08/11/2015 09:17 AM, Charles Manning wrote:
> wf111 really needs mdev

Problem I have seen with mdev in its default configuration is that it 
doesn't just handle firmware load requests, but also takes it upon 
itself to create/delete the device nodes (even when that already is 
being handled by devtmpfs), and does a bad job with that.

It doesn't preserve sequencing by default, and events are processed 
concurrently, and get out-of-order.

Becomes a problem if your software for example does anything that causes 
the kernel to re-read the partition table of your storage.
In that case the kernel sends events to delete all devices nodes of the 
storage device, and add them again, right after each other.
However depending on your luck, they can get processed in reverse order:

==
mdev[598]: 9.191594 ACTION:add SUBSYSTEM:block DEVNAME:mmcblk0p6 
DEVPATH:/devices/platform/soc/3f300000.mmc/mmc_host/mmc0/mmc0:1234/block/mmcblk0/mmcblk0p6
mdev[598]: dev 179,6
[...]
mdev[593]: 9.192168 ACTION:remove SUBSYSTEM:block DEVNAME:mmcblk0p6 
DEVPATH:/devices/platform/soc/3f300000.mmc/mmc_host/mmc0/mmc0:1234/block/mmcblk0/mmcblk0p6
mdev[593]: rule matched, line -1
mdev[593]: unlink: mmcblk0p6
==

Add is processed before remove, with the end result that /dev/mmcblk0p6 
goes missing...


So please don't pull in mdev automatically just for firmware loading, 
because it does more than just that, and can have serious side effects.


Yours sincerely,

Floris Bos
Charles Manning Aug. 12, 2015, 8:49 p.m. UTC | #8
On Wed, Aug 12, 2015 at 10:29 PM, Floris Bos <bos@je-eigen-domein.nl> wrote:
> On 08/11/2015 09:17 AM, Charles Manning wrote:
>>
>> wf111 really needs mdev
>
>
> Problem I have seen with mdev in its default configuration is that it
> doesn't just handle firmware load requests, but also takes it upon itself to
> create/delete the device nodes (even when that already is being handled by
> devtmpfs), and does a bad job with that.
>
> It doesn't preserve sequencing by default, and events are processed
> concurrently, and get out-of-order.
>
> Becomes a problem if your software for example does anything that causes the
> kernel to re-read the partition table of your storage.
> In that case the kernel sends events to delete all devices nodes of the
> storage device, and add them again, right after each other.
> However depending on your luck, they can get processed in reverse order:
>
> ==
> mdev[598]: 9.191594 ACTION:add SUBSYSTEM:block DEVNAME:mmcblk0p6
> DEVPATH:/devices/platform/soc/3f300000.mmc/mmc_host/mmc0/mmc0:1234/block/mmcblk0/mmcblk0p6
> mdev[598]: dev 179,6
> [...]
> mdev[593]: 9.192168 ACTION:remove SUBSYSTEM:block DEVNAME:mmcblk0p6
> DEVPATH:/devices/platform/soc/3f300000.mmc/mmc_host/mmc0/mmc0:1234/block/mmcblk0/mmcblk0p6
> mdev[593]: rule matched, line -1
> mdev[593]: unlink: mmcblk0p6
> ==
>
> Add is processed before remove, with the end result that /dev/mmcblk0p6 goes
> missing...
>
>
> So please don't pull in mdev automatically just for firmware loading,
> because it does more than just that, and can have serious side effects.

Ok, now I'm stumped. People are telling me "don't use mdev", but
nobody is providing a viable alternative.

The situation is this:
* I did not write wf111, it is not as if I am submitting a new package.
* We have an existing package wf111.
* The existing package does not work (ie. does not do fw loading)
unless there is mdev or udev. Without fw loading wf111 is broken.
* The bluegiga code includes prebuilt binaries and driver code and
comes from Bluegiga as a tarball.

If you build a system with wf111 and you do not have udev/mdev then
the system will not work. After some debugging, you will find the
reason is lack of udev/mdev, turn that on and it will work.

It seems the only way forward here is one of:
* Convince Bluegiga to change their architecture/drivers to not use
mdev/udev and release a new tarball.
* Fix mdev so it works properly.
* Run mdev in a way that just does the fw loading requests and not
anything else.

Will changing to eudev fix any of these issues?

I will try to get someone from Bluegiga involved in this thread so
that they might be convinced to fix this at a driver level.

Regards

Charles
Floris Bos Aug. 12, 2015, 9:47 p.m. UTC | #9
On 08/12/2015 10:49 PM, Charles Manning wrote:
> On Wed, Aug 12, 2015 at 10:29 PM, Floris Bos <bos@je-eigen-domein.nl> wrote:
>> On 08/11/2015 09:17 AM, Charles Manning wrote:
>>> wf111 really needs mdev
>>
>> Problem I have seen with mdev in its default configuration is that it
>> doesn't just handle firmware load requests, but also takes it upon itself to
>> create/delete the device nodes (even when that already is being handled by
>> devtmpfs), and does a bad job with that.
>>
>> It doesn't preserve sequencing by default, and events are processed
>> concurrently, and get out-of-order.
>>
>> Becomes a problem if your software for example does anything that causes the
>> kernel to re-read the partition table of your storage.
>> In that case the kernel sends events to delete all devices nodes of the
>> storage device, and add them again, right after each other.
>> However depending on your luck, they can get processed in reverse order:
>>
>> ==
>> mdev[598]: 9.191594 ACTION:add SUBSYSTEM:block DEVNAME:mmcblk0p6
>> DEVPATH:/devices/platform/soc/3f300000.mmc/mmc_host/mmc0/mmc0:1234/block/mmcblk0/mmcblk0p6
>> mdev[598]: dev 179,6
>> [...]
>> mdev[593]: 9.192168 ACTION:remove SUBSYSTEM:block DEVNAME:mmcblk0p6
>> DEVPATH:/devices/platform/soc/3f300000.mmc/mmc_host/mmc0/mmc0:1234/block/mmcblk0/mmcblk0p6
>> mdev[593]: rule matched, line -1
>> mdev[593]: unlink: mmcblk0p6
>> ==
>>
>> Add is processed before remove, with the end result that /dev/mmcblk0p6 goes
>> missing...
>>
>>
>> So please don't pull in mdev automatically just for firmware loading,
>> because it does more than just that, and can have serious side effects.
> Ok, now I'm stumped. People are telling me "don't use mdev", but
> nobody is providing a viable alternative.
>
> The situation is this:
> * I did not write wf111, it is not as if I am submitting a new package.
> * We have an existing package wf111.
> * The existing package does not work (ie. does not do fw loading)
> unless there is mdev or udev. Without fw loading wf111 is broken.

Suggest you double check there isn't a newer kernel version available 
for your target device.
Pretty sure newer kernels can also load the firmware straight from 
/lib/firmware without userspace helper.


Yours sincerely,

Floris Bos
Floris Bos Aug. 12, 2015, 10:54 p.m. UTC | #10
On 08/12/2015 07:16 AM, Charles Manning wrote:
>>> Actually udev would work too.
>> Recent kernels gained the ability to load firmware directly without any help
>> form userspace, so this is probably another option.
> Well not without rewriting the wf111 driver and the associated
> prebuilt "helper" binaries it comes with...

mdev will not launch any of those "prebuild helper binaries" 
automatically, unless you added any explicit rules to tell it to in 
mdev.conf
If you could make it work by just enabling mdev, it is more likely mdev 
only helped by serving files from /lib/firmware to the module, which -as 
mentioned- is not strictly necessary in recent kernels.


Yours sincerely,

Floris Bos
Thomas Petazzoni Aug. 18, 2015, 8:28 p.m. UTC | #11
Dear Charles Manning,

On Thu, 13 Aug 2015 08:49:26 +1200, Charles Manning wrote:

> It seems the only way forward here is one of:
> * Convince Bluegiga to change their architecture/drivers to not use
> mdev/udev and release a new tarball.
> * Fix mdev so it works properly.
> * Run mdev in a way that just does the fw loading requests and not
> anything else.

 * Use udev
 * Use the in-kernel firmware loader

There is currently no way in Buildroot to say "this package requires a
firmware loading capability". And actually implementing that is not so
easy since the in-kernel firmware loader is only available since (say)
3.7, and Buildroot does not know at configure time the kernel version
you're using, nor whether the in-kernel firmware loader is enabled or
not.

So I believe the right thing to do here is simply to update the wf111
Config.in help text to indicate that this package requires a firmware
loading mechanism to be enabled.

My colleague Antoine Ténart (who wrote the package) is successfully
using wf111, probably with udev (I haven't checked). I've Cc'ed Antoine.

Best regards,

Thomas
diff mbox

Patch

diff --git a/package/wf111/Config.in b/package/wf111/Config.in
index d2ba440..c8a7537 100644
--- a/package/wf111/Config.in
+++ b/package/wf111/Config.in
@@ -2,6 +2,7 @@  config BR2_PACKAGE_WF111
 	bool "wf111"
 	depends on BR2_LINUX_KERNEL
 	depends on BR2_ARM_CPU_ARMV5 || BR2_ARM_CPU_ARMV7A || BR2_i386
+	 depends on BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV
 	# Binary tools are distributed alongside the driver, and are
 	# dynamically linked against the glibc.
 	depends on BR2_TOOLCHAIN_USES_GLIBC
@@ -30,7 +31,7 @@  config BR2_PACKAGE_WF111_TARBALL_PATH
 
 endif
 
-comment "wf111 needs an (e)glibc toolchain"
+comment "wf111 needs an (e)glibc toolchain and mdev"
 	depends on BR2_LINUX_KERNEL
 	depends on BR2_ARM_CPU_ARMV5 || BR2_ARM_CPU_ARMV7A || BR2_i386
-	depends on !BR2_TOOLCHAIN_USES_GLIBC
+	depends on !BR2_TOOLCHAIN_USES_GLIBC || !BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV