* [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
@ 2012-10-05 17:05 Mark Brown
2012-10-08 22:56 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2012-10-05 17:05 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-kernel, Mark Brown
vmalloc() will fail (very loudly) if we try to allocate zero bytes to
read a zero byte file. Instead report that we successfully read in all
zero bytes.
It's not immediately obvious to me that this is better than returning an
error but it seems better to punt the decision about that to the caller
on the off chance that it's sensible.
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
drivers/base/firmware_class.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8154145..a14eb92 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -62,6 +62,11 @@ static bool fw_read_file_contents(struct file *file, struct firmware *fw)
char *buf;
size = fw_file_size(file);
+ if (size == 0) {
+ fw->data = NULL;
+ fw->size = 0;
+ return true;
+ }
if (size < 0)
return false;
buf = vmalloc(size);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-05 17:05 [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc() Mark Brown
@ 2012-10-08 22:56 ` Ming Lei
2012-10-09 4:19 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2012-10-08 22:56 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel
On Sat, Oct 6, 2012 at 1:05 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> vmalloc() will fail (very loudly) if we try to allocate zero bytes to
> read a zero byte file. Instead report that we successfully read in all
> zero bytes.
>
> It's not immediately obvious to me that this is better than returning an
> error but it seems better to punt the decision about that to the caller
> on the off chance that it's sensible.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
> drivers/base/firmware_class.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8154145..a14eb92 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -62,6 +62,11 @@ static bool fw_read_file_contents(struct file *file, struct firmware *fw)
> char *buf;
>
> size = fw_file_size(file);
> + if (size == 0) {
> + fw->data = NULL;
> + fw->size = 0;
> + return true;
> + }
Considered that zero-length firmware image doesn't make sense for drivers
(callers), maybe it is a insane firmware image, so how about treating it as a
failure?
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-08 22:56 ` Ming Lei
@ 2012-10-09 4:19 ` Mark Brown
2012-10-09 7:05 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2012-10-09 4:19 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-kernel
On Tue, Oct 09, 2012 at 06:56:02AM +0800, Ming Lei wrote:
> Considered that zero-length firmware image doesn't make sense for drivers
> (callers), maybe it is a insane firmware image, so how about treating it as a
> failure?
It seems better to punt that decision to callers - for example, the case
I ran into this with was a driver that was using a zero length firmware
to say that it didn't want to load an optional image but also didn't
want to have to time out if that was the case. That doesn't seem
unreasonable to me, and drivers already have to validate that what
they're getting makes sense.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 4:19 ` Mark Brown
@ 2012-10-09 7:05 ` Ming Lei
2012-10-09 7:13 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2012-10-09 7:05 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel
On Tue, Oct 9, 2012 at 12:19 PM, Mark Brown
> It seems better to punt that decision to callers - for example, the case
In fact, -ENOENT is returned to caller for non-direct loading situation,
see_request_firmware_load().
I understand drivers(caller) may be cheated if a zero-length firmware
image is obtained. In normal situation, one firmware image should
include something, instead of nothing, :-)
> I ran into this with was a driver that was using a zero length firmware
> to say that it didn't want to load an optional image but also didn't
> want to have to time out if that was the case. That doesn't seem
If so, I am wondering why the driver has to call request_firmware()?
Looks just bypassing request_firmware() is fine for the driver, doesn't it?
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 7:05 ` Ming Lei
@ 2012-10-09 7:13 ` Mark Brown
2012-10-09 7:34 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2012-10-09 7:13 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-kernel
On Tue, Oct 09, 2012 at 03:05:30PM +0800, Ming Lei wrote:
> On Tue, Oct 9, 2012 at 12:19 PM, Mark Brown
> > It seems better to punt that decision to callers - for example, the case
> In fact, -ENOENT is returned to caller for non-direct loading situation,
> see_request_firmware_load().
> I understand drivers(caller) may be cheated if a zero-length firmware
> image is obtained. In normal situation, one firmware image should
> include something, instead of nothing, :-)
Hrm, that didn't seem to be happening for me - the firmware load
completed successfully. Have to check how that happened.
> > I ran into this with was a driver that was using a zero length firmware
> > to say that it didn't want to load an optional image but also didn't
> > want to have to time out if that was the case. That doesn't seem
> If so, I am wondering why the driver has to call request_firmware()?
> Looks just bypassing request_firmware() is fine for the driver, doesn't it?
A driver has no way to tell if the firmware is there or not without
asking for it.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 7:13 ` Mark Brown
@ 2012-10-09 7:34 ` Ming Lei
2012-10-09 7:52 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2012-10-09 7:34 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel
On Tue, Oct 9, 2012 at 3:13 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>> If so, I am wondering why the driver has to call request_firmware()?
>> Looks just bypassing request_firmware() is fine for the driver, doesn't it?
>
> A driver has no way to tell if the firmware is there or not without
> asking for it.
Yes, I agree, and my question is only on what you mentioned:
"it didn't want to load an optional image"
maybe I misunderstood the above, never mind, :-)
So one driver should suppose the firmware is there, and the
firmware shouldn't be zero length, because the driver always
expects getting some bytes by calling request_firmware().
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 7:34 ` Ming Lei
@ 2012-10-09 7:52 ` Mark Brown
2012-10-09 12:02 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2012-10-09 7:52 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-kernel
On Tue, Oct 09, 2012 at 03:34:52PM +0800, Ming Lei wrote:
> Yes, I agree, and my question is only on what you mentioned:
> "it didn't want to load an optional image"
> maybe I misunderstood the above, never mind, :-)
> So one driver should suppose the firmware is there, and the
> firmware shouldn't be zero length, because the driver always
> expects getting some bytes by calling request_firmware().
The point is that there's some firmware which the driver wants to load
but where it's happy to continue if the user didn't provide one and
doesn't want to introduce needless delays.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 7:52 ` Mark Brown
@ 2012-10-09 12:02 ` Ming Lei
2012-10-09 12:36 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2012-10-09 12:02 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel
On Tue, Oct 9, 2012 at 3:52 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> The point is that there's some firmware which the driver wants to load
> but where it's happy to continue if the user didn't provide one and
> doesn't want to introduce needless delays.
OK, I got it, thank you for sharing the use case.
If loading directly, the patch isn't needed because filp_open() can
return failure on non-existent file.
If loading via user space, timeout or not depends on userspace,
at least udev won't timeout on non-existent firmware image.
Also looks request_firmware_nowait() is better for the case, _nowait()
can avoid unnecessary delay and speedup firmware loading if there
are more than one firmware to load.
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 12:02 ` Ming Lei
@ 2012-10-09 12:36 ` Mark Brown
2012-10-09 14:55 ` Ming Lei
0 siblings, 1 reply; 11+ messages in thread
From: Mark Brown @ 2012-10-09 12:36 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-kernel
On Tue, Oct 09, 2012 at 08:02:18PM +0800, Ming Lei wrote:
> If loading via user space, timeout or not depends on userspace,
> at least udev won't timeout on non-existent firmware image.
This may be a mdev or old udev thing... it's definitely prevelant.
> Also looks request_firmware_nowait() is better for the case, _nowait()
> can avoid unnecessary delay and speedup firmware loading if there
> are more than one firmware to load.
It doesn't really help as the ABI is such that you can only have one
request_firmware() in play at once (unless this changed since I last
looked at it).
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 12:36 ` Mark Brown
@ 2012-10-09 14:55 ` Ming Lei
2012-10-10 1:55 ` Mark Brown
0 siblings, 1 reply; 11+ messages in thread
From: Ming Lei @ 2012-10-09 14:55 UTC (permalink / raw)
To: Mark Brown; +Cc: linux-kernel
On Tue, Oct 9, 2012 at 8:36 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Oct 09, 2012 at 08:02:18PM +0800, Ming Lei wrote:
>
>> If loading via user space, timeout or not depends on userspace,
>> at least udev won't timeout on non-existent firmware image.
>
> This may be a mdev or old udev thing... it's definitely prevelant.
I just checked history of udev, and looks it can't timeout on
non-existent firmware file, even with the oldest shell script of
firmware.sh.
Also looks mdev of busybox checks firmware file too before loading.
IMO, firmware loader can't help the problem if userspace
chooses to timeout on non-existent file. Maybe you have to
depend on direct loading.
>
>> Also looks request_firmware_nowait() is better for the case, _nowait()
>> can avoid unnecessary delay and speedup firmware loading if there
>> are more than one firmware to load.
>
> It doesn't really help as the ABI is such that you can only have one
Could you let me know where the ABI is?
> request_firmware() in play at once (unless this changed since I last
> looked at it).
I guess you mean that only one firmware device can be added
as child of the device which is requesting firmware.
The commit below(already merged into linus tree) should fix
the problem:
99c2aa72306079976369aad7fc62cc71931d692a(firmware loader:
fix creation failure of fw loader device)
Could you test it to see if more than one request_firmware_nowait()
can be called concurrently for the same device?
Thanks,
--
Ming Lei
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc()
2012-10-09 14:55 ` Ming Lei
@ 2012-10-10 1:55 ` Mark Brown
0 siblings, 0 replies; 11+ messages in thread
From: Mark Brown @ 2012-10-10 1:55 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-kernel
On Tue, Oct 09, 2012 at 10:55:17PM +0800, Ming Lei wrote:
> On Tue, Oct 9, 2012 at 8:36 PM, Mark Brown
> > On Tue, Oct 09, 2012 at 08:02:18PM +0800, Ming Lei wrote:
> > It doesn't really help as the ABI is such that you can only have one
> Could you let me know where the ABI is?
It's defined by firmware_class?
> > request_firmware() in play at once (unless this changed since I last
> > looked at it).
> I guess you mean that only one firmware device can be added
> as child of the device which is requesting firmware.
> The commit below(already merged into linus tree) should fix
> the problem:
> 99c2aa72306079976369aad7fc62cc71931d692a(firmware loader:
> fix creation failure of fw loader device)
That's not been well advertised (and is extremely recent too)...
> Could you test it to see if more than one request_firmware_nowait()
> can be called concurrently for the same device?
Not for some time.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-10-10 1:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-05 17:05 [PATCH] firmware: Don't attempt to allocate zero bytes with vmalloc() Mark Brown
2012-10-08 22:56 ` Ming Lei
2012-10-09 4:19 ` Mark Brown
2012-10-09 7:05 ` Ming Lei
2012-10-09 7:13 ` Mark Brown
2012-10-09 7:34 ` Ming Lei
2012-10-09 7:52 ` Mark Brown
2012-10-09 12:02 ` Ming Lei
2012-10-09 12:36 ` Mark Brown
2012-10-09 14:55 ` Ming Lei
2012-10-10 1:55 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).