Bad Style in Mali Utgard Kernel Code

"Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live." - Martin Golding

Introduction

For a whole year I worked on a modification of the Mali Utgard kernel driver for Linux. Mali is a name of GPU series produced by ARM. I worked with a set of revisions of this driver and found some corner change in the approach ARM engineers had used for development just between the revisions R5P1 and R5P2.

New Revisions Considered Harmful

Shortly, it looks like that some time ago one guy or a group of people have developed a coding style solving a number of real production problems. Some of rules, as I can assume, were made to allow development of the driver with low-experienced engineers to minimize possible bugs, which can be done in C language almost everywhere by a programmer who doesn't know what he or she does. Yes, this coding style was different to the one used in mainline Linux kernel but it was not so bad. Moreover, I think that the driver of earlier revisions had many simple but interesting places which will be described in the next note. But almost everything was changed in the revision R5P2.

R5P2 is a first revision which has introduced some new function related to memory management. This function was COW, or Copy-On-Write. The idea follows just from the name, you can read a Wikipedia article to get this idea if you are not familiar with this. Of course this was an interesting optimization made for allocations the driver does by request of a user space library to keep objects of graphical scene. But it was done with a crazy bad approach in mind! Some parts of the solution still look for me so strange and non-logical that I don't want to think about the reasons to write such things right this way.

Moreover the code is not generic in some sense and any modification based on it requires changes in the underlying code. SMP synchronization is done with so many assumptions in mind that it results to race conditions and incorrect lifetime of objects when one of these assumptions is broken.

There are too many places which torture me. Let's return to a simple matter, coding style, forget about code quality.

That style, developed by creators of earlier versions was violated, not just in a number of places, everywhere! New code uses more Linux kernel-friendly spacing and naming but with crazy Windows-style line feed codes "\r\n". Some files still have normal "\n" at ends of lines, some were created and edited in Windows and the result was not properly filtered before publication of the source code. All the cool OS-independent things like generic wrappers around synchronization primitives, deadlock detection tool watching lock ordering and so on were thrown away as garbage. No, they are in the source code, but new synchronization primitives in memory manager and COW implementation don't fit to this infrastructure, don't use any wrappers carefully added by the predecessors, everything just spits on the good basis made by old engineers.

Understand me, I do not want to offend anybody, but publication of such code is a disrespect of other developers. This is not a kind of side patchset developed by some hacker to show a proof-of-concept. This is a real product of a huge company. This company has all necessary documentation on hardware and software they develop - not all the partners of ARM have such privileges. They have own engineers in headquarters and I think these engineers must be super-specialists because they are creators of the product used by incredible amount of people in mobile devices.

Negative Effects

My own changes needed to be made on top of R5P2's upstream modifications and of course they absorbed some issues from this underlying code. Some things were modified little bit to allow at least possibility of necessary modifications. But I had many annoying evenings at work because of the problems described above.

Harmful Modifications

To see all the things I am sad about just download some revision newer than R5P1 from the page related to Utgard Linux kernel driver. This is open source and distributed under GPL v2.

Just try to compare old files with the new ones:

Some new files:
driver/src/devicedrv/mali/linux/mali_memory_virtual.c
driver/src/devicedrv/mali/linux/mali_memory_virtual.h
driver/src/devicedrv/mali/linux/mali_memory_cow.c
driver/src/devicedrv/mali/linux/mali_memory_cow.h
driver/src/devicedrv/mali/linux/mali_memory_manager.c
driver/src/devicedrv/mali/linux/mali_memory_manager.h

Some modified ones:
driver/src/devicedrv/mali/linux/mali_memory_types.h

Conclusion