Devm functions and their CORRECT usage

Hi,

Lets start this blog with some good news. Guess what, I won the Linux Foundation Training Scholarship under Kernel Guru category. I am so excited for taking Linux Foundation training class. They announced names of scholarship winners in LinuxCon NA which was in settle last week and it came out that I am a youngest scholar who won this scholarship. 🙂

So, lets come to the topic of this blog post. Over the last couple of weeks, I worked on devm functions and their incorrect usage. I couldn’t make it to update my blog properly regarding the same. But in this blog, I have tried to write good summary of my work on devm functions and by adding links wherever possible for someone who is more interested in the discussions and patches. First of all, lets start with introducing devm functions.

Devm Functions: What, Why and Where

There are some common memory resources used by drivers which are allocated using functions and stored as a linked list of arbitrarily sized memory areas called devres. Some of this memory is allocated in the probe function. But all of this memory should be freed on a failure patch or before the driver is detached. Else, the driver will leak resources and wastes main memory.

There are couple of reasons behind introducing devm functions. First is of course about resource leaking. It can be possible that if anything fails in the middle of probe function, it frees anything allocated. Also, remove function is just a duplicate code of probe’s error handling. And most important thing is it is hard to spot problems in failure handling with non-managed resource allocation. So, we need managed versions of such functions = devm functions. Devm functions basically allocates memory in a order resources are allocated and deallocates those memory automatically in a reverse order. Here, is a link of slides which can be useful to understand devm functions and their usages.

Problems I encountered or came across while looking in to devm functions

1. Basically sometimes what happens is developers want to use devm functions because they think that it is better to use managed resource functions but they don’t know how to use it and end up with messing things. For example, here is one of my patch. In this file, we already have devm_snd_soc_register_card and as I said devm functions automatically handles when to free memory. So, actually we don’t need to use function for unregistering the card. I have sent some patches for similar cases and I have provided links of accepted patches at the end of the blog.

2. While working on devm functions, I encountered that many files are using devm_free_irq in remove functions with the devm_request_irq in probe function. One can understand that we need devm_free_irq in remove function when we don’t have devm counterparts for each allocated resource function and it needs to occur before something else to be released. That can be a case where we need to call devm_free_irq explicitly. But not all cases are like that. Here is a link of one of such discussion on a case where use of devm_free_irq is not at all necessary. There are some other such interesting cases for devm_free_irq and but one need to look closely for each case. Also, it can be possible that there are some other such cases for other devm functions. One another example is this.

3. Sometimes it is possible that memory lives within a single function and frees after some line of code. Then we don’t really need devm functions because it just wastes extra memory used for devres structures and few extra cycles for maintaining them. Here, is such example.

Here, are those Coccinelle semantic patches which I used to detect such cases. File devm_entry_points is used to detect problematic usage of devm functions and devm_opportunity is used for finding files which are still using non-managed functions whose devm counterparts already exists.  I worked on each case encountered by devm_entry_points but not going to discuss it here. In case you have any questions please ask me here and I’ll be happy to answer them.

I sent many patches using both of these files and for some which came in to my way while working on devm functions. Here, is a links of some of my accepted patches for the reference and there are many more in their way to be accepted. All of my accepted patches can be found here.  I am going to send some more as still there are many opportunities where devm functions can be used.

I know understanding devm functions is little bit complicated at first but once you will understand them properly, you can enjoy the beauty of them. Please ping me for any queries regarding devm functions or my patches.

Links:

  1. power_supply: bq24735: Convert to using managed resources
  2. ASoC: tegra: Use devm_clk_get
  3. ASoC: tegra: Convert to managed resources
  4. ASoC: davinci-vcif: Use devm_snd_soc_register_component
  5. crypto: sahara – Use dmam_alloc_coherent
  6. ASoC: rockchip: i2s: Adjust devm usage
  7. ASoC: samsung: Remove redundant arndale_audio_remove
  8. ata: pata_arasam_cf: Use devm_clk_get

Update – 1: I have continued working on devm functions even after the internship. So, now there are many patches which are accepted in the mainline kernel. And still there are many opportunities where one can go for working on. So, here is a link of my all patches. One can go there and check the things for better understanding.

