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

[PATCH v2] 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 modifying ata_acpi_cbl_80wire() to directly return the
cable type. 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.

Since the function now directly returns the cable type, it has been
renamed to ata_acpi_cbl_pata_type().

Signed-off-by: Tasos Sahanidis <tasos@tasossah.com>
---
 v1 -> v2: Address review feedback
     * ata_acpi_cbl_80wire -> ata_acpi_cbl_pata_type
     * ata_acpi_init_gtm() called inside ata_acpi_cbl_pata_type

 drivers/ata/libata-acpi.c | 24 ++++++++++++++++--------
 drivers/ata/pata_via.c    |  6 ++----
 include/linux/libata.h    |  7 +++----
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c
index b7f0bf795521..f2140fc06ba0 100644
--- a/drivers/ata/libata-acpi.c
+++ b/drivers/ata/libata-acpi.c
@@ -514,15 +514,19 @@ unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 EXPORT_SYMBOL_GPL(ata_acpi_gtm_xfermask);
 
 /**
- * ata_acpi_cbl_80wire		-	Check for 80 wire cable
+ * ata_acpi_cbl_pata_type - Return PATA cable type
  * @ap: Port to check
- * @gtm: GTM data to use
  *
- * Return 1 if the @gtm indicates the BIOS selected an 80wire mode.
+ * Return ATA_CBL_PATA* according to the transfer mode selected by BIOS
  */
-int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm)
+int ata_acpi_cbl_pata_type(struct ata_port *ap)
 {
 	struct ata_device *dev;
+	int ret = ATA_CBL_PATA_UNK;
+	const struct ata_acpi_gtm *gtm = ata_acpi_init_gtm(ap);
+
+	if (!gtm)
+		return ATA_CBL_PATA40;
 
 	ata_for_each_dev(dev, &ap->link, ENABLED) {
 		unsigned int xfer_mask, udma_mask;
@@ -530,13 +534,17 @@ 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 = ATA_CBL_PATA40;
+
+		if (udma_mask & ~ATA_UDMA_MASK_40C) {
+			ret = ATA_CBL_PATA80;
+			break;
+		}
 	}
 
-	return 0;
+	return ret;
 }
-EXPORT_SYMBOL_GPL(ata_acpi_cbl_80wire);
+EXPORT_SYMBOL_GPL(ata_acpi_cbl_pata_type);
 
 static void ata_acpi_gtf_to_tf(struct ata_device *dev,
 			       const struct ata_acpi_gtf *gtf,
diff --git a/drivers/ata/pata_via.c b/drivers/ata/pata_via.c
index 696b99720dcb..c8acf6511071 100644
--- a/drivers/ata/pata_via.c
+++ b/drivers/ata/pata_via.c
@@ -201,11 +201,9 @@ static int via_cable_detect(struct ata_port *ap) {
 	   two drives */
 	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;
-	return ATA_CBL_PATA40;
+	return ata_acpi_cbl_pata_type(ap);
 }
 
 static int via_pre_reset(struct ata_link *link, unsigned long deadline)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index e5695998acb0..ec3b0c9c2a8c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1363,7 +1363,7 @@ int ata_acpi_stm(struct ata_port *ap, const struct ata_acpi_gtm *stm);
 int ata_acpi_gtm(struct ata_port *ap, struct ata_acpi_gtm *stm);
 unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 				   const struct ata_acpi_gtm *gtm);
