On 5/20/25 00:13, Alexander Roman wrote:
> Problem:
This is not needed. It is clear that you are describing the problem.
> The current implementation of ahci_init_one has several issues with error handling
> and resource management. It lacks proper error cleanup paths, doesn't initialize
> pointers to NULL, and has inconsistent error handling patterns throughout the code.
> This can lead to resource leaks and make debugging initialization failures difficult.
>
> Solution:
Same here. It is clear you are describing how you solve the problem.
> This patch enhances the error handling and resource management in ahci_init_one by:
> - Adding comprehensive error checking with descriptive error messages
> - Improving error propagation through return codes
> - Adding proper error cleanup paths for all resource allocations
> - Initializing pointers to NULL to prevent use-after-free bugs
> - Implementing proper cleanup of allocated resources in error paths
> - Adding more descriptive error messages for all failure points
> - Including error codes in log messages for better diagnostics
> - Adding warning messages for potential system issues
> - Improving code structure with proper error handling paths
> - Adding proper error return labels
> - Making code more maintainable with consistent error handling patterns
>
> Technical Details:
Integrate that in the previous section please.
> - Added proper initialization of pointers (hpriv, host) to NULL
> - Added error cleanup paths with proper resource release
> - Improved error messages to include specific error codes
> - Added proper error handling for all resource allocation failures
> - Added proper cleanup of allocated resources in error paths
> - Improved code organization with clear error handling paths
> - Added proper error return labels for better code flow
>
> Note: Some error checks and logging have been simplified to reduce churn while
> maintaining robust error handling. The focus is on critical error paths and
> resource management rather than redundant checks. Log levels have been adjusted
> to use dev_warn for non-fatal warnings and dev_dbg for quirk failures.
>
> Signed-off-by: Alexander Roman <monderasdor@xxxxxxxxx>]
Please send patches to the maintainers too.
> ---
> drivers/ata/ahci.c | 150 ++++++++++++++++++++++++++++++-----------------------
> 1 file changed, 85 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1611,460 +1611,555 @@ 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 = -ENOMEM;
> - int n_ports, i, rc;
> u32 tmp, cap, port_map;
> u32 saved_cap;
> struct device *dev = &pdev->dev;
>
> VPRINTK("ahci_init_one enter\n");
This is old code. What is this patch against ???
This code does not exist since a long time ago. Please rebase.
>
> + /* acquire resources with proper error handling */
> - /* acquire resources */
The original comment was not useful. Making it more detailed still does not make
it useful as it is very clear what this code hunk does. I would be more in favor
of completely dropping this useless comment.
> rc = pcim_enable_device(pdev);
> if (rc) {
> + dev_err(dev, "Failed to enable PCI device: %d\n", rc);
> + goto err_out;
Wrong indentation.
> - return rc;
> }
>
> rc = pcim_iomap_regions(pdev, 1 << AHCI_PCI_BAR_STANDARD, DRV_NAME);
> if (rc) {
> + dev_err(dev, "Failed to map PCI regions: %d\n", rc);
> + goto err_out;
> - return rc;
> }
> 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) {
> + dev_err(dev, "Failed to allocate IRQ vectors: %d\n", rc);
> + goto err_out;
> - return rc;
> }
>
> + /* allocate and initialize host private data */
Same here: comment not needed. It is clear what the code does. Please only add
comments for describing things that are not obvious.
> hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL);
> if (!hpriv) {
> + dev_err(dev, "Failed to allocate host private data\n");
> + goto err_out;
> - return -ENOMEM;
> }
>
> hpriv->mmio = mmio;
> hpriv->flags = (unsigned long)ent->driver_data;
> hpriv->irq = pdev->irq;
>
> + /* apply board quirks */
> if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> + rc = ahci_intel_pcs_quirk(pdev, hpriv);
> + if (rc) {
> + dev_dbg(dev, "Intel PCS quirk failed (%d)\n", rc);
> + goto err_host;
> + }
> - ahci_intel_pcs_quirk(pdev, hpriv);
> }
>
> + /* apply port map mask if present */
Comment not needed.
> ahci_get_port_map_mask(dev, hpriv);
>
> + /* save initial config */
Here too.
> rc = ahci_pci_save_initial_config(pdev, hpriv);
> if (rc) {
> + dev_err(dev, "Failed to save initial configuration: %d\n", rc);
> + goto err_out;
> - return rc;
> }
>
> + /* prepare host */
Same.
> 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) {
> + dev_err(dev, "Failed to allocate ATA host\n");
> + goto err_out;
> - return -ENOMEM;
> }
>
> host->private_data = hpriv;
>
> + /* configure DMA masks */
Again...
> rc = ahci_configure_dma_masks(pdev, hpriv);
> if (rc) {
> + dev_err(dev, "Failed to configure DMA masks: %d\n", rc);
> + goto err_host;
> - return rc;
> }
>
> + /* initialize adapter */
And again... I stop here. You see my point. The function names are clear enough...
> ahci_pci_init_controller(host);
> rc = ahci_reset_controller(host);
> if (rc) {
> + dev_err(dev, "Failed to reset controller: %d\n", rc);
> + goto err_host;
> - return rc;
> }
>
> + /* apply fixups for broken systems */
> if (ahci_broken_system_poweroff(pdev)) {
> + dev_warn(dev, "System may need power cycle after shutdown\n");
> - dev_info(dev, "quirky BIOS, skipping spindown on poweroff\n");
> }
>
> + /* configure LPM policy */
> for (i = 0; i < n_ports; i++) {
> ahci_update_initial_lpm_policy(host->ports[i]);
> }
>
> + /* apply platform-specific workarounds */
> if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> + rc = ahci_intel_pcs_quirk(pdev, hpriv);
> + if (rc) {
> + dev_dbg(dev, "Intel PCS quirk failed (%d)\n", rc);
> + goto err_host;
> + }
> - ahci_intel_pcs_quirk(pdev, hpriv);
> }
>
> + /* apply Apple MCP89 workaround */
> if (is_mcp89_apple(pdev)) {
> + rc = ahci_mcp89_apple_enable(pdev);
> + if (rc) {
> + dev_err(dev, "Failed to enable MCP89 Apple: %d\n", rc);
> + goto err_host;
> + }
> - ahci_mcp89_apple_enable(pdev);
> }
>
> + /* apply Acer SA5-271 workaround */
> acer_sa5_271_workaround(hpriv, pdev);
>
> + /* initialize and enable interrupts */
> ahci_init_irq(pdev, n_ports, hpriv);
> ahci_pci_enable_interrupts(host);
>
> + /* print information */
> ahci_pci_print_info(host);
>
> + /* register with libata */
> rc = ata_host_activate(host, hpriv->irq, ahci_interrupt, IRQF_SHARED,
> + &ahci_sht);
> - &ahci_sht);
> if (rc) {
> + dev_err(dev, "Failed to activate ATA host: %d\n", rc);
> + goto err_host;
> - return rc;
> }
>
> return 0;
>
> +err_host:
> + ata_host_detach(host); // host is NULL-checked internally
> +err_out:
> + return rc;
> - return 0;
> }
>
--
Damien Le Moal
Western Digital Research
