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

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s