Advertisements

Ether device API functions and some other hacks

Hi again

I worked on ether device API functions in a last month before module init/exit cases. Mainly I worked on 3 functions and got understanding of some others. I am going to talk about some other functions along with ether device API functions in this blog. So, this is going to be somehow managed blog.

1. eth_zero_addr:

This is a ether device API function which replaces memset to assign zero address. Linux kernel community prefers to using eth_zero_addr function instead of memset to assign the zero address to the given array. The function definition goes like this:

static inline void eth_zero_addr(u8 *addr)
{
memset(addr, 0x00, ETH_ALEN);
}

There were like 10-12 files having such cases. I sent patches for all of them. Most of them are now added in a kernel tree. Here, is a semantic patch which I used to do this change. It is preety simple. Only thing is sometimes value ‘6’ is used as ETH_ALEN. In that case one need to check that it is really a network code and in a memset 6 means ETH_ALEN.

@eth_zero_addr@
expression e;
@@

-memset(e,0x00,\(6\|ETH_ALEN\));
+eth_zero_addr(e);

2. eth_broadcast_addr:
This is also ether device API function and replaces memset. Only difference is this function is used to assign the broadcast address to the given address array. There were only 2-3 such cases. I handled all of them too. Semantic patch I used is as follows:

@eth_broadcast_addr@
identifier e;
@@

-memset(e,\(0xff\|0xFF\|255\),\(6\|ETH_ALEN\));
+eth_broadcast_addr(e);

3. eth_hw_addr_random:

This is an intersting function. It generates random Ethernet address [MAC] to be used by a net device and set addr_assign_type so that state can be read by sysfs and be used by userspace. Now, definition of this function goes like this:

static inline void eth_hw_addr_random(struct net_device *dev)
{
dev->addr_assign_type = NET_ADDR_RANDOM;
eth_random_addr(dev->dev_addr);
}

Basically I found this case from the deprecated function list. There were like 63 uses of random_eth_addr in linux 3.0 and now very less uses are there. Primary reason behind this was here random_eth_addr is changed to eth_random_addrfor the consistency purpose. So, there were many commits proposing that change. But when I looked in to detail then found a large bunch of commits from Danny Kukawka in 2012, replacing random_eth_addr with eth_hw_addr_random. Although in each case one need to check many things at a time because sometimes original call of random_eth_addr/eth_random_addr do not make assignment to NET_ADDR_RANDOM. And in some cases we have seen that merging the call and the assignment is not correct. So, from my side I can just compile test the change. And in each case I need to write about this possibility under —. For the experiment I just sent a patch explaining the situation and it turned out positive. After tesing it maintainer applied the patch. That patch can be found here. I am going to send some more patches in a future.

4. Deprecated macro DEFINE_PCI_DEVICE_TABLE

This macro is actually deprecated and this comment is made in a header file too. I just accidentally came across this while reading some other code. And thought to handle it. Basically point is we can use struct pci_device_id instead of this macro. When I searched for the remaining use of this macro in a linux-next, it turned out that most of them were handled by Julia’s one of other intern in a Paris. Only 3 uses were remaining. I sent patches for them. One is applied in a local branch and others are on their way. This was a simple case too. Semantic patch I used is as follows:

@@
identifier a;
declarer name DEFINE_PCI_DEVICE_TABLE;
initializer i;
@@
– DEFINE_PCI_DEVICE_TABLE(a)
+ const struct pci_device_id a[]
= i;

5. Unnecessary function snd_pcm_lib_preallocate_free_for_all()

I picked this from deprecated functions list of Julia. Here, there is no point of using this function snd_pcm_lib_preallocate_free_for_all() because ALSA core takes care that all preallocated memory is freed when the PCM itself is freed. Only one case was left. I just sent a patch for it yesterday and to my surprise it was applied in a very next minute by Mark. 🙂 One can look at the patch here.

By the way, I came across non-modular version of module init/exit macros. Recently many such patches are added in a kernel tree. It is interesting to understand the reasons behind introducing and using such macro. I guess one separate post would be fine for that. I will post that blog in a 2-3 days. Also, I am working on fixing some usage of devm functions from last 2 weeks. I’ll write a blog about it in a next week too.

