Typical mistakes when submitting a new code to Linux kernel

Andy Shevchenko, Espoo, Finland

LVEE 2017

Code review is a crucial part of developing a stable and robust open source project, such as Linux kernel. New comers and surprisingly many active and known contributors have been doing same mistakes, like using non-scalable APIs, over and over. How can we eliminate or avoid them? Looking at the patterns the best practices would be suggested. In addition, relatively new Linux kernel APIs, such as Unified Device Properties, will be discussed.

Introduction

According to the Linux kernel v4.11 development statistics ¹ the code base is grown by nearly 300,000 lines. It means that from now on many of new drivers and other modules are in the vanilla kernel. While performing review on some of the drivers I have noticed that even oldbies have been making same mistakes over and over when submitting a new code. Majority of the issues is poor knowledge of the library APIs in the kernel. For drivers it includes, for example, Device Resource Management API ² ³ or Unified Device Properties API ⁴, while other modules re-implement simple helper functions, like hex2bin(), from the internal library. Besides that driver developers often are focusing on single CPU architecture or hardware platform which makes the code not so scalable. In the following chapters the most occurred mistakes will be considered. In some cases the non-obvious issues would be discussed, e.g. devm_request_threaded_irq() coexistence together with tasklets and 64-bit hardware registers access and handling.

Device Resource Management

Device Resource Management API ² ³ is quite old idea (it dates back to 2007!), that is still evolving, though developers don’t use or use it in a suboptimal way.

The most recent example I have met in ⁵ is the following piece of code:

    ret = of_address_to_resource(np, 0, &res_xbar);
    if (ret) {
        dev_err(dev, "Failed to get xbar resources");
        return ret;
    }
    if (!devm_request_mem_region(dev, res_xbar.start, resource_size(&res_xbar), res_xbar.name)) {
        dev_err(dev, "Failed to get xbar resources");
        return -ENODEV;
    }
    xbar_membase = devm_ioremap_nocache(dev, res_xbar.start, resource_size(&res_xbar));
    if (!xbar_membase) {
        dev_err(dev, "Failed to remap xbar resources");
        return -ENODEV;
    }

Obviously the developer is not familiar with non-device tree platforms and Device Resource Management API. The better approach is just missed, i.e.

    res_xbar = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    xbar_membase = devm_ioremap_resource(dev, res_xbar);
    if (IS_ERR(xbar_membase))
        return PTR_ERR(xbar_membase);

In the end we got much simpler code, resource provider agnostic — it will work on Device Tree, ACPI, or legacy platform data enabled platforms.

Of course there are some nuances when the driver is converted to new API. One of them is a good understanding when it’s safe to use devm_request_threaded_irq(). For example, if driver is using tasklets the IRQ handler should be removed before tasklets. It will guarantee that no more tasks are scheduled on non-existing data.

Nevertheless previously mentioned examples maybe are not the best ones when we speak about hardware IP blocks which would have been using on various CPU architectures and platforms, where Unified Device property API is a big helper.

Unified Device Properties

Back to 2014 Rafael Wysocki had submitted a new common API ⁴ for drivers to get device properties in resource provider agnostic way. It means legacy platform data, ACPI, or Device Tree resource providers do not require special handling anymore in the drivers.

Besides that few other subsystems, i.e. GPIO, I2C, SPI, were updated accordingly to hide resource provider details in their core parts.

One of the recent and nice example of conversion to new API is the commit 95129b6f0806 (“NFC: pn544: Get rid of code duplication in →probe()”) ⁶ along with few preparations in vanilla kernel.

While the above is related mostly to the drivers, the small parts of the rest of the code might have been reinventing the wheel, such as hex2bin() helper or even more often helpers to dump small buffers in hexadecimal or escaped format.

Special extensions of %p

How often do you need to dump several bytes from the buffer in the log to reveal some details about an error? I’m pretty sure it have been happening sometimes. Linux kernel vsnprintf() and thus its derivatives implements special extensions of %p specifier ⁷. In particular, instead of reinventing hexdump functionality the developer may just use something like:

    u8 data[100];
    ...
    pr_debug("Buf: %*phC\n", (int)sizeof(data), data); /* only first 64 bytes! */

That code will dump first 64 bytes in hexadecimal format with ‘:’ (colon) as a delimiter.

There are also many other extensions, e.g. printing buffer in escaped format, IPv4 and IPv6 addresses, MAC addresses, architecture dependent types, device resources and so on.

Recommendations on how prepare changes to Linux kernel

When developer consider the code tested and ready for submission the last things to be worth doing are:

Check the code again for duplication.
Take the material from the above chapters into consideration when doing drivers.
Establish internal mailing list for review and review process if it’s not done yet.
If in doubt, feel free to ask.