On Wed, Oct 11, 2017 at 01:01:26PM +0000, David Laight wrote: > From: Dongdong Liu > > Sent: 11 October 2017 11:53 > > In the AER case, the mask isn't strictly necessary because there are no > > higher-order bits above the Interrupt Message Number, but using a #define > > will make it possible to grep for it. > > > > Suggested-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx> > > Signed-off-by: Dongdong Liu <liudongdong3@xxxxxxxxxx> > > --- > > drivers/pci/pcie/portdrv_core.c | 4 ++-- > > include/uapi/linux/pci_regs.h | 2 ++ > > 2 files changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > > index 9692379..2b48297 100644 > > --- a/drivers/pci/pcie/portdrv_core.c > > +++ b/drivers/pci/pcie/portdrv_core.c > > @@ -116,7 +116,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) > > */ > > pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); > > pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); > > - entry = reg32 >> 27; > > + entry = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27; > > You've still got the hard coded 27 - which is almost more important than the mask. > Perhaps define things so this is: > entry = PCI_ERR_ROOT_AER_IRQ(reg32); > > Think I'd shift then mask with 0x1f. Personally, I prefer to have the mask unshifted so it's easy to see where the fields are in the register and to compare them with the spec, e.g., #define PCI_EXP_DEVCAP_PAYLOAD 0x00000007 /* Max_Payload_Size */ #define PCI_EXP_DEVCAP_PHANTOM 0x00000018 /* Phantom functions */ #define PCI_EXP_DEVCAP_EXT_TAG 0x00000020 /* Extended tags */ Should there be a #define for the shift? Maybe. It doesn't bother me unless it's used in many places. Here I think there'd only be one use, so having it hard-coded along with the symbolic mask is enough for me. Sometimes too many #defines clutter up the include file. Just my personal preferences. Bjorn