Re: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Tue, Nov 18, 2014 at 06:31:38AM +, Kweh, Hock Leong wrote:
-Original Message-
From: Matt Fleming [mailto:m...@console-pimps.org]
Sent: Monday, November 17, 2014 11:12 PM
- Only doing module unload is required to be aware of this synchronization
- Ensuring the call back does not fall into unloaded code which may
cause
undefined behavior.
- Ensuring the put_device() module_put() code have finished in
firmware_class.c
function request_firmware_work_func() before the device is
unregistered
and module unloaded happen.
Shouldn't the existing module_{put,get}() and {put,get}_device() calls
provide all the necessary synchronisation?
Module unload should not be possible while other code is using the
module (and the module refcnt has been incremented accordindly).
Right?
--
Matt Fleming, Intel Open Source Technology Center
Hi Matt,
Yes, you are right. If the module refcount is not zero, you will get error
message and returned while you do rmmod. But I strongly believe if we
have the capability in our code to take care of it by doing synchronization,
we should take care of it in case people are doing rmmod -f. Don't
you think so?
If you do 'rmmod -f' you get to keep all of the broken pieces of your
kernel, no need to try to help out with crazy things like that.
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 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Thu, 13 Nov, at 02:51:28AM, Kweh, Hock Leong wrote:
Hi everyone,
First of all, I would like to apologize if my commit message gives you guys
an impression
that to use request_firmware_abort(), you guys MUST do the synchronization on
your own.
But the fact is, it is not a MUST. Below will provide more detail.
Regarding this synchronization topic, I would like to open a discussion to
get a
better approach to handle this problem. Before jumping onto the design, I
would
like to give a background of why I am doing in this way.
- Only doing module unload is required to be aware of this synchronization
- Ensuring the call back does not fall into unloaded code which may
cause
undefined behavior.
- Ensuring the put_device() module_put() code have finished in
firmware_class.c
function request_firmware_work_func() before the device is
unregistered
and module unloaded happen.
Shouldn't the existing module_{put,get}() and {put,get}_device() calls
provide all the necessary synchronisation?
Module unload should not be possible while other code is using the
module (and the module refcnt has been incremented accordindly).
Right?
--
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
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 hock.leong.k...@intel.com 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 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Mon, 03 Nov 2014, Kweh Hock Leong wrote: 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); This is _so_ asking for people to get it wrong, it is not funny :-( -- One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie. -- The Silicon Valley Tarot Henrique Holschuh -- 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 1/3] firmware loader: Introduce new API - request_firmware_abort()
On Mon, 2014-11-03 at 08:15 -0200, Henrique de Moraes Holschuh wrote: On Mon, 03 Nov 2014, Kweh Hock Leong wrote: 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); This is _so_ asking for people to get it wrong, it is not funny :-( Yeah. How about we make request_firmware_abort() synchronous so that drivers don't have to perform these reference counting games? -- 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
