Re: [PATCH 1/3] vme_user: Ensure driver compiles after VME bridges

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/11/13 20:58, Aaron Sierra wrote:
> ----- Original Message -----
>> From: "Martyn Welch" <martyn.welch@xxxxxx>
>> Subject: Re: [PATCH 1/3] vme_user: Ensure driver compiles after VME bridges
>>
>> On 05/11/13 17:53, Aaron Sierra wrote:
>>> Martyn,
>>> Can you please elaborate on why you feel it is not a fix? For instance,
>>> will this type of solution not be accepted upstream? Is this solution not
>>> complete enough? Do you feel that it doesn't resolve any issue?
>>>
>>> -Aaron
>>
>> The vme_user driver is in the staging tree as it is not in a fit state to
>> incorporate into the main kernel tree. As far as I can see, the below patch
>> just causes the driver to be built at a subtly different time, from a subtly
>> different location (outside of the staging tree). To be honest, I'm not sure
>> how this is fixing anything.
>>
>> Martyn
> 
> Martyn,
> You are correct about the function of this patch, however, I know that
> it resolves a real problem despite its inconsequential appearance.
> 
> Using a 3.10.6 kernel, if I compile vme, vme_tsi148, and vme_user as
> modules, then attempting to insert vme_user into the kernel before the
> vme subsystem driver has been loaded results in the module failing to
> load with the following (this is good):
> 
> vme_user: Unknown symbol vme_master_read (err 0)
> vme_user: Unknown symbol vme_master_write (err 0)
> vme_user: Unknown symbol vme_free_consistent (err 0)
> vme_user: Unknown symbol vme_register_driver (err 0)
> vme_user: Unknown symbol vme_irq_generate (err 0)
> vme_user: Unknown symbol vme_alloc_consistent (err 0)
> vme_user: Unknown symbol vme_slave_request (err 0)
> vme_user: Unknown symbol vme_master_free (err 0)
> vme_user: Unknown symbol vme_master_request (err 0)
> vme_user: Unknown symbol vme_slave_free (err 0)
> vme_user: Unknown symbol vme_slave_set (err 0)
> vme_user: Unknown symbol vme_master_get (err 0)
> vme_user: Unknown symbol vme_slave_get (err 0)
> vme_user: Unknown symbol vme_master_set (err 0)
> vme_user: Unknown symbol vme_unregister_driver (err 0)
> vme_user: Unknown symbol vme_get_size (err 0)
> 
> This "Unknown symbol" error forces modules to be loaded in this order
> at the very least; vme -> vme_user.
> 
> However, when these modules are compiled into the kernel, unfortunate
> things start to take place. This is the order that the three VME
> drivers are compiled when using an unpatched kernel:
> 
>   CC      drivers/staging/vme/devices/vme_user.o
>   LD      drivers/staging/vme/devices/built-in.o
>   LD      drivers/staging/vme/built-in.o
>   LD      drivers/staging/built-in.o
>   CC      drivers/vme/vme.o
>   CC      drivers/vme/bridges/vme_tsi148.o
>   LD      drivers/vme/bridges/built-in.o
>   LD      drivers/vme/built-in.o
>   LD      drivers/built-in.o
> 
> Note that this is not only the order that the drivers are compiled, but
> also the order that they are initialized. Also notice that since all
> three drivers are compiled into the kernel, there are no "Unknown
> symbols" to prevent the vme_user driver from initializing as if the VME
> bus subsystem has already been initialized. Therefore the following load
> order occurs; vme_user -> vme -> vme_tsi148.
> 
> This is the result of the unpatched initialization order:
> 
> [snip]
> hidraw: raw HID events driver (C) Jiri Kosina
> usbcore: registered new interface driver usbhid
> usbhid: USB HID core driver
> vme_user: VME User Space Access Driver
> ------------[ cut here ]------------
> kernel BUG at drivers/base/driver.c:169!
> invalid opcode: 0000 [#1] SMP
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.10.6-xes_r3-00018-g300fcda-dirty #112
> Hardware name: Extreme Engineering Solutions, Inc. (X-ES) XCalibur4331/XCalibur4331, BIOS 1-2.21.1 12/10/2012
> task: ffff880232888000 ti: ffff880232884000 task.ti: ffff880232884000
> RIP: 0010:[<ffffffff812ba23a>]  [<ffffffff812ba23a>] driver_register+0x1d/0x106
> RSP: 0000:ffff880232885e18  EFLAGS: 00010246
> RAX: ffffffff8184ef20 RBX: ffffffff8184ee68 RCX: 0000000000000026
> RDX: 0000000000000023 RSI: 0000000000000020 RDI: ffffffff8184ee68
> RBP: ffff880232885e48 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: ffffffff81d5f420 R12: ffffffff8184ee30
> R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff8184eee0
> FS:  0000000000000000(0000) GS:ffff88023bc80000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000000000000 CR3: 000000000180c000 CR4: 00000000000007e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Stack:
>  0000000000000001 ffffffff818a576d ffffffff8184ee30 0000000000000000
>  0000000000000000 ffffffff8184eee0 ffff880232885eb8 ffffffff813a63aa
>  ffff880232885eb8 ffffffff814bc12b ffff880232885eb8 0000002000000008
> Call Trace:
>  [<ffffffff818a576d>] ? staging_init+0x8/0x8
>  [<ffffffff813a63aa>] vme_register_driver+0x4c/0x23b
>  [<ffffffff814bc12b>] ? printk+0x48/0x4a
>  [<ffffffff818a576d>] ? staging_init+0x8/0x8
>  [<ffffffff818a5790>] vme_user_init+0x23/0x25
>  [<ffffffff81000243>] do_one_initcall+0x7b/0x10c
>  [<ffffffff81880d5e>] kernel_init_freeable+0x101/0x190
>  [<ffffffff818806b0>] ? do_early_param+0x86/0x86
>  [<ffffffff814b7e4b>] ? rest_init+0x6f/0x6f
> usb 1-1: new high-speed USB device number 2 using ehci-pci
>  [<ffffffff814b7e54>] kernel_init+0x9/0xd1
>  [<ffffffff814c276c>] ret_from_fork+0x7c/0xb0
>  [<ffffffff814b7e4b>] ? rest_init+0x6f/0x6f
> Code: 9e f0 ff 48 8b 83 90 00 00 00 5a 5b 5d c3 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 89 fb 41 50 48 8b 47 08 48 83 78 78 00 75 02 <0f> 0b 48 83 78 40 00 74 07 48 83 7f 38 00 75 1c 48 83 78 48 00
> RIP  [<ffffffff812ba23a>] driver_register+0x1d/0x106
>  RSP <ffff880232885e18>
> ---[ end trace 0ccf2d5a87725d2a ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [snip]
> 

Ouch - yes I see this as well.

> After applying the patch that I submitted, the three VME drivers are
> compiled as such:
> 
>   CC      drivers/vme/vme.o
>   CC      drivers/vme/../staging/vme/devices/vme_user.o
>   LD      drivers/vme/../staging/vme/devices/built-in.o
>   LD      drivers/vme/../staging/vme/built-in.o
>   CC      drivers/vme/bridges/vme_tsi148.o
>   LD      drivers/vme/bridges/built-in.o
>   LD      drivers/vme/built-in.o
> 
> You should see that this forces the vme_user driver to compile after the
> VME subsystem driver and therefore initialize in the necessary order
> (and without the kernel panic):
> 

The problem with this approach is that it only solves it for the narrow case
of the contents of drivers/staging/vme/. If we have a GPIO driver, which
resides in drivers/gpio/, that happens to be a VME device and is compiled into
the kernel, this will also happen. The answer seems to be that we shouldn't be
treating the core subsystem code as a module - it needs to be treated like
other subsystems. By doing something like:

diff --git a/drivers/vme/Kconfig b/drivers/vme/Kconfig
index c5c2246..a6a6f95 100644
--- a/drivers/vme/Kconfig
+++ b/drivers/vme/Kconfig
@@ -3,7 +3,7 @@
 #

 menuconfig VME_BUS
-       tristate "VME bridge support"
+       bool "VME bridge support"
        depends on PCI
        ---help---
          If you say Y here you get support for the VME bridge Framework.
diff --git a/drivers/vme/vme.c b/drivers/vme/vme.c
index f6856b4..c4ad939 100644
--- a/drivers/vme/vme.c
+++ b/drivers/vme/vme.c
@@ -1512,9 +1512,5 @@ static void __exit vme_exit(void)
        bus_unregister(&vme_bus_type);
 }

-MODULE_DESCRIPTION("VME bridge driver framework");
-MODULE_AUTHOR("Martyn Welch <martyn.welch@xxxxxx");
-MODULE_LICENSE("GPL");
-
-module_init(vme_init);
+subsys_initcall(vme_init);
 module_exit(vme_exit);

Martyn

> [snip]
> hidraw: raw HID events driver (C) Jiri Kosina
> usbcore: registered new interface driver usbhid
> usbhid: USB HID core driver
> vme_tsi148 0000:04:04.0: Board is the VME system controller
> vme_tsi148 0000:04:04.0: VME geographical address is 1
> vme_tsi148 0000:04:04.0: VME Write and flush and error check is disabled
> vme_tsi148 0000:04:04.0: CR/CSR Offset: 1
> vme_tsi148 0000:04:04.0: Enabling CR/CSR space
> vme_user: VME User Space Access Driver
> [snip]
> 
> Thanks for reconsidering.
>
> -Aaron
> _______________________________________________
> devel mailing list
> devel@xxxxxxxxxxxxxxxxxxxxxx
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> 


-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@xxxxxx                  | VAT:GB 927559189
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel




[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux