Devm Patches: Successes and Issues Encountered

Over the last 2 weeks I have learnt that there are many managed interface and of different types. I would like to share my experience with patches till now. I have tried to add links wherever possible for someone who is more interested in the discussions and patches.

Let me start with some good news. The following patches have been applied to the kernel :).

Now my journey has seen some issues as some of the devm functions are a bit complex and the corresponding free/unregister function need to be seen very carefully to decide whether to do away with them or not. Otherwise, we could cause the terrible memory leaks or dangerously introduce a use after resource deallocation which can immediately crash the kernel unless the associated structure has been reallocated.

  1.  drivers/input/misc/pcf50633-input.c : In this case, I had just changed one of the resources to be allocated using devm function which was earlier allocated with kzalloc(). Now, Dimitry pointed out that mixing managed and unmanaged resources can be dangerous as it is really hard to track which one should be freed and which will be freed automatically. On further investigation I found input_allocate_device() function has a corresponding devm_input_allocate_device() function but there were other functions  like kobject_get_path() which don’t have corresponding managed interface and hence the driver was skipped.
  2. drivers/tty/hvc/hvc_opal.c : In this there was a case where a return was being done without kfree() of resources preciously allocated using kzalloc. Shifting to devm_kzalloc would have changed the workflow, but it appeared more like a memory leak bug and asked a query to the maintainers. I got a reply that one of the entries can be populated with a pointer to a static instance (hvc_opal_boot_priv) for the early boot/debug console and shouldn’t be freed. Hence, the error path was allowed to leak.
  3.  drivers/input/serio/apbps2.c : In this case,  in the remove function there was a call to serio_unregister_port.  This function uses the kobject interface that is complicated and seems to ultimately free its argument. Looking at some other calls to serio_register_port and serio_unregister_port, it is found that they also use kzalloc and no kfree. So, in conclusion, it is a good idea to be suspicious if there is no kfree anywhere for the data.  It could of course be a memory leak, but it could also be that the data should not be kfreed.
  4. drivers/dma/iopadma.c : In the remove function, a list of iop_chans is being freed and then the device. So, apparently there is a list of iop_chans per device. The probe function also makes a list for the channels, and makes one iop_chan, and puts it in the list. But the fact that there is an iteration (list_for_each_entry_safe) suggests that there can be other elements of this list.  It is unclear as to how these elements are added. If they are added somewhere else with kzalloc, then we can’t devm the one added here, but not the other ones. I have asked  a query to the maintainers and am waiting for an answer.
  5. drivers/input/misc/sparcspkr.c : This could only be partially converted to use devm for resource allocation due to absence of a devm version of of_ioremap. A solution could be to introduce a devm version of the of_ioremap function as it is used commonly. A query has been asked to the maintainer and I am waiting for an answer.
  6. drivers/misc/eeprom/sunxi_sid.c : The resource entropy was being devmified but entropy is a buffer that is just allocated and freed within the normal execution of the function. The buffer is to be freed even when the probe function succeeds.  A devm buffer would only be freed if the probe function fails.  Otherwise, it will be kept around until the remove function succeeds.
  7. drivers/input/misc/mc13783-pwrbutton.c : The probe function used mc13xxx_irq_request which is not devm’d, and doing so is complicated due to the locks around the frees.  So, it was left to avoid mixing two styles.

The following patches though not yet accepted have been approved by the maintainers are expected to be accepted soon. Fingers crossed!!

Other patches submitted:

Advertisements

Move resources allocated using unmanaged interface to managed devm interface

So today let’s talk about devm functions as that is what I have been upto the past couple of weeks. Yes, should have finished the task by now but due to some reasons was not active a couple of days :(.
Now what are devm functions. 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 path or a driver detach. Else, the driver will be leaking resources and wasting precious main memory!
Detection of such memory leaks is difficult as they not clearly noticeable. So, managed interfaces have been created for common resources used by drivers. They have a name similar to the corresponding unmanaged function making it easy to identify e.g.:  dma_alloc_coherent() and dmam_alloc_coherent(), kzalloc() and devm_kzalloc(). If a managed function is used for memory allocation the resources are guaranteed to be freed both on initialization failure and driver detachment. Many of these managed functions are devm functions. eg: devm_kzalloc, devm_iio_device_alloc().
My task has been mainly to identify the cases where kzalloc() can be changed to devm_kzalloc() and the corresponding kfree()s can be done away with. Then the changes were applied and minor code changes like removal of labels , removing of brackets in single statement ifs were done. I have been using a Coccinelle semantic patch to do this task and have made tweaks to handle some specific cases. Here is a major part of the Coccinelle semantic patch making the change:

@platform@
identifier p, probefn, removefn;
@@
struct platform_driver p = {
.probe = probefn,
.remove = removefn,
};

@prb@
identifier platform.probefn, pdev;
expression e, e1, e2;
@@
probefn(struct platform_device *pdev, …) {
<+…
– e = kzalloc(e1, e2)
+ e = devm_kzalloc(&pdev->dev, e1, e2)

?-kfree(e);
…+>
}

@rem depends on prb@
identifier platform.removefn;
expression e;
@@
removefn(…) {
<…
– kfree(e);
…>
}

There are many cases in which a lot of code goes away and the probe and remove functions appear really simple and easier to understand.
At the same time, there are issues such as cases to which the patch applies but does not benefit much as there are major resources that are still unmanaged. Also, some more advanced functions that can be managed like IRQs have not been handled at this stage.

Thanks to OPW.

Dreams do come true. This statement came true for me on the night of 21st March when I did a Ctrl+F for my name on the selection list of OPW and the result was 1 match! Yeah.. I was in. I would be learning this summers while contributing to the Linux Kernel. Yes, it is the same kernel that is the basis of the operating system used by most people at my university or by lakhs of Linux users worldwide. Yes it the free and open-source operating system that we hear about all the time. I have been given the opportunity to contribute to the kernel, to help make some changes that may make the operating system less buggy, more compatible to variety of softwares and hence improve the user experience for the Linux users worldwide. I will put my heart and soul into this project, learn for the sake of knowledge and work for the betterment of the software. I express my heartfelt thanks to the mentors for there faith in me and the help and guidance they have always given.

The main purpose of this blog will be to track my progress through my project Coccinelle. I will probably write about any issues or interesting technical subjects I encounter through the project, provided they are related to FOSS.

The content of this blog will be mostly technical. Hope you enjoy the read and find some cool stuff here!!