Make vcio (mailbox) driver available on ARCH_BCM2835 #949
Conversation
|
How does this compare to bcm2835 mbox driver? |
|
bcm2835-mailbox can only be used with Device Tree. AFAICT the API is also centered around DT making it difficult to patch locally for use with regular platform devices. The vcio driver has it's own API which these depending drivers use: This is just transitional work. When we move to ARCH_BCM2835, maybe we could write a module that wraps bcm2835-mailbox providing the legacy API. Unless most of these drivers have been pushed upstream first. |
|
Okay. I think dropping non-DT support is a matter of when rather than if. No objections in principle, but I can't test this until after the weekend. |
|
AFAIK only the firmware driver uses the mailbox, because the mailbox has only one channel. [PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver |
|
@pelwell any objections? |
bcm2835-mmc: Add option to disable some delays bcm2835-mmc: Add option to disable MMC_QUIRK_BLK_NO_CMD23 bcm2835-mmc: Default to disabling MMC_QUIRK_BLK_NO_CMD23
|
I was just about to read the commits again, but the 4.0.2 update has meant the PR needs rebasing. |
|
You can still read the commits here. @notro can you rebase? |
The vcio driver hardcodes these resources, so this is the first step in correcting this. Spell out the device name so we don't have to include mach/vcio.h, since this header file will eventually go away when the driver is later moved to drivers/mailbox. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
No need to panic the kernel if the video driver fails. Just print a message and return an error. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
checkpatch.pl errors and warnings:
Many whitespace related issues in some form or another.
Consider using <linux/uaccess.h> instead of <asm/uaccess.h>.
braces {} are not necessary for single statement blocks.
Use pr_* instead of printk.
Do not initialise statics to 0 or NULL.
Avoid CamelCase.
sizeof size should be sizeof(size).
break is not useful after a goto or return.
struct file_operations should normally be const
Possible unnecessary 'out of memory' message
Comparison to NULL could be written "!res"
quoted string split across lines.
This has not been adressed:
WARNING: consider using a completion
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
The config reference SERIAL_BCM_MBOX_CONSOLE does not exist, so remove the whole clause as it will always be false. Remove includes that are not needed. Add <linux/fs.h>. Also sort include headers alphabetically, since this is now the preferred coding style. Remove vc_mailbox->dev since it is not used. Compact some comments to one line. Remove superfluous comments. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
chrdev is created in the probe function, but teared down in module exit. Move chrdev teardown to happen on device removal. Also add missing mbox_dev disabling. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
No need to use if/else clauses on error when return can be used directly. Also test for errors first if possible. This is done to enhance readability. bcm_vcio_probe() is not touched, it will be reworked later. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Move some macros that are only used by the driver: MAJOR_NUM IOCTL_MBOX_PROPERTY DEVICE_FILE_NAME This one becomes superfluous: BCM_VCIO_DRIVER_NAME Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
No need to keep pointers to the sub registers. Only store the base address. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
It is not common practice to print messages from a module init function that only register a driver. Remove obsolete module alias. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Use device resources instead of hardcoding them. Use devm_* functions where possible. Merge dev_mbox_register() with probe function. Add Device Tree support. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Copy the arch vcio.c driver to drivers/mailbox. This is done to make it available on ARCH_BCM2835. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Load ordering of modules are determined by the initcall used. If it's the same initcall level, makefile ordering decides. Now that the mailbox driver is being moved, it's no longer placed before the power driver by the linker. So use a later initcall level to let the mailbox driver load first. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Use bcm2708-vcio instead of the arch version. Change affected drivers to use linux/platform_data/mailbox-bcm2708.h Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Remove the arch vcio.c driver and header file. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Add bcm2708-vcio to Device Tree and don't add the platform device when booting in DT mode. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Enable the mailbox driver. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Add mailbox to Device Tree. There are no kernel users yet, but it's available to userspace. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
|
I have rebased. |
|
I want to test the mailbox behaviour to see if I've understood the FIFO status logic, but apart from that and the ATOMIC allocation I have no objections to merging. |
No need to do atomic allocation in a context that can sleep. Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
|
Added commit for GFP_ATOMIC issue. |
With the VC reader blocked and the ARM writing, MAIL0_STA reads empty permanently while MAIL1_STA goes from empty (0x40000000) to non-empty (0x00000001-0x00000007) to full (0x80000008). Suggested-by: Phil Elwell <phil@raspberrypi.org> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
|
Added status register fix |
|
Can you squash (at least last two, and any more you think are appropriate). |
|
I have tried to follow mainline philosophy here, one commit per change. Well I skipped that on the checkpatch commit, that one would have to be split in several commits in mainline. So I don't think squashing any of these is really best practice. |
|
Not sure I agree. The last two commits are bug fixes to files introduced by the PR. I see no reason why the bugs and the fixes need to be preserved forever. The problem is having large numbers of non-upstreamed commits does make rebasing onto new kernel versions harder, both when trying to find commits, and when you get conflicts. Nothing worse than having a file that has a long history of typo/fix/revert commits that later gets an upstream update that means you have to fix conflicts repeatedly when rebasing. I think at least the last two commits should be squashed. |
Both are present in the current vcio driver, so they don't fix a problem introduced by this PR. But I'll squash the last two. |
|
Okay, I get your point. Technically the last two commits could be a separate fix (e.g. preceding this PR). |
Yes. And technically it would have been best that Phil did the status register fix, because he knows the subject matter. I wouldn't know how to do the test he did. This is somewhat similar to the challenge I face when I now try to get Gellert's slave_sg DMA patch into mainline, I know very little about the driver making it impossible to answer technical questions.
Yes, I have seen that you sometimes squash on the next cycle.
Then I'll happily drop the squashing. |
|
Thank you, notro. I had expected this all to get squashed into one patch. Had I know it would remain a series of patches then I would have happily written the status fix patch myself, and it would have looked exactly like yours. |
Make vcio (mailbox) driver available on ARCH_BCM2835
Lockdep complains that __mmdrop is not safe from the softirq context: ================================= [ INFO: inconsistent lock state ] 4.6.0-oomfortification2-00011-geeb3eadeab96-dirty raspberrypi#949 Tainted: G W --------------------------------- inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage. swapper/1/0 [HC0[0]:SC1[1]:HE1:SE0] takes: (pgd_lock){+.?...}, at: pgd_free+0x19/0x6b {SOFTIRQ-ON-W} state was registered at: __lock_acquire+0xa06/0x196e lock_acquire+0x139/0x1e1 _raw_spin_lock+0x32/0x41 __change_page_attr_set_clr+0x2a5/0xacd change_page_attr_set_clr+0x16f/0x32c set_memory_nx+0x37/0x3a free_init_pages+0x9e/0xc7 alternative_instructions+0xa2/0xb3 check_bugs+0xe/0x2d start_kernel+0x3ce/0x3ea x86_64_start_reservations+0x2a/0x2c x86_64_start_kernel+0x17a/0x18d irq event stamp: 105916 hardirqs last enabled at (105916): free_hot_cold_page+0x37e/0x390 hardirqs last disabled at (105915): free_hot_cold_page+0x2c1/0x390 softirqs last enabled at (105878): _local_bh_enable+0x42/0x44 softirqs last disabled at (105879): irq_exit+0x6f/0xd1 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(pgd_lock); <Interrupt> lock(pgd_lock); *** DEADLOCK *** 1 lock held by swapper/1/0: #0: (rcu_callback){......}, at: rcu_process_callbacks+0x390/0x800 stack backtrace: CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W 4.6.0-oomfortification2-00011-geeb3eadeab96-dirty raspberrypi#949 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014 Call Trace: <IRQ> print_usage_bug.part.25+0x259/0x268 mark_lock+0x381/0x567 __lock_acquire+0x993/0x196e lock_acquire+0x139/0x1e1 _raw_spin_lock+0x32/0x41 pgd_free+0x19/0x6b __mmdrop+0x25/0xb9 __put_task_struct+0x103/0x11e delayed_put_task_struct+0x157/0x15e rcu_process_callbacks+0x660/0x800 __do_softirq+0x1ec/0x4d5 irq_exit+0x6f/0xd1 smp_apic_timer_interrupt+0x42/0x4d apic_timer_interrupt+0x8e/0xa0 <EOI> arch_cpu_idle+0xf/0x11 default_idle_call+0x32/0x34 cpu_startup_entry+0x20c/0x399 start_secondary+0xfe/0x101 More over commit a79e53d ("x86/mm: Fix pgd_lock deadlock") was explicit about pgd_lock not to be called from the irq context. This means that __mmdrop called from free_signal_struct has to be postponed to a user context. We already have a similar mechanism for mmput_async so we can use it here as well. This is safe because mm_count is pinned by mm_users. This fixes bug introduced by "oom: keep mm of the killed task available" Link: http://lkml.kernel.org/r/1472119394-11342-5-git-send-email-mhocko@kernel.org Signed-off-by: Michal Hocko <mhocko@suse.com> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Cc: Oleg Nesterov <oleg@redhat.com> Cc: David Rientjes <rientjes@google.com> Cc: Vladimir Davydov <vdavydov@parallels.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Move the mailbox driver to drivers/mailbox to make it available on ARCH_BCM2835.
Tested on Pi1 and Pi2 with and without Device Tree (BCM270x).
Mailbox on ARCH_BCM2835 is tested with a hacked up bcm2708_fb and from userspace: