| Message ID | 1495103748-7876-2-git-send-email-gabriele.paoloni@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
> + * > + * pci_irq_vector() below is able to handle entry differently > + * depending on MSI vs MSI-x case > + * > */ One more instance of this comment left :) > pcie_capability_read_word(dev, PCI_EXP_FLAGS, ®16); > entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9; > @@ -100,7 +107,10 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) > * MSI/MSI-X vectors assigned to the port is going to be used > * for AER, where "For MSI-X, the value in this register > * indicates which MSI-X Table entry is used to generate the > - * interrupt message." > + * interrupt message." and "For MSI, the value > + * in this field indicates the offset between the base Message > + * Data and the interrupt message that is generated." > + * > */ And while this is getting a little too deep into nitpicking: we usually don't add empty lines to the end of comments.
Hi Christoph > -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Christoph Hellwig > Sent: 21 May 2017 09:33 > To: Gabriele Paoloni > Cc: bhelgaas@google.com; helgaas@kernel.org; Linuxarm; linux- > pci@vger.kernel.org; lukas@wunner.de; linux-kernel@vger.kernel.org; > mika.westerberg@linux.intel.com; hch@infradead.org; liudongdong (C) > Subject: Re: [PATCH v3 1/2] PCI/portdrv: add support for different MSI > interrupts for PCIe port services > > > + * > > + * pci_irq_vector() below is able to handle entry > diff erently > > + * depending on MSI vs MSI-x case > > + * > > */ > > One more instance of this comment left :) I thought that in your previous review you meant to say that we should avoid 2 instances of the same comment (in this patchset I only have this instance as I have removed the same comment from patch 2/2...) > > > pcie_capability_read_word(dev, PCI_EXP_FLAGS, ®16); > > entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9; > > @@ -100,7 +107,10 @@ static int pcie_port_enable_msix(struct pci_dev > *dev, int *irqs, int mask) > > * MSI/MSI-X vectors assigned to the port is going to be > used > > * for AER, where "For MSI-X, the value in this register > > * indicates which MSI-X Table entry is used to generate > the > > - * interrupt message." > > + * interrupt message." and "For MSI, the value > > + * in this field indicates the offset between the base > Message > > + * Data and the interrupt message that is generated." > > + * > > */ > > And while this is getting a little too deep into nitpicking: we > usually > don't add empty lines to the end of comments. Ok I can remove the extra line and send out patch v4... Many thanks Gab
On Sun, May 21, 2017 at 03:00:46PM +0000, Gabriele Paoloni wrote: > > I thought that in your previous review you meant to say that we should > avoid 2 instances of the same comment (in this patchset I only have this > instance as I have removed the same comment from patch 2/2...) No, there is no pointin the comment. The whole point of pci_irq_vector is to abstract away from how to get at the interrupt in detail. There is no need to comment on this at the callsite. If you think the existing documentation for the function is not good enough send a patch to improve it there instead of commenting the callsites.
Hi Christoph > -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Christoph Hellwig > Sent: 22 May 2017 18:44 > To: Gabriele Paoloni > Cc: Christoph Hellwig; bhelgaas@google.com; helgaas@kernel.org; > Linuxarm; linux-pci@vger.kernel.org; lukas@wunner.de; linux- > kernel@vger.kernel.org; mika.westerberg@linux.intel.com; liudongdong > (C) > Subject: Re: [PATCH v3 1/2] PCI/portdrv: add support for different MSI > interrupts for PCIe port services > > On Sun, May 21, 2017 at 03:00:46PM +0000, Gabriele Paoloni wrote: > > > > I thought that in your previous review you meant to say that we > should > > avoid 2 instances of the same comment (in this patchset I only have > this > > instance as I have removed the same comment from patch 2/2...) > > No, there is no pointin the comment. The whole point of pci_irq_vector > is to abstract away from how to get at the interrupt in detail. There > is no need to comment on this at the callsite. If you think the > existing documentation for the function is not good enough send a patch > to improve it there instead of commenting the callsites. Ok got it. I have just sent out v5 where this is fixed. Thanks Gab
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h index 587aef3..1993e2c 100644 --- a/drivers/pci/pcie/portdrv.h +++ b/drivers/pci/pcie/portdrv.h @@ -13,10 +13,12 @@ #define PCIE_PORT_DEVICE_MAXSERVICES 5 /* - * According to the PCI Express Base Specification 2.0, the indices of - * the MSI-X table entries used by port services must not exceed 31 + * According to the PCI Express Base Specification REV. 3.1 and according + * to the PCI Local Bus Specification REV. 3.0 respectively, the indices of + * the MSI-X table entries or the max number of MSI vectors used by port + * services must not exceed 31 */ -#define PCIE_PORT_MAX_MSIX_ENTRIES 32 +#define PCIE_PORT_MAX_MSI_ENTRIES 32 #define get_descriptor_id(type, service) (((type - 4) << 8) | service) diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index cea504f..4d16cf2 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -44,14 +44,15 @@ static void release_pcie_device(struct device *dev) } /** - * pcie_port_enable_msix - try to set up MSI-X as interrupt mode for given port + * pcie_port_enable_irq_vec - try to set up MSI-X or MSI as interrupt mode + * for given port * @dev: PCI Express port to handle * @irqs: Array of interrupt vectors to populate * @mask: Bitmask of port capabilities returned by get_port_device_capability() * * Return value: 0 on success, error code on failure */ -static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) +static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) { int nr_entries, entry, nvec = 0; @@ -61,8 +62,8 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) * equal to the number of entries this port actually uses, we'll happily * go through without any tricks. */ - nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSIX_ENTRIES, - PCI_IRQ_MSIX); + nr_entries = pci_alloc_irq_vectors(dev, 1, PCIE_PORT_MAX_MSI_ENTRIES, + PCI_IRQ_MSIX | PCI_IRQ_MSI); if (nr_entries < 0) return nr_entries; @@ -77,7 +78,13 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) * Number field in the PCI Express Capabilities register", where * according to Section 7.8.2 of the specification "For MSI-X, * the value in this field indicates which MSI-X Table entry is - * used to generate the interrupt message." + * used to generate the interrupt message." and "For MSI, the + * value in this field indicates the offset between the base + * Message Data and the interrupt message that is generated." + * + * pci_irq_vector() below is able to handle entry differently + * depending on MSI vs MSI-x case + * */ pcie_capability_read_word(dev, PCI_EXP_FLAGS, ®16); entry = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9; @@ -100,7 +107,10 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) * MSI/MSI-X vectors assigned to the port is going to be used * for AER, where "For MSI-X, the value in this register * indicates which MSI-X Table entry is used to generate the - * interrupt message." + * interrupt message." and "For MSI, the value + * in this field indicates the offset between the base Message + * Data and the interrupt message that is generated." + * */ pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ERR); pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS, ®32); @@ -124,7 +134,7 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) /* Now allocate the MSI-X vectors for real */ nr_entries = pci_alloc_irq_vectors(dev, nvec, nvec, - PCI_IRQ_MSIX); + PCI_IRQ_MSIX | PCI_IRQ_MSI); if (nr_entries < 0) return nr_entries; } @@ -146,26 +156,23 @@ static int pcie_port_enable_msix(struct pci_dev *dev, int *irqs, int mask) */ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask) { - unsigned flags = PCI_IRQ_LEGACY | PCI_IRQ_MSI; int ret, i; for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++) irqs[i] = -1; /* - * If MSI cannot be used for PCIe PME or hotplug, we have to use - * INTx or other interrupts, e.g. system shared interrupt. + * Make sure MSI can be used for PCIe PME or hotplug. otherwise we have + * to use INTx or other interrupts, e.g. system shared interrupt. */ - if (((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi()) || - ((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())) { - flags &= ~PCI_IRQ_MSI; - } else { - /* Try to use MSI-X if supported */ - if (!pcie_port_enable_msix(dev, irqs, mask)) + if (!((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi()) && + !((mask & PCIE_PORT_SERVICE_HP) && pciehp_no_msi())) + /* Try to use MSI-X or MSI if supported */ + if (!pcie_port_enable_irq_vec(dev, irqs, mask)) return 0; - } - ret = pci_alloc_irq_vectors(dev, 1, 1, flags); + /* fall back to legacy IRQ */ + ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_LEGACY); if (ret < 0) return -ENODEV;