Lesson 4: Add dynamic memory allocation to ”xxx” driver #34
Conversation
Task: Add dynamic memory allocation to ”xxx” driver
from sysfs example using kmalloc and kmem_cache API.
- modify building with BB board
- add module parameter mem_type as:
0 - kmalloc;
1 - cache;
2 - vmalloc
Signed-off-by: oleksii-kogutenko <[email protected]>
oleksii-kogutenko
force-pushed
the
alexandr_mm
branch
from
542d267 to
c95425b
Compare
| * 144 * same, but that's harder if the variable must be non-static or is inside a | ||
| * 145 * structure. This allows exposure under a different name. | ||
| * 146 */ | ||
| module_param( mem_type, int, 0444 ); |
There was a problem hiding this comment.
Is it intended to be read-only?
There was a problem hiding this comment.
I made access right to read-only, because of didn't see any reasons to made other rights
There was a problem hiding this comment.
- Memory allocation (at least by
kmallocandvmalloc) should be dynamic; - Memory cache case IMO requires some refactoring.
- Please fix checkpatch issues:
${KERNEL_SOURCE}/scripts/checkpatch.pl 0001-Add-dynamic-memory-allocation-to-xxx-driver.patch --terse
total: 41 errors, 32 warnings, 197 lines checked
| KERNELDIR := $(BUILD_KERNEL) | ||
| ifeq ($(KERNELDIR),) | ||
| ifeq ($(BBB_KERNEL),) | ||
| $(error Path to kernel tree - KERNELDIR or BBB_KERNEL variable is not defined!) |
There was a problem hiding this comment.
Indentation with tabs aren't allowed outside of rules.
I should fix that in mpu6050 example.
Sometimes I hate make syntax.
| KERNELDIR ?= $(BBB_KERNEL) | ||
|
|
||
| export ARCH ?= arm | ||
| export CROSS_COMPILE ?= arm-linux-gnueabihf- |
There was a problem hiding this comment.
Why have you made this exercise cross-compiled?
Don't you like qemu that much?
| } | ||
| strcpy( buf, pbuf_msg ); | ||
| pr_info( "[%s:%s] read %ld\n", THIS_MODULE->name, __FUNCTION__, (long)strlen( buf ) ); | ||
| return strlen( buf ); |
There was a problem hiding this comment.
It's better to avoid calling strlen() twice.
| * #define KMEM_CACHE(__struct, __flags) kmem_cache_create(#__struct,\ | ||
| * 148 sizeof(struct __struct), __alignof__(struct __struct),\ | ||
| * 149 (__flags), NULL) | ||
| */ |
There was a problem hiding this comment.
Please avoid such copy-paste comments in sources.
If you need some manual to be left for future, rewrite it accordingly (or write a link to original definition).
|
|
||
| void init_kcache(void) | ||
| { | ||
| if (mem_type != MEM_TYPE_KCACHE) return; |
There was a problem hiding this comment.
IMO, it's more logically to use opposite code structure:
if (mem_type == MEM_TYPE_KCACHE) {
/* init cache */
}
And in this particular case - I'd move mem_type verification to the x_init()
and made call of init_kcache() conditional.
Otherwise init_kcache() should be renamed to reflect, it inits any kind of buffer (even if for other types nothing is done).
| strncpy(buf_msg, buf, count); | ||
| buf_msg[count] = '\0'; | ||
| if (!pbuf_msg) { | ||
| pbuf_msg = mem_alloc(mem_type, BUFFER_SIZE); |
There was a problem hiding this comment.
The idea was to allocate required amount of memory dynamically.
Not to do that once (even if on first write), but be able to adjust buffer size at least for bigger count.
| } | ||
|
|
||
| void deinit_kcache(void) | ||
| { |
There was a problem hiding this comment.
Again - function name doesn't correspond to its function.
| @@ -35,20 +146,37 @@ int __init x_init(void) | |||
| { | |||
| int res; | |||
|
|
|||
| pr_info("mem_type is %d\n", mem_type); | |||
| x_class = class_create(THIS_MODULE, "x-class"); | |||
| if (IS_ERR(x_class)) | |||
| pr_info("bad class create\n"); | |||
| res = class_create_file(x_class, &class_attr_xxx); | |||
There was a problem hiding this comment.
xxx_store may already be called at this point,
while pmem_cache hasn't yet been created.
Task: Add dynamic memory allocation to ”xxx” driver
from sysfs example using kmalloc and kmem_cache API.
0 - kmalloc;
1 - cache;
2 - vmalloc
Signed-off-by: oleksii-kogutenko [email protected]