Re: [PATCH] PCI/DPC: Disable ERR_COR explicitly for native dpc service

[Yicong Yang <yangyicong@hisilicon.com>]

On 2021/4/11 3:17, Lukas Wunner wrote:
> On Sat, Apr 10, 2021 at 10:21:03AM -0500, Bjorn Helgaas wrote:
>> Anybody want to chime in and review this?  Sometimes I feel like a
>> one-man band :)
> 
> Can't say anything about the object of the patch, but style-wise
> this looks cryptic:
> 
>>> --- a/drivers/pci/pcie/dpc.c
>>> +++ b/drivers/pci/pcie/dpc.c
>>> @@ -302,7 +302,7 @@ static int dpc_probe(struct pcie_device *dev)
>>>  	pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl);
>>>  
>>> -	ctl = (ctl & 0xfff4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>> +	ctl = (ctl & 0xffe4) | PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN;
>>>  	pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl);
> 
> Instead of writing "ctl & 0xfff4", I'd prefer defining macros for the
> register bits of interest, then use "ctl &= ~(u16)(bits to clear)"
> and in a separate line use "ctl |= (bits to set)".
> 

yes. that's clearer. I'll use macros if we're going to have this patch.

> Obviously, clearing bits that are unconditionally set afterwards is
> unnecessary (as is done here).
> 

ok. I found this when I read the Spec and thought it might be sanity
to ensure the bit is not set.

> 
>>>  	pci_info(pdev, "enabled with IRQ %d\n", dev->irq);
> 
> This looks superfluous since the IRQ can be found out in /proc/interrupts.

this can help us track dpc interrupts on certain ports through /proc/interrupts,
as all the dpc interrupts are named 'pcie-dpc' and its hard to distinguish.

Thanks,
Yicong

> 
> Thanks,
> 
> Lukas
> 
> .
>