Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares
On Thu, Jul 26, 2012 at 12:52 AM, Borislav Petkov wrote:
> On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
>> This patches introduces the three helpers below:
>>
>> void device_cache_firmwares(void)
>> void device_uncache_firmwares(void)
>> void device_uncache_firmwares_delay(unsigned long)
>
> I kinda don't like the plural of firmware: "firmwares". Can we call
> those
> device_cache_fw_images
> device_uncache_fw_images
Looks fine.
>
> or
> device_cache_fw_blobs
>
> or whatever?
>
>> so we can call device_cache_firmwares() to cache firmwares for
>> all devices which need firmwares to work, and the device driver
>> can get the firmware easily from kernel memory when system isn't
>> readly for completing their requests of loading firmwares.
>>
>> When system is ready for completing firmware loading, driver core
>> can call device_uncache_firmwares() or its delay version to free
>> the cached firmwares.
>>
>> The above helpers should be used to cache device firmwares during
>> system suspend/resume cycle in the following patches.
>>
>> Signed-off-by: Ming Lei
>> ---
>> drivers/base/firmware_class.c | 236
>> +++--
>> 1 file changed, 230 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index c181e6d..7a96e75 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -22,6 +22,10 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> +
>> +#include "base.h"
>>
>> MODULE_AUTHOR("Manuel Estrada Sainz");
>> MODULE_DESCRIPTION("Multi purpose firmware loading support");
>> @@ -91,6 +95,17 @@ struct firmware_cache {
>> /* firmware_buf instance will be added into the below list */
>> spinlock_t lock;
>> struct list_head head;
>> +
>> + /*
>> + * Name of firmware which has been cached successfully will be
>> + * added into the below list so that device uncache helper can
>> + * trace which firmware has been cached before.
>> + */
>
> Comment above the list_head and maybe call the list_head fw_names or so?
>
>> + spinlock_t name_lock;
>> + struct list_head name_head;
>> + wait_queue_head_t wait_queue;
>
> Stray \t
>
>> + int cnt;
>> + struct delayed_work work;
>> };
>>
>> struct firmware_buf {
>> @@ -107,6 +122,11 @@ struct firmware_buf {
>> char fw_id[];
>> };
>>
>> +struct fw_name_for_cache {
>> + struct list_head list;
>> + char name[];
>> +};
>
> Maybe fw_cache_entry?
Looks fine.
>
>> +
>> struct firmware_priv {
>> struct timer_list timeout;
>> bool nowait;
>> @@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
>> kref_put(>ref, __fw_free_buf);
>> }
>>
>> -static void fw_cache_init(void)
>> -{
>> - spin_lock_init(_cache.lock);
>> - INIT_LIST_HEAD(_cache.head);
>> -}
>> -
>> static struct firmware_priv *to_firmware_priv(struct device *dev)
>> {
>> return container_of(dev, struct firmware_priv, dev);
>> @@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
>> return -EINVAL;
>> }
>>
>> +static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
>> +{
>> + struct fw_name_for_cache *nc;
>> +
>> + nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
>> + if (!nc)
>> + goto exit;
>> +
>> + strcpy(nc->name, name);
>> +exit:
>> + return nc;
>> +}
>> +
>> +static void free_fw_name_cache(struct fw_name_for_cache *nc)
>> +{
>> + kfree(nc);
>> +}
>> +
>> +static void __async_dev_cache_firmware(void *fw_name,
>> + async_cookie_t cookie)
>
> Arg alignment.
I am wondering why checkpatch.pl doesn't add the check...
>
>> +{
>> + struct fw_name_for_cache *nc;
>> + struct firmware_cache *fwc = _cache;
>> + char *curr_name;
>> + int ret;
>> +
>> + /* 'fw_name' is stored in devres, and it may be released,
>> + * so allocate buffer to store the firmware name
>> + */
>> + curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
>> + if (!curr_name) {
>> + ret = -ENOMEM;
>> + goto drop_ref;
>> + }
>> +
>> + strcpy(curr_name, fw_name);
>
> AFAICT kstrdup already copies the existing string, why do you need to
> strcpy it again?
Good catch.
>
>> +
>> + ret = cache_firmware(curr_name);
>> +
>> + if (!ret) {
>
> Invert check logic to save an indentation level:
>
> if (ret)
> goto free;
>
> nc = alloc...
>
> ...
>
> free:
> kfree(curr_name);
>
>> + /*
>> + * allocate/all the instance of alloc_fw_name_cache
>> + * for uncaching later if cache_firmware returns
>> + * successfully
>> + */
>> + nc = alloc_fw_name_cache(curr_name);
>> +
>> + /*
>> + * have to uncache firmware in case of
Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares
On Thu, Jul 26, 2012 at 12:52 AM, Borislav Petkov b...@amd64.org wrote:
On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
This patches introduces the three helpers below:
void device_cache_firmwares(void)
void device_uncache_firmwares(void)
void device_uncache_firmwares_delay(unsigned long)
I kinda don't like the plural of firmware: firmwares. Can we call
those
device_cache_fw_images
device_uncache_fw_images
Looks fine.
or
device_cache_fw_blobs
or whatever?
so we can call device_cache_firmwares() to cache firmwares for
all devices which need firmwares to work, and the device driver
can get the firmware easily from kernel memory when system isn't
readly for completing their requests of loading firmwares.
When system is ready for completing firmware loading, driver core
can call device_uncache_firmwares() or its delay version to free
the cached firmwares.
The above helpers should be used to cache device firmwares during
system suspend/resume cycle in the following patches.
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/base/firmware_class.c | 236
+++--
1 file changed, 230 insertions(+), 6 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c181e6d..7a96e75 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -22,6 +22,10 @@
#include linux/slab.h
#include linux/sched.h
#include linux/list.h
+#include linux/async.h
+#include linux/pm.h
+
+#include base.h
MODULE_AUTHOR(Manuel Estrada Sainz);
MODULE_DESCRIPTION(Multi purpose firmware loading support);
@@ -91,6 +95,17 @@ struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
struct list_head head;
+
+ /*
+ * Name of firmware which has been cached successfully will be
+ * added into the below list so that device uncache helper can
+ * trace which firmware has been cached before.
+ */
Comment above the list_head and maybe call the list_head fw_names or so?
+ spinlock_t name_lock;
+ struct list_head name_head;
+ wait_queue_head_t wait_queue;
Stray \t
+ int cnt;
+ struct delayed_work work;
};
struct firmware_buf {
@@ -107,6 +122,11 @@ struct firmware_buf {
char fw_id[];
};
+struct fw_name_for_cache {
+ struct list_head list;
+ char name[];
+};
Maybe fw_cache_entry?
Looks fine.
+
struct firmware_priv {
struct timer_list timeout;
bool nowait;
@@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
kref_put(buf-ref, __fw_free_buf);
}
-static void fw_cache_init(void)
-{
- spin_lock_init(fw_cache.lock);
- INIT_LIST_HEAD(fw_cache.head);
-}
-
static struct firmware_priv *to_firmware_priv(struct device *dev)
{
return container_of(dev, struct firmware_priv, dev);
@@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
return -EINVAL;
}
+static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
+{
+ struct fw_name_for_cache *nc;
+
+ nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
+ if (!nc)
+ goto exit;
+
+ strcpy(nc-name, name);
+exit:
+ return nc;
+}
+
+static void free_fw_name_cache(struct fw_name_for_cache *nc)
+{
+ kfree(nc);
+}
+
+static void __async_dev_cache_firmware(void *fw_name,
+ async_cookie_t cookie)
Arg alignment.
I am wondering why checkpatch.pl doesn't add the check...
+{
+ struct fw_name_for_cache *nc;
+ struct firmware_cache *fwc = fw_cache;
+ char *curr_name;
+ int ret;
+
+ /* 'fw_name' is stored in devres, and it may be released,
+ * so allocate buffer to store the firmware name
+ */
+ curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
+ if (!curr_name) {
+ ret = -ENOMEM;
+ goto drop_ref;
+ }
+
+ strcpy(curr_name, fw_name);
AFAICT kstrdup already copies the existing string, why do you need to
strcpy it again?
Good catch.
+
+ ret = cache_firmware(curr_name);
+
+ if (!ret) {
Invert check logic to save an indentation level:
if (ret)
goto free;
nc = alloc...
...
free:
kfree(curr_name);
+ /*
+ * allocate/all the instance of alloc_fw_name_cache
+ * for uncaching later if cache_firmware returns
+ * successfully
+ */
+ nc = alloc_fw_name_cache(curr_name);
+
+ /*
+ * have to uncache firmware in case of allocation
+ * failure since we can't trace the firmware cache
+ * any more without the firmware name.
+ */
+ if (!nc) {
+
Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares
On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
> This patches introduces the three helpers below:
>
> void device_cache_firmwares(void)
> void device_uncache_firmwares(void)
> void device_uncache_firmwares_delay(unsigned long)
I kinda don't like the plural of firmware: "firmwares". Can we call
those
device_cache_fw_images
device_uncache_fw_images
or
device_cache_fw_blobs
or whatever?
> so we can call device_cache_firmwares() to cache firmwares for
> all devices which need firmwares to work, and the device driver
> can get the firmware easily from kernel memory when system isn't
> readly for completing their requests of loading firmwares.
>
> When system is ready for completing firmware loading, driver core
> can call device_uncache_firmwares() or its delay version to free
> the cached firmwares.
>
> The above helpers should be used to cache device firmwares during
> system suspend/resume cycle in the following patches.
>
> Signed-off-by: Ming Lei
> ---
> drivers/base/firmware_class.c | 236
> +++--
> 1 file changed, 230 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index c181e6d..7a96e75 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -22,6 +22,10 @@
> #include
> #include
> #include
> +#include
> +#include
> +
> +#include "base.h"
>
> MODULE_AUTHOR("Manuel Estrada Sainz");
> MODULE_DESCRIPTION("Multi purpose firmware loading support");
> @@ -91,6 +95,17 @@ struct firmware_cache {
> /* firmware_buf instance will be added into the below list */
> spinlock_t lock;
> struct list_head head;
> +
> + /*
> + * Name of firmware which has been cached successfully will be
> + * added into the below list so that device uncache helper can
> + * trace which firmware has been cached before.
> + */
Comment above the list_head and maybe call the list_head fw_names or so?
> + spinlock_t name_lock;
> + struct list_head name_head;
> + wait_queue_head_t wait_queue;
Stray \t
> + int cnt;
> + struct delayed_work work;
> };
>
> struct firmware_buf {
> @@ -107,6 +122,11 @@ struct firmware_buf {
> char fw_id[];
> };
>
> +struct fw_name_for_cache {
> + struct list_head list;
> + char name[];
> +};
Maybe fw_cache_entry?
> +
> struct firmware_priv {
> struct timer_list timeout;
> bool nowait;
> @@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
> kref_put(>ref, __fw_free_buf);
> }
>
> -static void fw_cache_init(void)
> -{
> - spin_lock_init(_cache.lock);
> - INIT_LIST_HEAD(_cache.head);
> -}
> -
> static struct firmware_priv *to_firmware_priv(struct device *dev)
> {
> return container_of(dev, struct firmware_priv, dev);
> @@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
> return -EINVAL;
> }
>
> +static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
> +{
> + struct fw_name_for_cache *nc;
> +
> + nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
> + if (!nc)
> + goto exit;
> +
> + strcpy(nc->name, name);
> +exit:
> + return nc;
> +}
> +
> +static void free_fw_name_cache(struct fw_name_for_cache *nc)
> +{
> + kfree(nc);
> +}
> +
> +static void __async_dev_cache_firmware(void *fw_name,
> + async_cookie_t cookie)
Arg alignment.
> +{
> + struct fw_name_for_cache *nc;
> + struct firmware_cache *fwc = _cache;
> + char *curr_name;
> + int ret;
> +
> + /* 'fw_name' is stored in devres, and it may be released,
> + * so allocate buffer to store the firmware name
> + */
> + curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
> + if (!curr_name) {
> + ret = -ENOMEM;
> + goto drop_ref;
> + }
> +
> + strcpy(curr_name, fw_name);
AFAICT kstrdup already copies the existing string, why do you need to
strcpy it again?
> +
> + ret = cache_firmware(curr_name);
> +
> + if (!ret) {
Invert check logic to save an indentation level:
if (ret)
goto free;
nc = alloc...
...
free:
kfree(curr_name);
> + /*
> + * allocate/all the instance of alloc_fw_name_cache
> + * for uncaching later if cache_firmware returns
> + * successfully
> + */
> + nc = alloc_fw_name_cache(curr_name);
> +
> + /*
> + * have to uncache firmware in case of allocation
> + * failure since we can't trace the firmware cache
> + * any more without the firmware name.
> + */
> + if (!nc) {
> + uncache_firmware(curr_name);
> + } else {
> + spin_lock(>name_lock);
> + list_add(>list,
Re: [RFC PATCH 11/13] driver core: firmware: introduce devices_cache/uncache_firmwares
On Wed, Jul 25, 2012 at 01:00:11AM +0800, Ming Lei wrote:
This patches introduces the three helpers below:
void device_cache_firmwares(void)
void device_uncache_firmwares(void)
void device_uncache_firmwares_delay(unsigned long)
I kinda don't like the plural of firmware: firmwares. Can we call
those
device_cache_fw_images
device_uncache_fw_images
or
device_cache_fw_blobs
or whatever?
so we can call device_cache_firmwares() to cache firmwares for
all devices which need firmwares to work, and the device driver
can get the firmware easily from kernel memory when system isn't
readly for completing their requests of loading firmwares.
When system is ready for completing firmware loading, driver core
can call device_uncache_firmwares() or its delay version to free
the cached firmwares.
The above helpers should be used to cache device firmwares during
system suspend/resume cycle in the following patches.
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/base/firmware_class.c | 236
+++--
1 file changed, 230 insertions(+), 6 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index c181e6d..7a96e75 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -22,6 +22,10 @@
#include linux/slab.h
#include linux/sched.h
#include linux/list.h
+#include linux/async.h
+#include linux/pm.h
+
+#include base.h
MODULE_AUTHOR(Manuel Estrada Sainz);
MODULE_DESCRIPTION(Multi purpose firmware loading support);
@@ -91,6 +95,17 @@ struct firmware_cache {
/* firmware_buf instance will be added into the below list */
spinlock_t lock;
struct list_head head;
+
+ /*
+ * Name of firmware which has been cached successfully will be
+ * added into the below list so that device uncache helper can
+ * trace which firmware has been cached before.
+ */
Comment above the list_head and maybe call the list_head fw_names or so?
+ spinlock_t name_lock;
+ struct list_head name_head;
+ wait_queue_head_t wait_queue;
Stray \t
+ int cnt;
+ struct delayed_work work;
};
struct firmware_buf {
@@ -107,6 +122,11 @@ struct firmware_buf {
char fw_id[];
};
+struct fw_name_for_cache {
+ struct list_head list;
+ char name[];
+};
Maybe fw_cache_entry?
+
struct firmware_priv {
struct timer_list timeout;
bool nowait;
@@ -214,12 +234,6 @@ static void fw_free_buf(struct firmware_buf *buf)
kref_put(buf-ref, __fw_free_buf);
}
-static void fw_cache_init(void)
-{
- spin_lock_init(fw_cache.lock);
- INIT_LIST_HEAD(fw_cache.head);
-}
-
static struct firmware_priv *to_firmware_priv(struct device *dev)
{
return container_of(dev, struct firmware_priv, dev);
@@ -981,6 +995,216 @@ int uncache_firmware(const char *fw_name)
return -EINVAL;
}
+static struct fw_name_for_cache *alloc_fw_name_cache(const char *name)
+{
+ struct fw_name_for_cache *nc;
+
+ nc = kzalloc(sizeof(nc) + strlen(name) + 1, GFP_KERNEL);
+ if (!nc)
+ goto exit;
+
+ strcpy(nc-name, name);
+exit:
+ return nc;
+}
+
+static void free_fw_name_cache(struct fw_name_for_cache *nc)
+{
+ kfree(nc);
+}
+
+static void __async_dev_cache_firmware(void *fw_name,
+ async_cookie_t cookie)
Arg alignment.
+{
+ struct fw_name_for_cache *nc;
+ struct firmware_cache *fwc = fw_cache;
+ char *curr_name;
+ int ret;
+
+ /* 'fw_name' is stored in devres, and it may be released,
+ * so allocate buffer to store the firmware name
+ */
+ curr_name = kstrdup((const char *)fw_name, GFP_KERNEL);
+ if (!curr_name) {
+ ret = -ENOMEM;
+ goto drop_ref;
+ }
+
+ strcpy(curr_name, fw_name);
AFAICT kstrdup already copies the existing string, why do you need to
strcpy it again?
+
+ ret = cache_firmware(curr_name);
+
+ if (!ret) {
Invert check logic to save an indentation level:
if (ret)
goto free;
nc = alloc...
...
free:
kfree(curr_name);
+ /*
+ * allocate/all the instance of alloc_fw_name_cache
+ * for uncaching later if cache_firmware returns
+ * successfully
+ */
+ nc = alloc_fw_name_cache(curr_name);
+
+ /*
+ * have to uncache firmware in case of allocation
+ * failure since we can't trace the firmware cache
+ * any more without the firmware name.
+ */
+ if (!nc) {
+ uncache_firmware(curr_name);
+ } else {
+ spin_lock(fwc-name_lock);
+ list_add(nc-list, fwc-name_head);
+
