On 5/22/25 12:26, Alexander Roman wrote:
> Add comprehensive error handling to ahci_init_one() to:
> 1. Prevent resource leaks during initialization failures
> 2. Ensure proper cleanup of allocated resources
> 3. Provide detailed error reporting for debugging
> 4. Maintain consistent error handling patterns
>
> Key changes:
> - Initialize all pointers to NULL
> - Add centralized error handling via goto labels
> - Improve error messages with specific error codes
> - Remove duplicate Intel PCS quirk call
> - Adjust log levels (dev_err for fatal, dev_dbg for quirks)
>
> Signed-off-by: Alexander Roman <monderasdor@xxxxxxxxx>
I received 2 x v3 patches with different commit messages and titles, but these 2
patches touch the same code.. Very confusing...
Which one is the "correct" patch you want us to consider ?
And please send patches to *all* maintainers of the subsystem.
You can check that with "scripts/get_maintainer.pl driver/ata"
(you are missing Niklas).
Note: it is too late to apply this patch anyway. If accepted, it will go in
during 6.16-rc1. So no rush to clean this up. Take your time and make a proper
patch please.
> ---
> drivers/ata/ahci.c | 98 ++++++++++++++++++++++++++--------------------
> 1 file changed, 55 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index abc1234..def5678 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1611,7 +1611,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> struct ahci_host_priv *hpriv = NULL;
> struct ata_host *host = NULL;
> void __iomem *mmio = NULL;
> - int n_ports, i, rc;
> + int n_ports, i, rc = -ENOMEM;
> u32 tmp, cap, port_map;
> u32 saved_cap;
> struct device *dev = &pdev->dev;
> @@ -1619,60 +1619,72 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> VPRINTK("ahci_init_one enter\n");
>
> rc = pcim_enable_device(pdev);
> - if (rc)
> - return rc;
> + if (rc) {
> + dev_err(dev, "failed to enable PCI device (err=%d)\n", rc);
> + goto err_out;
> + }
>
> rc = pcim_iomap_regions(pdev, 1 << AHCI_PCI_BAR_STANDARD, DRV_NAME);
> - if (rc)
> - return rc;
> + if (rc) {
> + dev_err(dev, "failed to map PCI regions (err=%d)\n", rc);
> + goto err_out;
> + }
> mmio = pcim_iomap_table(pdev)[AHCI_PCI_BAR_STANDARD];
>
> rc = pci_alloc_irq_vectors(pdev, 1, AHCI_MAX_PORTS, PCI_IRQ_ALL_TYPES);
> - if (rc < 0)
> - return rc;
> + if (rc < 0) {
> + dev_err(dev, "failed to allocate IRQ vectors (err=%d)\n", rc);
> + goto err_out;
> + }
>
> hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL);
> - if (!hpriv)
> - return -ENOMEM;
> + if (!hpriv) {
> + dev_err(dev, "failed to allocate host private data\n");
> + goto err_out;
> + }
>
> hpriv->mmio = mmio;
> hpriv->flags = (unsigned long)ent->driver_data;
> hpriv->irq = pdev->irq;
>
> if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> - ahci_intel_pcs_quirk(pdev, hpriv);
> + rc = ahci_intel_pcs_quirk(pdev, hpriv);
> + if (rc)
> + dev_dbg(dev, "Intel PCS quirk failed (err=%d)\n", rc);
> }
>
> ahci_get_port_map_mask(dev, hpriv);
>
> rc = ahci_pci_save_initial_config(pdev, hpriv);
> - if (rc)
> - return rc;
> + if (rc) {
> + dev_err(dev, "failed to save initial config (err=%d)\n", rc);
> + goto err_out;
> + }
>
> cap = hpriv->cap;
> saved_cap = cap;
> port_map = hpriv->port_map;
> n_ports = ahci_calc_n_ports(cap, port_map);
>
> host = ata_host_alloc_pinfo(dev, ahci_port_info + ent->driver_data, n_ports);
> - if (!host)
> - return -ENOMEM;
> + if (!host) {
> + dev_err(dev, "failed to allocate ATA host\n");
> + goto err_out;
> + }
>
> host->private_data = hpriv;
>
> rc = ahci_configure_dma_masks(pdev, hpriv);
> - if (rc)
> - return rc;
> + if (rc) {
> + dev_err(dev, "failed to configure DMA masks (err=%d)\n", rc);
> + goto err_host;
> + }
>
> ahci_pci_init_controller(host);
> rc = ahci_reset_controller(host);
> - if (rc)
> - return rc;
> -
> - if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> - ahci_intel_pcs_quirk(pdev, hpriv);
> + if (rc) {
> + dev_err(dev, "failed to reset controller (err=%d)\n", rc);
> + goto err_host;
> }
>
> if (ahci_broken_system_poweroff(pdev)) {
> @@ -1685,20 +1697,20 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> }
>
> if (is_mcp89_apple(pdev)) {
> - ahci_mcp89_apple_enable(pdev);
> + rc = ahci_mcp89_apple_enable(pdev);
> + if (rc)
> + dev_warn(dev, "Apple MCP89 enable failed (err=%d)\n", rc);
> }
>
> - acer_sa5_271_workaround(hpriv, pdev);
> -
> ahci_init_irq(pdev, n_ports, hpriv);
> ahci_pci_enable_interrupts(host);
>
> ahci_pci_print_info(host);
>
> rc = ata_host_activate(host, hpriv->irq, ahci_interrupt, IRQF_SHARED,
> - &ahci_sht);
> - if (rc)
> - return rc;
> -
> - return 0;
> + &ahci_sht);
> + if (rc) {
> + dev_err(dev, "failed to activate ATA host (err=%d)\n", rc);
> + goto err_host;
> + }
> }
>
--
Damien Le Moal
Western Digital Research
