[PATCH] ata: libata-acpi: Do not assume 40 wire cable if no devices are enabled

[PATCH] ata: libata-acpi: Do not assume 40 wire cable if no devices are enabled

[Tasos Sahanidis]

On at least an ASRock 990FX Extreme 4 with a VIA VT6330, the devices
have not yet been enabled by the first time ata_acpi_cbl_80wire() is
called. This means that the ata_for_each_dev loop is never entered,
and a 40 wire cable is assumed.

The VIA controller on this board does not report the cable in the PCI
config space, thus having to fall back to ACPI even though no SATA
bridge is present.

The _GTM values are correctly reported by the firmware through ACPI,
which has already set up faster transfer modes, but due to the above
the controller is forced down to a maximum of UDMA/33.

Resolve this by returning EAGAIN in ata_acpi_cbl_80wire() if no devices
have been detected and modify pata_via to handle this scenario.

First, an unknown cable is assumed which preserves the mode set by the
firmware, and then on subsequent calls when the devices have been
enabled, an 80 wire cable is correctly detected.

Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
---
 drivers/ata/libata-acpi.c | 11 ++++++++---
 drivers/ata/pata_via.c    | 13 ++++++++++---
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index b7f0bf795521..c508a19c2495 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -523,6 +523,7 @@ EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
 int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
 {
 	struct ata_device *dev;
+	int ret = -EAGAIN;
 
 	ata_for_each_dev(dev, &ap->link, ENABLED) {
 		unsigned int xfer_mask, udma_mask;
@@ -530,11 +531,15 @@ int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
 		xfer_mask = ata_acpi_gtm_xfermask(dev, gtm);
 		ata_unpack_xfermask(xfer_mask, NULL, NULL, &udma_mask);
 
-		if (udma_mask & ~ATA_UDMA_MASK_40C)
-			return 1;
+		ret = 0;
+
+		if (udma_mask & ~ATA_UDMA_MASK_40C) {
+			ret = 1;
+			break;
+		}
 	}
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
 
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index 696b99720dcb..4d03b4a1ea4d 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -182,6 +182,7 @@ static int via_cable_detect(struct ata_port *ap) {
 	const struct via_isa_bridge *config = ap->host->private_data;
 	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
 	u32 ata66;
+	const struct ata_acpi_gtm *gtm;
 
 	if (via_cable_override(pdev))
 		return ATA_CBL_PATA40_SHORT;
@@ -202,9 +203,15 @@ static int via_cable_detect(struct ata_port *ap) {
 	if (ata66 & (0x10100000 >> (16 * ap->port_no)))
 		return ATA_CBL_PATA80;
 	/* Check with ACPI so we can spot BIOS reported SATA bridges */
-	if (ata_acpi_init_gtm(ap) &&
-	    ata_acpi_cbl_80wire(ap, ata_acpi_init_gtm(ap)))
-		return ATA_CBL_PATA80;
+	gtm = ata_acpi_init_gtm(ap);
+	if (gtm) {
+		int cbl = ata_acpi_cbl_80wire(ap, gtm);
+
+		if (cbl < 0)
+			return ATA_CBL_PATA_UNK;
+		else if (cbl == 1)
+			return ATA_CBL_PATA80;
+	}
 	return ATA_CBL_PATA40;
 }
 
-- 
2.43.0

Re: [PATCH] ata: libata-acpi: Do not assume 40 wire cable if no devices are enabled

[Damien Le Moal]

On 5/14/25 20:29, Tasos Sahanidis wrote:
> On at least an ASRock 990FX Extreme 4 with a VIA VT6330, the devices
> have not yet been enabled by the first time ata_acpi_cbl_80wire() is
> called. This means that the ata_for_each_dev loop is never entered,
> and a 40 wire cable is assumed.
> 
> The VIA controller on this board does not report the cable in the PCI
> config space, thus having to fall back to ACPI even though no SATA
> bridge is present.
> 
> The _GTM values are correctly reported by the firmware through ACPI,
> which has already set up faster transfer modes, but due to the above
> the controller is forced down to a maximum of UDMA/33.
> 
> Resolve this by returning EAGAIN in ata_acpi_cbl_80wire() if no devices
> have been detected and modify pata_via to handle this scenario.
> 
> First, an unknown cable is assumed which preserves the mode set by the
> firmware, and then on subsequent calls when the devices have been
> enabled, an 80 wire cable is correctly detected.
> 
> Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
> ---
>  drivers/ata/libata-acpi.c | 11 ++++++++---
>  drivers/ata/pata_via.c    | 13 ++++++++++---
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
> index b7f0bf795521..c508a19c2495 100644
> --- a/drivers/ata/libata-acpi.c
> +++ b/drivers/ata/libata-acpi.c
> @@ -523,6 +523,7 @@ EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
>  int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
>  {
>  	struct ata_device *dev;
> +	int ret = -EAGAIN;

See below, but adding:

	gtm = ata_acpi_init_gtm(ap);
	if (!gtm)
		return ATA_CBL_PATA40;

would be an additional nice cleanup.

>  
>  	ata_for_each_dev(dev, &ap->link, ENABLED) {
>  		unsigned int xfer_mask, udma_mask;
> @@ -530,11 +531,15 @@ int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
>  		xfer_mask = ata_acpi_gtm_xfermask(dev, gtm);
>  		ata_unpack_xfermask(xfer_mask, NULL, NULL, &udma_mask);
>  
> -		if (udma_mask & ~ATA_UDMA_MASK_40C)
> -			return 1;
> +		ret = 0;
> +
> +		if (udma_mask & ~ATA_UDMA_MASK_40C) {
> +			ret = 1;
> +			break;
> +		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
>  
> diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
> index 696b99720dcb..4d03b4a1ea4d 100644
> --- a/drivers/ata/pata_via.c
> +++ b/drivers/ata/pata_via.c
> @@ -182,6 +182,7 @@ static int via_cable_detect(struct ata_port *ap) {
>  	const struct via_isa_bridge *config = ap->host->private_data;
>  	struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>  	u32 ata66;
> +	const struct ata_acpi_gtm *gtm;
>  
>  	if (via_cable_override(pdev))
>  		return ATA_CBL_PATA40_SHORT;
> @@ -202,9 +203,15 @@ static int via_cable_detect(struct ata_port *ap) {
>  	if (ata66 & (0x10100000 >> (16 * ap->port_no)))
>  		return ATA_CBL_PATA80;
>  	/* Check with ACPI so we can spot BIOS reported SATA bridges */
> -	if (ata_acpi_init_gtm(ap) &&
> -	    ata_acpi_cbl_80wire(ap, ata_acpi_init_gtm(ap)))
> -		return ATA_CBL_PATA80;
> +	gtm = ata_acpi_init_gtm(ap);
> +	if (gtm) {
> +		int cbl = ata_acpi_cbl_80wire(ap, gtm);

While at it, maybe change ata_acpi_cbl_80wire() returns ATA_CBL_PATA_UNK,
ATA_UDMA_MASK_40C or ATA_CBL_PATA80, and rename that function to something like:

ata_acpi_cbl_pata_type()

The call to ata_acpi_init_gtm() can also go into that function, thus removing
the gtm argument.

With that, this big ugly hunk would become:

	return ata_acpi_cbl_pata_type(ap);

> +
> +		if (cbl < 0)
> +			return ATA_CBL_PATA_UNK;
> +		else if (cbl == 1)
> +			return ATA_CBL_PATA80;
> +	}
>  	return ATA_CBL_PATA40;
>  }
>  


-- 
Damien Le Moal
Western Digital Research