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@gmail.com>
---
 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;
+	}
 }

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@gmail.com>

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

On 24 May 2025 14:36:24 CEST, Damien Le Moal <dlemoal@kernel.org> wrote:
>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@gmail.com>
>
>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");

There is no VPRINTK() here since a long time ago.

So this must be based on some ancient kernel version.

Please base your patches on:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next


Kind regards,
Niklas
Hello Alexander,

Hello Alexander,

On Sun, May 25, 2025 at 12:10:07AM +0200, Niklas Cassel wrote:
> >> @@ -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");
> 
> There is no VPRINTK() here since a long time ago.
> 
> So this must be based on some ancient kernel version.
> 
> Please base your patches on:
> https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next

Sorry, but I forgot to mention, please don't use the In-Reply-To: header
to reference older versions of your patch.

To quote Submitting Patches, it creates an 'unmanageable forest':
https://docs.kernel.org/process/submitting-patches.html#explicit-in-reply-to-headers


Kind regards,
Niklas