Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Mon, Nov 03, 2014 at 11:07:08AM +0800, Kweh Hock Leong wrote: > From: "Kweh, Hock Leong" > > Besides aborting through user helper interface, a new API > request_firmware_abort() allows kernel driver module to abort the > request_firmware() / request_firmware_nowait() when they are no > longer needed. It is useful for cancelling an outstanding firmware > load if initiated from inside a module where that module is in the > process of being unloaded, since the unload function may not have > a handle to the struct firmware_buf. > > Note for people who use request_firmware_nowait(): > You are required to do your own synchronization after you call > request_firmware_abort() in order to continue unloading the > module. The example synchronization code shows below: > > while (THIS_MODULE && module_refcount(THIS_MODULE)) > msleep(20); As others have pointed out, this isn't ok, and is totally racy and should never end up in a driver. Never mess with THIS_MODULE from within the same module, otherwise it's racy and broken code. So can you please rework this to not require this? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] firmware loader: fix hung task warning dump
On Mon, Nov 03, 2014 at 11:07:09AM +0800, Kweh Hock Leong wrote: > From: "Kweh, Hock Leong" > > When using request_firmware_nowait() with FW_ACTION_NOHOTPLUG param to > expose user helper interface, if the user do not react immediately, after > 120 seconds there will be a hung task warning message dumped as below: > > [ 3000.784235] INFO: task kworker/0:0:8259 blocked for more than 120 seconds. > [ 3000.791281] Tainted: GE 3.16.0-rc1-yocto-standard #41 > [ 3000.798082] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables > this message. > [ 3000.806072] kworker/0:0 D cd0075c8 0 8259 2 0x > [ 3000.812765] Workqueue: events request_firmware_work_func > [ 3000.818253] cd375e18 0046 000e cd0075c8 00f0 cd40ea00 > cd375fec 1b883e89 > [ 3000.826374] 026b cd40ea00 8000 0001 cd0075c8 > cd375de4 c119917f > [ 3000.834492] cd563360 cd375df4 c119a0ab cd563360 cd375e24 > c119a1d6 > [ 3000.842616] Call Trace: > [ 3000.845252] [] ? kernfs_next_descendant_post+0x3f/0x50 > [ 3000.851543] [] ? kernfs_activate+0x6b/0xc0 > [ 3000.856790] [] ? kernfs_add_one+0xd6/0x130 > [ 3000.862047] [] schedule+0x22/0x60 > [ 3000.866548] [] schedule_timeout+0x175/0x1d0 > [ 3000.871887] [] ? __kernfs_create_file+0x71/0xa0 > [ 3000.877574] [] ? sysfs_add_file_mode_ns+0xaa/0x180 > [ 3000.883533] [] wait_for_completion+0x6f/0xb0 > [ 3000.888961] [] ? wake_up_process+0x40/0x40 > [ 3000.894219] [] _request_firmware+0x750/0x9f0 > [ 3000.899666] [] ? n_tty_receive_buf2+0x1f/0x30 > [ 3000.905200] [] request_firmware_work_func+0x22/0x50 > [ 3000.911235] [] process_one_work+0x122/0x380 > [ 3000.916571] [] worker_thread+0xf9/0x470 > [ 3000.921555] [] ? create_and_start_worker+0x50/0x50 > [ 3000.927497] [] ? create_and_start_worker+0x50/0x50 > [ 3000.933448] [] kthread+0x9f/0xc0 > [ 3000.937850] [] ret_from_kernel_thread+0x20/0x30 > [ 3000.943548] [] ? kthread_worker_fn+0x100/0x100 > > This patch change the wait_for_completion() function call to > wait_for_completion_interruptible() function call for solving the issue. > > Cc: Matt Fleming > Signed-off-by: Kweh, Hock Leong Acked-by: Greg Kroah-Hartman -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Nov 8, 2014 5:05 AM, "Matt Fleming" wrote: > > On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote: > > > > Am I missing something here? The current proposal is missing the > > success/failure part, unless you count the loaded count (in a > > different sysfs directory) as a useful interface for that. > > As Wilson pointed out, you only get the ability to make meaningful > success/failure declarations once you've performed the reboot. > > I know of no firmware that will hot-patch itself when you call > UpdateCapsule(). A reboot is always required. Certainly that's the way > Windows will work from what I've read, which means that for x86 it's > pretty much set in stone. I dunno. If nothing else, efi_capsule_update can fail due to ENOMEM. > > Which means there's only so much info you can return to userspace once > you've handed the blob to the firmware. I don't see a huge problem with > printing things in kernel buffer, since that's how other > firmware-related things work today. I think the kernel log is fine. But if the code is going to report success / failure to userspace at all, shouldn't that indication be reliable? TBH, I find this discussion very strange. In summary: me: This API is really awkward. others: But it's using the subsystem that it should be using. me: Then fix the subsystem? others: The subsystem the correct choice. me: But the API is still really awkward, and, by the way, it probably has at least two races that user code could hit. And, by the way, the sample script written by the author of the patches is subject to *both* races most likely and therefore won't work reliably. you: Common use cases (e.g. Windows-style uses, perhaps) don't need the features that are racy anyway. My only response is that (a) something else might want the full functionality and (b) Wilson's actual example script exercises the racy code. I think I'm done reviewing these patches. I'll probably grumble at the result the first time I actually try to install an EFI capsule, though. --Andy P.S. What happens when a strange UEFI BIOS really wants two capsules, and the second one will brick the machine if the first one isn't there, and the first one failed to load but no one noticed because there's no useful error handling? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
On Tue, 04 Nov, at 08:35:40AM, Andy Lutomirski wrote: > > Am I missing something here? The current proposal is missing the > success/failure part, unless you count the loaded count (in a > different sysfs directory) as a useful interface for that. As Wilson pointed out, you only get the ability to make meaningful success/failure declarations once you've performed the reboot. I know of no firmware that will hot-patch itself when you call UpdateCapsule(). A reboot is always required. Certainly that's the way Windows will work from what I've read, which means that for x86 it's pretty much set in stone. Which means there's only so much info you can return to userspace once you've handed the blob to the firmware. I don't see a huge problem with printing things in kernel buffer, since that's how other firmware-related things work today. -- Matt Fleming, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
