On Fri, 13 Oct 2023, Bjorn Helgaas wrote: > On Fri, Oct 13, 2023 at 02:20:04PM +0300, Ilpo Järvinen wrote: > > Use defines instead of literals and replace custom masking and shifts > > with FIELD_GET() where it makes sense. > > ... > > > pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CAP, &cap); > > - 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; > > + pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, &ctl); > > + ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; > > + ctl &= ~PCI_EXP_DPC_CTL_EN_NONFATAL; > > This has been a little obtuse from the beginning. > > The original clears bits 0, 1, 3, then sets bits 0 and 3. > The new code sets bits 0, 3, then clears bit 1. > > These are equivalent, but it's definitely some work to verify it. > > I think the point is to enable DPC on ERR_FATAL (but not ERR_NONFATAL) > and to enable DPC interrupts. What about something like this? > #define PCI_EXP_DPC_CTL_EN_MASK 0x0003 > > ctl &= ~PCI_EXP_DPC_CTL_EN_MASK; > ctl |= PCI_EXP_DPC_CTL_EN_FATAL | PCI_EXP_DPC_CTL_INT_EN; Thanks for the suggestion, it looks cleaner, yes. I realized the bit changes weren't as obvious as they could be but I failed to see I could add such a combined mask to make the intent dead obvious. With a small change replacing the 0x0003 literal with the combined defines of the actual bits it seems very clear what's going on. I'll go for that. I'll place my non-FIELD_GET() changes on DPC top of your FIELD_GET/PREP()-only change so each patch can be smaller and more focused. -- i. > > pci_write_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_CTL, ctl); > > - pci_info(pdev, "enabled with IRQ %d\n", dev->irq); > > > > + pci_info(pdev, "enabled with IRQ %d\n", dev->irq); > > pci_info(pdev, "error containment capabilities: Int Msg #%d, RPExt%c PoisonedTLP%c SwTrigger%c RP PIO Log %d, DL_ActiveErr%c\n", > > cap & PCI_EXP_DPC_IRQ, FLAG(cap, PCI_EXP_DPC_CAP_RP_EXT), > > FLAG(cap, PCI_EXP_DPC_CAP_POISONED_TLP), > > -- > > 2.30.2 > > >
