Geekly Articles each Day
📜 ⬆️ ⬇️

Making the code cleaner: Refactoring the PCI driver for a NAND Denali controller

Using the example of a PCI driver for a NAND Denali controller, I will show how simplified the code is when using macros and helper functions available in relatively recent versions of the Linux kernel.

This old driver is hardly used, but is a good example of code that can be refactored. The driver itself is located in drivers / mtd / nand / denali_pci.c .

Kconfig preparation


First of all let's touch Kconfig. Since the driver is broken according to the already classical scheme, namely: the main part + driver on the bus, following the logic (including Torvalds himself ), we need to hide the choice of the main part from the user. Along the way, we replace spaces on tabs in those lines that we change.
')
Do it once!

Application macros


The next step is to use the module_pci_driver() macro:

 --- 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);


Do two!

Transition to Managed Resource API


Now we come to the most interesting, replacing calls with managed resources ( here I described them).

Let's start with a simple, devm_kzalloc() :

Look what happened
 --- 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); }



Since the device is connected to the PCI bus, we use the devres API for PCI devices:

Look what happened
 --- 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); }


Unfortunately, it is not possible to get rid of ioremap_nocache() , there is no appropriate device at hand (and I’m not at all sure that such a thing exists in the world today) to check what information is stored in the corresponding PCI BARs.

Combine the resulting in this chapter, and do three!

Additional refactoring


To restore complete beauty, replace pr_err() with 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; }


Make four!

Let's sum up:

  drivers/mtd/nand/Kconfig | 13 +++++-------- drivers/mtd/nand/denali_pci.c | 43 +++++++++++-------------------------------- 2 files changed, 16 insertions(+), 40 deletions(-)


The obvious good. Do not forget to use the available API in the new code!

UPDATE.
All 4 are already in the maintainer tree . Commit IDs: af83a67cad14, add243d5bc37, 2445d33d8523, 04868a67ed58.

Source: https://habr.com/ru/post/264289/

More articles:


All Articles