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

[Damien Le Moal <dlemoal@kernel.org>]

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