Re: [PATCH] firmware: speed up request_firmware()

[Clemens Ladisch <clemens@ladisch.de>]

Andrey Borzenkov wrote:
> On Friday 03 of April 2009 10:46:11 Clemens Ladisch wrote:
> > 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.

It's only an optimization.

> > @@ -46,6 +46,7 @@ struct firmware_priv {
> >  	struct firmware *fw;
> >  	unsigned long status;
> >  	int alloc_size;
> > +	int size_hint;
> 
> Unsigned?

It is as signed as alloc_size.  ;-)

Yes, all these variables probably should be size_t.  

> > +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?

Good question.  Apparently, for the same reason why we'd need
firmware_data_read ...

> > +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? 

It is already possible to do "cat /dev/zero > .../data".  What we'd need
is a limit not only on this variable but on the size of the firmware
image itself.


Okay, I'll do a bunch of patches to fix these warts in the firmware
loader.


Best regards,
Clemens