Messages in this thread | | | From | Andrey Borzenkov <> | Subject | Re: [PATCH] firmware: speed up request_firmware() | Date | Fri, 3 Apr 2009 21:25:24 +0400 |
| |
On Friday 03 of April 2009 10:46:11 Clemens Ladisch wrote: > Ira Snyder wrote: > > I didn't want to change an existing kernel interface, so I just > > made the easiest change that worked for me. > > Well, userspace does know the actual size of the image, so I see no > reason why it shouldn't be able to tell the kernel about it > beforehand. >
Right; but it will need some time for user space to catch up.
> > I'd be happy to test patches anyone comes up with. > > ===== > > This adds a data_size attribute to the firmware loading device so > that userspace can tell us about the firmware image size. This > allows us to preallocate the buffer with the final size, thus > avoiding reallocating the buffer for every page of data as it comes > in. > > Signed-off-by: Clemens Ladisch <clemens@ladisch.de> > > --- linux-2.6.orig/Documentation/firmware_class/README > +++ linux-2.6/Documentation/firmware_class/README > @@ -21,7 +21,7 @@ > kernel(driver): calls request_firmware(&fw_entry, $FIRMWARE, > device) > > userspace: > - - /sys/class/firmware/xxx/{loading,data} appear. > + - /sys/class/firmware/xxx/{loading,data,data_size} appear. > - hotplug gets called with a firmware identifier in $FIRMWARE > and the usual hotplug environment. > - hotplug: echo 1 > /sys/class/firmware/xxx/loading > @@ -29,11 +29,13 @@ > kernel: Discard any previous partial load. > > userspace: > + - hotplug: echo ... > /sys/class/firmware/xxx/data_size > - hotplug: cat appropriate_firmware_image > \ > /sys/class/firmware/xxx/data > > - kernel: grows a buffer in PAGE_SIZE increments to hold the image as > it - comes in. > + kernel: Copies the firmware image into a buffer of the specified > size. + If the image is larger, the buffer automatically grows in + > PAGE_SIZE increments. > > userspace: > - hotplug: echo 0 > /sys/class/firmware/xxx/loading > @@ -60,6 +62,9 @@ > > HOTPLUG_FW_DIR=/usr/lib/hotplug/firmware/ > > + if [ -e /sys/$DEVPATH/data_size ]; then > + stat -c %s $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data_size > + fi > echo 1 > /sys/$DEVPATH/loading > cat $HOTPLUG_FW_DIR/$FIRMWARE > /sysfs/$DEVPATH/data > echo 0 > /sys/$DEVPATH/loading > @@ -73,6 +78,9 @@ > - firmware_data_read() and firmware_loading_show() are just > provided for testing and completeness, they are not called in normal > use. > > + - /sys/class/firmware/xxx/data_size is optional for compatibility > with + older kernels. > + > - There is also /sys/class/firmware/timeout which holds a timeout > in seconds for the whole load operation. > > --- linux-2.6.orig/Documentation/firmware_class/hotplug-script > +++ linux-2.6/Documentation/firmware_class/hotplug-script > @@ -6,6 +6,9 @@ > > HOTPLUG_FW_DIR=/usr/lib/hotplug/firmware/ > > +if [ -e /sys/$DEVPATH/data_size ]; then > + stat -c %s $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data_size > +fi > echo 1 > /sys/$DEVPATH/loading > cat $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data > echo 0 > /sys/$DEVPATH/loading > --- linux-2.6.orig/drivers/base/firmware_class.c > +++ linux-2.6/drivers/base/firmware_class.c > @@ -46,6 +46,7 @@ struct firmware_priv { > struct firmware *fw; > unsigned long status; > int alloc_size; > + int size_hint;
Unsigned?
> struct timer_list timeout; > }; > > @@ -114,6 +115,32 @@ static struct class firmware_class = { > .dev_release = fw_dev_release, > }; > > +static ssize_t firmware_data_size_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct firmware_priv *fw_priv = dev_get_drvdata(dev); > + > + return sprintf(buf, "%d\n", fw_priv->size_hint); > +} > +
Why would you need it? It is no importance once firmware had been loaded. I'd personally consider it as write-only.
> +static ssize_t firmware_data_size_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct firmware_priv *fw_priv = dev_get_drvdata(dev); > + long value; > + int err; > + > + err = strict_strtol(buf, 10, &value); > + if (err) > + return err; > + fw_priv->size_hint = value;
Should not there be some protection against using silly large values?
> + return count; > +} > + > +static DEVICE_ATTR(data_size, 0644, > + firmware_data_size_show, firmware_data_size_store); > + > static ssize_t firmware_loading_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > @@ -207,6 +234,7 @@ fw_realloc_buffer(struct firmware_priv * > if (min_size <= fw_priv->alloc_size) > return 0; > > + min_size = max(min_size, fw_priv->size_hint); > new_size = ALIGN(min_size, PAGE_SIZE); > new_data = vmalloc(new_size); > if (!new_data) { > @@ -359,6 +387,12 @@ static int fw_setup_device(struct firmwa > goto error_unreg; > } > > + retval = device_create_file(f_dev, &dev_attr_data_size); > + if (retval) { > + dev_err(device, "%s: device_create_file failed\n", __func__); > + goto error_unreg; > + } > + > retval = device_create_file(f_dev, &dev_attr_loading); > if (retval) { > dev_err(device, "%s: device_create_file failed\n", __func__);
[unhandled content-type:application/pgp-signature] | |