andy_shev Aug 6 2015 at 16:11

Сделаем код чище: Рефакторинг драйвера PCI для контроллера NAND Denali

4 min

5.8K

На примере драйвера PCI для контроллера NAND Denali я покажу как упрощается код при использовании макросов и функций-помощников, доступных в относительно свежих версиях ядра Linux.

Этот старый драйвер врядли используется, но является хорошим примером кода, который можно отрефакторить. Сам драйвер находится в 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.

Tags:

Hubs:

Open source