Re:Re: [PATCH v2] ata: ahci: fail probe if BAR too small for claimed ports

["李佑鸿 " <dayou5941@163.com>]

At 2026-04-27 07:46:12, "Damien Le Moal" <dlemoal@kernel.org> wrote:
>On 4/25/26 3:55 PM, dayou5941@163.com wrote:
>> From: liyouhong <liyouhong@kylinos.cn>
>> 
>> When an AHCI controller is disabled in BIOS, its HOST_CAP register may
>> contain invalid values (e.g., 0xFFFFFFFF) indicating an impossibly large
>> number of ports. If CAP.NP claims more ports than can physically fit
>> within the mapped BAR region, accessing port registers beyond the BAR
>> boundary causes a kernel panic.
>> 
>> Add validation in ahci_init_one() to check that the BAR size is
>> sufficient for the number of ports claimed in CAP.NP. The check
>> calculates the required MMIO size as:
>> 
>>   required_size = 0x100 (global registers) + max_ports * 0x80
>> 
>> If required_size exceeds the actual BAR size, the probe fails with
>> -ENODEV, preventing the panic and providing a clear error message.
>> 
>> This solution follows the suggestion by Damien Le Moal and Niklas Cassel
>> to detect and reject obviously broken controller configurations early.
>> 
>> v2:
>> - Complete rewrite based on community feedback
>> - Move check from libahci.c to ahci.c
>> - Fail probe early instead of attempting to work around invalid state
>> - Implement BAR size validation as suggested
>> 
>> Reported-by: liyouhong <liyouhong@kylinos.cn>
>> Suggested-by: Damien Le Moal <dlemoal@kernel.org>
>> Suggested-by: Niklas Cassel <niklas.soderlund@corigine.com>
>> Signed-off-by: liyouhong <liyouhong@kylinos.cn>
>
>Your patch is not formatted correctly: the changelog should not be part of the
>commit message but should come between the "---" separator after the tags and
>the first "diff" line of the patch proper. The "---" separator is missing here
>too. Did you generate this patch with "git format-patch" ?
>
>> 
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 1d73a53370cf..09026ea12cde 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -1888,6 +1888,23 @@ static ssize_t remapped_nvme_show(struct device *dev,
>>  
>>  static DEVICE_ATTR_RO(remapped_nvme);
>>  
>> +static int ahci_validate_bar_size(struct pci_dev *pdev, void __iomem *mmio)
>> +{
>> +	u32 cap = readl(mmio + HOST_CAP);
>> +	unsigned int max_ports = ahci_nr_ports(cap);
>> +	u32 last_port_end = 0x100 + (max_ports * 0x80);
>> +	resource_size_t bar_size = pci_resource_len(pdev, AHCI_PCI_BAR_STANDARD);
>> +
>> +	if (last_port_end > bar_size) {
>
>It may be good to check also that max_ports is not zero here.
>
>> +		dev_err(&pdev->dev,
>> +			"AHCI: BAR5 too small for %u ports (last port ends at %u, BAR %llu)\n",
>> +			max_ports, last_port_end, (unsigned long long)bar_size);
>

>Please make this a warning (dev_warn()).


Thank you for the review. Here's v3 with the requested changes:


Fixed patch format (added "---" separator, moved changelog).
Changed dev_err to dev_warn.


Regarding the max_ports check: The ahci_nr_ports() helper function is defined as:
static inline unsigned int ahci_nr_ports(u32 cap)
{
    return (cap & 0x1f) + 1;  // Note the +1
}


This function always returns a value ≥ 1, so checking for 0 is not needed.
The hardware could theoretically report CAP.NP=0, but that indicates 1 port
, not 0 ports.


Please let me know if you'd like additional validation.



Best regards,
liyouhong