Till then signing off. Have a happy weekend.

[Part II] Macros module init/exit, Boilerplate code and Linux Kernel

So, as I said in my last blog, I will explain about transformation semantic patch which can be used after matching module init/exit part. Before I go in to the detail, I would like to clear some points. Someone asked me about the use of * in the semantic patch of my last blog post. So, I thought I should add it in this blog too. Basically we use * for something of interest. For example in our case we just want to check if actually there are some cases present which do nothing except register/unregister in module init/exit. One can’t use * with +/-. Following is an output of the semantic patch explained in last blog post.

diff -u -p ./lib/ts_fsm.c /tmp/nothing/lib/ts_fsm.c
--- ./lib/ts_fsm.c
+++ /tmp/nothing/lib/ts_fsm.c
@@ -327,12 +327,10 @@ static struct ts_ops fsm_ops = {
static int __init init_fsm(void)
{
- return textsearch_register(&fsm_ops);
}
static void __exit exit_fsm(void)
{
- textsearch_unregister(&fsm_ops);
}
MODULE_LICENSE("GPL");

Now lets continue from where we left in last blog post. So, after matching cases, we need to remove functions of module init/exit along with declarations of module-init/module_exit. Also, we need to add helper macros like module_platform_driver. We can do that in individual cases by matching register/unregister functions. Here, is an example of such semantic patch for cases where we can use helper macro module_platform_driver.

@r@
declarer name module_init;
identifier f;
@@
module_init(f);

@s@
declarer name module_exit;
identifier e;
@@
module_exit(e);

@a@
identifier r.f;
identifier x;
@@
static f(…) {return platform_driver_register(&x); }

@b depends on a@
identifier s.e,a.x;
@@
static e(…) { platform_driver_unregister(&x); }

@t depends on r && a@
identifier r.f;
@@
-module_init(f);

@v depends on s && a && b@
declarer name module_platform_driver;
identifier s.e, a.x;
@@
-module_exit(e);
+module_platform_driver(x);

@c depends on b@
identifier r.f, a.x;
@@
-static f(…) { return platform_driver_register(&x); }

@d depends on c@
identifier s.e, a.x;
@@

-static e(…) { platform_driver_unregister(&x); }

In first four rules of the semantic patch we are doing matching and then depending on these 4 rules we are doing transformation. This semantic patch can be used for any such cases. Only thing which one need to change is names of register/unregister functions and helper macro. Interesting, right! Best part is those algorithms which are used by Julia in developing a tool. Because Coccinelle is very fast. And I think that’s the beauty of Coccinelle 🙂 In last 2 weeks, I worked on some ether device API functions too. But may be one separate blog will be good to explain them. So, this will be the subject of my next blog. Till then stay connected. Stay happy. Ttyl.

[Part I] Macros module init/exit, Boilerplate code and Linux Kernel

Hii

Lets talk about boiler plate code today. I worked on cases of boiler plate code in Linux kernel during last few days. I am going to divide this particular topic in 2 blog posts. In this post, I will talk about boiler plate code, cases of them and how can we find them in the kernel using Coccinelle. In the second post, I will talk about how Coccinelle can help to handle such cases.

Boiler plate code:

Boilerplate code is any seemingly repetitive code that shows up again and again in order to get some result that seems like it ought to be much simpler. Basically boilerplate code or boilerplate is the sections of code that have to be included in many places with little or no alteration.

Bolier plate code and init/exit macros:

In kernel macro module_init can either be called during do_initcalls (if builtin) or at module insertion time (if a module). Macro module_exit is used to wrap the driver clean-up code with cleanup_module when used with rmmod and the driver is a module. If the driver is statically compiled into the kernel, module_exit has no effect. There can only be one module_init and one module_exit per module. In 70% of cases drivers don’t do anything special in module init/exit. So, such bolier plate code can be eliminated using some helper macros like module_platform_driver, module_pci_driver, module_pcmcia_driver etc. Here, is an example of such code form kernel:


static int __init snirm710_init(void)
{
return platform_driver_register(&snirm710_driver);
}
static void __exit snirm710_exit(void)
{
platform_driver_unregister(&snirm710_driver);
}
module_init(snirm710_init);
module_exit(snirm710_exit);

[From file drivers/scsi/sni_53c710.c]

Basically these helper macros are defined for drivers whose init and exit paths does only register and unregister. Sometimes we have unnecessary print statements and code in module init/exit too. In such cases we can use these helper macros too. Currently there are some such general macros and driver specific helper macros presented in the kernel. And many more such opportunities are there.

Macros module init/exit and Coccinelle

Generally we can use following Coccinelle semantic patch to match the module init/exit associated functions and statements.

@r@
declarer name module_init;
identifier f;
@@
module_init(f);

@s@
declarer name module_exit;
identifier f;
@@
module_exit(f);

@a@
identifier r.f;
statement S;
@@
f(…) { S }

@depends on a@
identifier s.f;
statement S;
@@
f(…) {
*S
}

@b@
identifier s.f;
statement S;
@@
f(…) { S }

@depends on b@
identifier r.f;
statement S;
@@
f(…) {
*S
}

So, after getting output of this semantic patch I analyzed all cases. And grouped them together accordingly. I grouped them in with categories like old macro and new macro means cases where we can use already defined helper macro and where we need to define new macros respectively. I am thinking to put that file on my github along with some coccinelle scripts so that others can use it. I am going to handle most of them. I have sent some patches and will sent for all those cases where old macro can be used. For the new macro cases, I will send patches introducing some of those macros which can handle maximum cases and such cases are already present there.

Note that above script can help with matching things only. For the transformation one need to be more specific in the script. I will talk about those scripts and some intersting stuff regarding the same in my next post.

Till then stay tuned!

Say yes or no to mod_timer!

Its been almost 20 days that I have not written any blog on my progress. Yes, I know I should and I am feeling sorry for that. 😦 Anyways I am back. 🙂 I worked on many new things and learnt a lot during last few days. I will not be able to sum up everything in a single blog. That’s why here I am going to post 3-4 blog posts back to back again. So, lets talk about mod_timer today.

I came to know about mod_timer function and its usage during Outreachy application. Actually one of Coccinelle challenge problem was for setup_timer function [View my blog on it to get more information]. While looking at the definition of setup_timer function, I got to know about mod_timer function. During application period, I sent few patches for that and they were accepted too. So, I decided to look at them again with the goal of writing one common semantic patch. But I ended up with skipping them completely. There are particular reason behind this. I was not sure about writing this post but then I thought outreachy is all about learning. And as I learnt something from this, I should share it.

Why mod_timer was introduced?

First of all, let me start with why mod_timer function was introduced. So, kernel documentation says that mod_timer is a more efficient way to update the expire field of an active timer (if the timer is inactive it will be activated). And if there are multiple unserialized concurrent users of the same timer, then mod_timer is the only safe way to modify the timeout. According to definition, mod_timer(timer, expires) is equivalent to:

del_timer(timer);
timer->expires = expires;
add_timer(timer);

What is the problem with using mod_timer for these kind of patterns?

It looks pretty obvious use, right?? But no, it is not. And reason is, it can be possible that instances of this pattern also modify the timer’s data and function fields which if the timer is still running and expiring while being fiddled with, might result in
a race condition. Which means that after the change there will be an old function but new data field being used in combination or something like that. So, it’s not a good idea to remove del_timer in the such cases and use mod_timer. One can look at the ROSE directory stack where such cases are present and we can’t use mod_timer there.

Is there any other pattern where mod_timer can be used?

Yes. I have seen some patches in sound directory from Takashi Iwai about introducing use of mod_timer. He introduced use of mod_timer for following kind of pattern:

timer->expires = expires;
add_timer(timer);

I looked in to those cases where he refactored such code with mod_timer. And came across this particular case. Here, in the later one he uses setup_timer and mod_timer. There is no del_timer, because the timer is not yet running. It looked little strange because the code is not modifying an existing timer, but rather starting up a new one. So, Julia told me to ask Takashi for the same. I mailed him and also asked for suggestion about doing this in general in the kernel. He replied that “The behavior of mod_timer() for the first invocation in this case is defined to behave just like add_timer(), so it’s correct to use it. But yes it isn’t always intuitive as a replacement of add_timer()”.

So thing is for this kind of pattern mod_timer is a matter of taste and it depends on maintainers. And I guess when it comes to adding any semantic patch or doing something in a kernel, one should always think about general cases. So, Julia advised me that it could be more useful to work on other functions/macros which really need some work. And I ended up with closing chapter of mod_timer here. 🙂

Macros with unnecessary __constant prefix

This is another set of macros on which I worked during last 2 weeks. Use of __constant_ macros decreased after linux 3.10. According to one research of Julia lawall on deprecated functions, there were 56 uses of __constant_htons in linux 3.10, but now there are 9 only. Basically there is complete set of __constant_ macros in byte order [little endian and big endian] header files. I was in a impression that major part of kernel which was using these macros is handled . I was not expecting that much cases primarily. So,at first my aim was to send patches for all of those remaining cases and then at the end get rid of __constant_ definitions completely. But I was surprised when I ran Coccinelle semantic patch over linux-next. I got around 30 files which are still using these functions. Now it was important to figure out that these macros are not actually handled by anyone or there is something for which these macros are working really good.

Why __constant prefix is unnecessary??

There are two definitions for each __constant_ macro. That is one for big endian case and one for little endian case. Now for example consider macro cpu_to_le16. In big endian cases, macro cpu_to_le16 unfolds to __swab16 which provides special case for constants. In little endian cases, __constant_cpu_to_le16 and cpu_to_le16 expand directly to the same expression. So, we can replace __constant_cpu_to_le16 with cpu_to_le16. On the contrary, for big endian byte order macros opposite happens. For example in little endian cases, macro cpu_to_be16 unfolds to __swab16 which provides special case for constants and in big endian cases, __constant_cpu_to_be16 and cpu_to_be16 expand directly to the same expression. That’s why __constant prefix is unnecessary and we can completely get rid of these macros.

How Coccinelle semantic patch can help in replacing __constant_ with ?

This is very simple case for Coccinelle to handle. One just need 1 metavariable and 1 branch for each transformation in a semantic patch. Here, is a complete semantic patch with all __constant_   macros:

@@expression x;@@

(
– __constant_htons(x)
+ htons(x)
|
– __constant_htonl(x)
+ htonl(x)
|
– __constant_ntohs(x)
+ htons(x)
|
– __constant_ntohl(x)
+ htonl(x)
|
– __constant_cpu_to_le64(x)
+ cpu_to_le64(x)
|
– __constant_le64_to_cpu(x)
+ le64_to_cpu(x)
|
– __constant_cpu_to_le32(x)
+ cpu_to_le32(x)
|
– __constant_le32_to_cpu(x)
+ le32_to_cpu(x)
|
– __constant_cpu_to_le16(x)
+ cpu_to_le16(x)
|
– __constant_le16_to_cpu(x)
+ le16_to_cpu(x)
|
– __constant_cpu_to_be64(x)
+ cpu_to_be64(x)
|
– __constant_be64_to_cpu(x)
+ be64_to_cpu(x)
|
– __constant_cpu_to_be32(x)
+ cpu_to_be32(x)
|
– __constant_be32_to_cpu(x)
+ be32_to_cpu(x)
|
– __constant_cpu_to_be16(x)
+ cpu_to_be16(x)
|
– __constant_be16_to_cpu(x)
+ be16_to_cpu(x)
)

Does all files compiles after this change?

Yes. They do. Only problem was with s390 directory. Although errors which we get while compiling s390 files has nothing to do with this change. But those errors are in included header files and when header files have errors gcc eventually stops from there. So, to be sure that this change is correct, I checked each s390 file with cross compiler.

I have sent some patches related to same. I hope there is nothing which I am missing in this case. And once those patches will be accepted, I will send all other patches and hopefully till the end of internship I will be done with getting rid of all __constant_ definitions and their uses. Important thing which I learned during this was cross compilation of s390. I will write about cross compilation process of s390 in my next blog along with some other architecture related stuff.

Till then stay tuned! 🙂

Setup_timer and its uses

So, today lets talk about setup_timer function as that is what I have been up to the past couple of weeks along with some other functions. Yes, I know I should have write about it 2 weeks ago but I was kind of busy with some other stuff along with my internship. 😦 That’s why didn’t find time to dedicate a proper blog to my work. Anyways, I am going to write 2 blogs about the work I did till now. In this blog I will write about setup_timer function and some interesting things I encountered while working with it.

Basically definition of setup_timer function unfolds to __setup_timer which goes like this:


#define __setup_timer(_timer, _fn, _data, _flags)
do {
__init_timer((_timer), (_flags));
(_timer)->function = (_fn);
(_timer)->data = (_data);
} while (0)

So, thing is we can use setup_timer instead of structure field assignments to initialize a timer. But its not always necessary that call to init_ and the initialization of the function & data fields occur in the order shown in the definition. It may possible that code can be something like this:

init_timer(&mp->mib_counters_timer);
mp->mib_counters_timer.data = (unsigned long)mp;
mp->mib_counters_timer.function = mib_counters_timer_wrapper;
[From file : drivers/net/ethernet/marvell/mv643xx_eth.c]

Actually for using setup_timer we can consider following cases:
1. init_timer first, function & data initialization after
a)function first then data
b)data first then function
2. function and data before, init_timer after
a)function first then data
b)data first then function
3. init_timer first then function (no data)
4. function first then data (no data)

