Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3e025745-06af-c5c6-aa70-6ff1f9ad0962@bytedance.com>
Date:   Mon, 26 Sep 2022 22:16:23 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.3.0
Subject: Re: [PATCH 3/3] PCI/AER: Use pci_aer_raw_clear_status() to clear root
 port's AER error status
Content-Language: en-US
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     fancer.lancer@gmail.com, jdmason@kudzu.us, dave.jiang@intel.com,
        allenbh@gmail.com, bhelgaas@google.com, ruscur@russell.cc,
        oohall@gmail.com, james.smart@broadcom.com,
        dick.kennedy@broadcom.com, jejb@linux.ibm.com,
        martin.petersen@oracle.com, linux-scsi@vger.kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        ntb@lists.linux.dev, linuxppc-dev@lists.ozlabs.org
References: <20220922215030.GA1341314@bhelgaas>
From:   Zhuo Chen <chenzhuo.1@bytedance.com>
In-Reply-To: <20220922215030.GA1341314@bhelgaas>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/23/22 5:50 AM, Bjorn Helgaas wrote:
> On Fri, Sep 02, 2022 at 02:16:34AM +0800, Zhuo Chen wrote:
>> Statements clearing AER error status in aer_enable_rootport() has the
>> same function as pci_aer_raw_clear_status(). So we replace them, which
>> has no functional changes.
>>
>> Signed-off-by: Zhuo Chen <chenzhuo.1@bytedance.com>
>> ---
>>   drivers/pci/pcie/aer.c | 7 +------
>>   1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
>> index d2996afa80f6..eb0193f279f2 100644
>> --- a/drivers/pci/pcie/aer.c
>> +++ b/drivers/pci/pcie/aer.c
>> @@ -1287,12 +1287,7 @@ static void aer_enable_rootport(struct aer_rpc *rpc)
>>   				   SYSTEM_ERROR_INTR_ON_MESG_MASK);
>>   
>>   	/* Clear error status */
>> -	pci_read_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, &reg32);
>> -	pci_write_config_dword(pdev, aer + PCI_ERR_ROOT_STATUS, reg32);
>> -	pci_read_config_dword(pdev, aer + PCI_ERR_COR_STATUS, &reg32);
>> -	pci_write_config_dword(pdev, aer + PCI_ERR_COR_STATUS, reg32);
>> -	pci_read_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, &reg32);
>> -	pci_write_config_dword(pdev, aer + PCI_ERR_UNCOR_STATUS, reg32);
>> +	pci_aer_raw_clear_status(pdev);
> 
> It's true that this is functionally equivalent.
> 
> But 20e15e673b05 ("PCI/AER: Add pci_aer_raw_clear_status() to
> unconditionally clear Error Status") says pci_aer_raw_clear_status()
> is only for use in the EDR path (this should have been included in the
> function comment), so I think we should preserve that property and use
> pci_aer_clear_status() here.
> 
> pci_aer_raw_clear_status() is the same as pci_aer_clear_status()
> except it doesn't check pcie_aer_is_native().  And I'm pretty sure we
> can't get to aer_enable_rootport() *unless* pcie_aer_is_native(),
> because get_port_device_capability() checks the same thing, so they
> should be equivalent here.
> 
> Bjorn
Thanks Bjorn, this very detailed correction is helpful. By the way, 
'only for use in the EDR path' obviously written in the function 
comments may be better. So far only commit log has included these.

I will change to use pci_aer_clear_status() in next patch.

-- 
Thanks,
Zhuo Chen