-int ata_acpi_cbl_80wire(struct ata_port *ap, const struct ata_acpi_gtm *gtm);
+int ata_acpi_cbl_pata_type(struct ata_port *ap);
 #else
 static inline const struct ata_acpi_gtm *ata_acpi_init_gtm(struct ata_port *ap)
 {
@@ -1388,10 +1388,9 @@ static inline unsigned int ata_acpi_gtm_xfermask(struct ata_device *dev,
 	return 0;
 }
 
-static inline int ata_acpi_cbl_80wire(struct ata_port *ap,
-				      const struct ata_acpi_gtm *gtm)
+static inline int ata_acpi_cbl_pata_type(struct ata_port *ap)
 {
-	return 0;
+	return ATA_CBL_PATA40;
 }
 #endif
 
-- 
2.43.0

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

[Damien Le Moal]

On 2025/05/19 10:56, 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 modifying ata_acpi_cbl_80wire() to directly return the
> cable type. 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.
> 
> Since the function now directly returns the cable type, it has been
> renamed to ata_acpi_cbl_pata_type().

Nit: "it has been renamed" -> "it is renamed"

> @@ -530,13 +534,17 @@ 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 = ATA_CBL_PATA40;
> +
> +		if (udma_mask & ~ATA_UDMA_MASK_40C) {
> +			ret = ATA_CBL_PATA80;

Please change this to "return ATA_CBL_PATA80;" and change the last return at the
end of the function to "return ATA_CBL_PATA40;". That will be cleaner.

Other than these, this looks good.

-- 
Damien Le Moal
Western Digital Research

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

[Tasos Sahanidis]

On 2025-05-20 12:29, Damien Le Moal wrote:
> Nit: "it has been renamed" -> "it is renamed"

Will do.

>> @@ -530,13 +534,17 @@ 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 = ATA_CBL_PATA40;
>> +
>> +		if (udma_mask & ~ATA_UDMA_MASK_40C) {
>> +			ret = ATA_CBL_PATA80;
> 
> Please change this to "return ATA_CBL_PATA80;" and change the last return at the
> end of the function to "return ATA_CBL_PATA40;". That will be cleaner.
> 
> Other than these, this looks good.
> 

Apologies, but I am not sure I understand.

Wouldn't changing the last return to ATA_CBL_PATA40 undo the point of
this change? The function must return ATA_CBL_PATA_UNK if the loop is
never entered (no enabled devices).

If it does enter the loop but the mask hasn't matched at all after it
has gone through all the devices, it must return ATA_CBL_PATA40, which
is why there's the unconditional assignment ret = ATA_CBL_PATA40. If,
however, there is at least one "80 wire device", the whole cable is
considered 80 wire (thus the immediate break in the if).

I believe the most that can be done is:

if (udma_mask & ~ATA_UDMA_MASK_40C)
	return ATA_CBL_PATA80;

with the rest remaining the same. I opted for assignment + break there
instead of return as it seemed more readable to me, and it's consistent.

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

[Damien Le Moal]

On 5/21/25 10:56, Tasos Sahanidis wrote:
> On 2025-05-20 12:29, Damien Le Moal wrote:
>> Nit: "it has been renamed" -> "it is renamed"
> 
> Will do.
> 
>>> @@ -530,13 +534,17 @@ 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 = ATA_CBL_PATA40;
>>> +
>>> +		if (udma_mask & ~ATA_UDMA_MASK_40C) {
>>> +			ret = ATA_CBL_PATA80;
>>
>> Please change this to "return ATA_CBL_PATA80;" and change the last return at the
>> end of the function to "return ATA_CBL_PATA40;". That will be cleaner.
>>
>> Other than these, this looks good.
>>
> 
> Apologies, but I am not sure I understand.
> 
> Wouldn't changing the last return to ATA_CBL_PATA40 undo the point of
> this change? The function must return ATA_CBL_PATA_UNK if the loop is
> never entered (no enabled devices).

Yes, my bad. That last return should return unknown.
Let me apply the patches and see how it looks. This is all cosmetic anyway.

> 
> If it does enter the loop but the mask hasn't matched at all after it
> has gone through all the devices, it must return ATA_CBL_PATA40, which
> is why there's the unconditional assignment ret = ATA_CBL_PATA40. If,
> however, there is at least one "80 wire device", the whole cable is
> considered 80 wire (thus the immediate break in the if).
> 
> I believe the most that can be done is:
> 
> if (udma_mask & ~ATA_UDMA_MASK_40C)
> 	return ATA_CBL_PATA80;
> 
> with the rest remaining the same. I opted for assignment + break there
> instead of return as it seemed more readable to me, and it's consistent.


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Mon, 19 May 2025 11:56:55 +0300, 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.
> 
> [...]

Applied to libata/linux.git (for-6.16-fixes), thanks!

[1/1] ata: libata-acpi: Do not assume 40 wire cable if no devices are enabled
      https://git.kernel.org/libata/linux/c/33877220

Kind regards,
Niklas