Here, is a Coccinelle semantic patch:

@match_immediate_function_data_after_init_timer@
expression e, func, da;
@@

-init_timer(&e);
+setup_timer(&e, func, da);

(
-e.function = func;
-e.data = da;
|
-e.data = da;
-e.function = func;
)

@match_function_and_data_after_init_timer@
expression e1, e2, e3, e4, e5, a, b;
@@

-init_timer(&e1);
+setup_timer(&e1, a, b);

… when != a = e2
when != b = e3
(
-e1.function = a;
… when != b = e4
-e1.data = b;
|
-e1.data = b;
… when != a = e5
-e1.function = a;
)

@r1 exists@
identifier f;
position p;
@@

f(…) { … when any
init_timer@p(…)
… when any
}

@r2 exists@
identifier g != r1.f;
struct timer_list t;
expression e8;
@@

g(…) { … when any
t.data = e8
… when any
}

@script:python depends on r2@
p << r1.p;
@@

print “Data field initialized in another function. Dangerous to use setup_timer %s:%s” % (p[0].file,p[0].line)
cocci.include_match(False)

@r3@
expression e6, e7, c;
position r1.p;
@@

-init_timer@p(&e6);
+setup_timer(&e6, c, 0UL);
… when != c = e7
-e6.function = c;

