На примере драйвера PCI для контроллера NAND Denali я покажу как упрощается код при использовании макросов и функций-помощников, доступных в относительно свежих версиях ядра Linux.
Этот старый драйвер врядли используется, но является хорошим примером кода, который можно отрефакторить. Сам драйвер находится в drivers/mtd/nand/denali_pci.c.
Первым делом коснёмся Kconfig. Поскольку драйвер разбит по уже классической схеме, а именно: основная часть + драйвер на шине, следуя логике (в том числе и самого Торвальдса) нам необходимо скрыть выбор основной части от пользователя. Попутно заменяем пробелы на табуляции в тех строках, которые изменяем.
Делай раз!
Следующим шагом будет применение макроса
Делай два!
Теперь переходим к самому интересному, замене вызовов на управляемые ресурсы (вот здесь я описывал их).
Начнём с простого,
Поскольку устройство подключено к шине PCI, воспользуемся devres API для устройств PCI:
К сожалению избавиться от
Объединяем полученное в этой главе, и делай три!
Для наведения полной красоты заменяем
Делай четыре!
Подведём итоги:
Очевидная польза. Не забываем использовать доступные API в новом коде!
UPDATE.
Все 4 уже в дереве мейнтейнера. Commit IDs: af83a67cad14, add243d5bc37, 2445d33d8523, 04868a67ed58.
Этот старый драйвер врядли используется, но является хорошим примером кода, который можно отрефакторить. Сам драйвер находится в drivers/mtd/nand/denali_pci.c.
Подготовка Kconfig
Первым делом коснёмся Kconfig. Поскольку драйвер разбит по уже классической схеме, а именно: основная часть + драйвер на шине, следуя логике (в том числе и самого Торвальдса) нам необходимо скрыть выбор основной части от пользователя. Попутно заменяем пробелы на табуляции в тех строках, которые изменяем.
Делай раз!
Применение макросов
Следующим шагом будет применение макроса
module_pci_driver():--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -129,14 +129,4 @@ static struct pci_driver denali_pci_driver = {
.remove = denali_pci_remove,
};
-static int denali_init_pci(void)
-{
- return pci_register_driver(&denali_pci_driver);
-}
-module_init(denali_init_pci);
-
-static void denali_exit_pci(void)
-{
- pci_unregister_driver(&denali_pci_driver);
-}
-module_exit(denali_exit_pci);
+module_pci_driver(denali_pci_driver);
Делай два!
Переход на API управляемых ресурсов
Теперь переходим к самому интересному, замене вызовов на управляемые ресурсы (вот здесь я описывал их).
Начнём с простого,
devm_kzalloc():Смотрим что получилось
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -35,14 +35,14 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
unsigned long csr_len, mem_len;
struct denali_nand_info *denali;
- denali = kzalloc(sizeof(*denali), GFP_KERNEL);
+ denali = devm_kzalloc(&dev->dev, sizeof(*denali), GFP_KERNEL);
if (!denali)
return -ENOMEM;
ret = pci_enable_device(dev);
if (ret) {
pr_err("Spectra: pci_enable_device failed.\n");
- goto failed_alloc_memery;
+ return ret;
}
if (id->driver_data == INTEL_CE4100) {
@@ -103,9 +103,6 @@ failed_req_regions:
pci_release_regions(dev);
failed_enable_dev:
pci_disable_device(dev);
-failed_alloc_memery:
- kfree(denali);
-
return ret;
}
@@ -119,7 +116,6 @@ static void denali_pci_remove(struct pci_dev *dev)
iounmap(denali->flash_mem);
pci_release_regions(dev);
pci_disable_device(dev);
- kfree(denali);
}
Поскольку устройство подключено к шине PCI, воспользуемся devres API для устройств PCI:
Смотрим что получилось
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -30,7 +30,7 @@ MODULE_DEVICE_TABLE(pci, denali_pci_ids);
static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
- int ret = -ENODEV;
+ int ret;
resource_size_t csr_base, mem_base;
unsigned long csr_len, mem_len;
struct denali_nand_info *denali;
@@ -39,7 +39,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
if (!denali)
return -ENOMEM;
- ret = pci_enable_device(dev);
+ ret = pcim_enable_device(dev);
if (ret) {
pr_err("Spectra: pci_enable_device failed.\n");
return ret;
@@ -70,14 +70,13 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
ret = pci_request_regions(dev, DENALI_NAND_NAME);
if (ret) {
pr_err("Spectra: Unable to request memory regions\n");
- goto failed_enable_dev;
+ return ret;
}
denali->flash_reg = ioremap_nocache(csr_base, csr_len);
if (!denali->flash_reg) {
pr_err("Spectra: Unable to remap memory region\n");
- ret = -ENOMEM;
- goto failed_req_regions;
+ return -ENOMEM;
}
denali->flash_mem = ioremap_nocache(mem_base, mem_len);
@@ -99,10 +98,6 @@ failed_remap_mem:
iounmap(denali->flash_mem);
failed_remap_reg:
iounmap(denali->flash_reg);
-failed_req_regions:
- pci_release_regions(dev);
-failed_enable_dev:
- pci_disable_device(dev);
return ret;
}
@@ -114,8 +109,6 @@ static void denali_pci_remove(struct pci_dev *dev)
denali_remove(denali);
iounmap(denali->flash_reg);
iounmap(denali->flash_mem);
- pci_release_regions(dev);
- pci_disable_device(dev);
}
К сожалению избавиться от
ioremap_nocache() не представляется возможным, нет соответствующего устройства под рукой (да и я вообще не уверен, что такое существует в мире сегодня) чтобы проверить, какая информация хранится в соответствующих PCI BAR'ах.Объединяем полученное в этой главе, и делай три!
Дополнительный рефакторинг
Для наведения полной красоты заменяем
pr_err() на dev_err():--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -41,7 +41,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
ret = pcim_enable_device(dev);
if (ret) {
- pr_err("Spectra: pci_enable_device failed.\n");
+ dev_err(&dev->dev, "Spectra: pci_enable_device failed.\n");
return ret;
}
@@ -69,19 +69,19 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
ret = pci_request_regions(dev, DENALI_NAND_NAME);
if (ret) {
- pr_err("Spectra: Unable to request memory regions\n");
+ dev_err(&dev->dev, "Spectra: Unable to request memory regions\n");
return ret;
}
denali->flash_reg = ioremap_nocache(csr_base, csr_len);
if (!denali->flash_reg) {
- pr_err("Spectra: Unable to remap memory region\n");
+ dev_err(&dev->dev, "Spectra: Unable to remap memory region\n");
return -ENOMEM;
}
denali->flash_mem = ioremap_nocache(mem_base, mem_len);
if (!denali->flash_mem) {
- pr_err("Spectra: ioremap_nocache failed!");
+ dev_err(&dev->dev, "Spectra: ioremap_nocache failed!");
ret = -ENOMEM;
goto failed_remap_reg;
}
Делай четыре!
Подведём итоги:
drivers/mtd/nand/Kconfig | 13 +++++--------
drivers/mtd/nand/denali_pci.c | 43 +++++++++++--------------------------------
2 files changed, 16 insertions(+), 40 deletions(-)
Очевидная польза. Не забываем использовать доступные API в новом коде!
UPDATE.
Все 4 уже в дереве мейнтейнера. Commit IDs: af83a67cad14, add243d5bc37, 2445d33d8523, 04868a67ed58.
amarao
Спасибо.
А теперь вопрос: драйвер-то после изменений работает? То есть меня всегда удивляло, как можно рефакторить код, который не покрыт тестами.
andy_shev
Конкретно этот компилируется, а в целом тестируется на реальном железе обычно, так что в других случаях, например, в этом, драйвер работает на MinnowBoard (v1) или вот этот на Galileo, хотя в другом закралась ошибка, исправленная позднее.
amarao
Я сейчас даже не наезжаю на тему «рефакторите то, что не можете проверить», а мне интересно, как это вообще в комьюнити работает?
Допустим, приходит архитектор и говорит: давайте вместо стопятьсот строк кода будут вот эти три — одинаковые и удобные. И как бы да. Но теперь надо в тысячах драйверов это поправить. Допустим, энтузиазм таков, что поправляют. Но дальше-то… Кто проверяет, что при этом драйвера работают? Производители железа ровными рядами идут тестировать изменения? А если кто-то не простестировал?
К чему я? Как в ядре решается вопрос с тестированием кода драйверов? Я вот даже придумать ничего не могу, кроме как гигантских рядов с стендами и разными железками…
andy_shev
Давайте я в свободное время подготовлю развёрнутый ответ на этот вопрос в виде статьи. Поля для комментария явно мало.
amarao
Десять рук за и заранее спасибо.
Потому что даже в отрыве от Ядра, я не понимаю, как делать тестирование кода, который сильно зависит от внешнего мира, который тяжело мокать.
bitterman
кому девайс нужен — тот тестирует. для остальных критерий работоспособности этого участка ядра (неиспользуемого у 99.99% пользователей) — его компилируемость.
в общем, кому надо — жалуется :-)