Debugfs functions are not supposed to be checked

One of the common warnings that I see from Smatch is:

drivers/net/wireless/ath/ath5k/debug.c:985 ath5k_debug_init_device() warn: 'phydir' is an error pointer or valid

The idea behind this warning is that it’s important to know if a function returns error pointers or NULL. Sometimes functions return both and I have written about that before. But debugfs is a special case where the functions are not supposed to be checked for errors.

In my previous blog, I explained that when a function returns both error pointers and NULL then the error pointer is an error and the NULL is means the feature is disabled. NULL is a no-op, special kind of success. Debugfs started out as the reverse of that. NULL meant failure and error pointers mean disabled. This caused no end of confusion.

Then in commit ff9fb72bc077 (“debugfs: return error values, not NULL”), Debugfs was changed to always return error pointers if it wasn’t available (whether because it was disabled or because there was an allocation failure).

The thing about debugfs is that it was never supposed to be checked for errors. If debugfs_create_dir() returns an error pointer and we pass an error pointer “parent” to debugfs_create_file() then nothing bad happens. The error pointer is caught in start_creating() and the function is a no-op.

Debugfs functions will never go back to returning NULL on error. It was a mistake to do that. All that checking can safely be deleted. Deleting all the debugfs error handling is the correct way to address this.

To be honest, I had forgotten that checking for NULL was ever allowed. I think that in light of that, I will silence that warning. Otherwise the instinct that people have is to change the NULL check to an IS_ERR() check which breaks the driver when debugfs is disabled in the .config.

There are a few rare times where debugfs functions errors do need to be handled. This is when a driver is poking in debugfs internals. It’s not supposed to happen. And the probe() should not fail just because debugfs is disabled.

A second issue is that I should create a new warning when people check for IS_ERR(). This can be harmless when the KConfig ensures that debugfs is enabled. And I could check for that but all my test configs enable debugfs so it’s just easier to always warn about it.