There are 2 points about this semantic patch which I think I should write here:

  • Generally … matches with the code when there is something in between matching parts. But that doesn’t mean it do not match those code when there is nothing in between. Reason here we need this extra rule at the beginning is,  to increase the performance and speed. As when there are loops, semantic patch will take much more time than it is expected. Even with the timeout = 120 seconds, it was skipping some files. And adding one extra rule at the beginning was trivial and nicest solution of this problem. Although we are trying to look at things if we can do something else. I will update this blog if found something better. For now this works pretty fine.
  • For case 3 and 4, I observed some interesting patterns in code. We have some cases where init_ and function initialization are initialized in one function. And data is initialized in another function. In such cases it could be risky to use setup_timer. So, I just end up with one rule which matches this kind of code and gives warning. And then if no data field is found in entire file then it goes for writing data value = 0UL.

One more thing is, this is not whole semantic patch. As one can see, this is just part of it. It’s not covering all cases. But point here is this part covers most of  the cases[at least in a linux kernel] and other rules are pretty straight forward. There are two other functions, setup_timer_on_stack and setup_deferrable_timer_on_stack which can go with this semantic patch. I worked on setup_timer_on_stack and it is quite similar. So, not going to write much more about it.

By the way, I sent few patches on this and I am happy that those patches are accepted. I am going to send more. But yes still there are few points which I need to consider before adding this semantic patch in to the kernel. I think for now this is enough for this blog. I will update the blog with the complete semantic patch whenever this patch will be added in a kernel. 🙂