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
.