| Message ID | 1524817367-239076-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
s/PCI: Add new helper for allocating irq domain for INTx/ PCI: Add pci_alloc_intx_irqd() for allocating IRQ domain On Fri, Apr 27, 2018 at 04:22:47PM +0800, Shawn Lin wrote: > It seems host drivers which allocate irq domain for INTx s/irq/IRQ/ > and the related code block looks highly similar to each other. > Add a new helper, pci_alloc_intx_irqd(), to avoid code duplication > as much as possible. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > include/linux/pci.h | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 74 insertions(+) > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 73178a2..68a1994 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -31,6 +31,8 @@ > #include <linux/device.h> > #include <linux/interrupt.h> > #include <linux/io.h> > +#include <linux/irq.h> > +#include <linux/of_irq.h> > #include <linux/resource_ext.h> > #include <uapi/linux/pci.h> > > @@ -1449,6 +1451,74 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, > return 0; > } > > +static int pcie_intx_map(struct irq_domain *domain, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); > + irq_set_chip_data(irq, domain->host_data); > + > + return 0; > +} > + > +static struct irq_domain_ops pci_intx_domain_ops = { > + .map = pcie_intx_map, > +}; > + > +/** > + * pci_alloc_intx_irqd() - PCI helper for allocating INTx irq domain s/PCI helper for allocating INTx irq domain/ Allocate INTx IRQ domain > + * @dev: device associated with the PCI controller. > + * @host: pointer to host specific data struct > + * @general_xlate: flag for whether use pci_irqd_intx_xlate() helper > + * @intx_domain_ops: pointer to driver specific struct irq_domain_ops > + * @local_intc: pointer to driver specific interrupt controller node > + * > + * A simple helper for drivers to allocate irq domain for INTx. If > + * intx_domain_ops is NULL, use pci_intx_domain_ops by defalut. And if > + * local_intc is present, then use it firstly, otherwise, fallback to get > + * interrupt controller node from @dev. s/irq/IRQ/ s/defalut/default/ > + * Returns valid pointer of struct irq_domain on success, or PTR_ERR(-EINVAL) > + * if failure occurred. > + */ > +static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev, > + void *host, bool general_xlate, > + const struct irq_domain_ops *intx_domain_ops, > + struct device_node *local_intc) Why is this in the header file? This is not a performance path and I don't want to deal with this __maybe_unused stuff. > +{ > + struct device_node *intc = local_intc; > + struct irq_domain *domain; > + const struct irq_domain_ops *irqd_ops; > + bool need_put = false; > + > + if (!intc) { > + intc = of_get_next_child(dev->of_node, NULL); > + if (!intc) { > + dev_err(dev, "missing child interrupt-controller node\n"); > + return ERR_PTR(-EINVAL); > + } > + need_put = true; > + } > + > + if (intx_domain_ops) { > + irqd_ops = intx_domain_ops; > + } else { > + if (general_xlate) > + pci_intx_domain_ops.xlate = &pci_irqd_intx_xlate; > + irqd_ops = &pci_intx_domain_ops; > + } > + > + domain = irq_domain_add_linear(intc, PCI_NUM_INTX, > + irqd_ops, host); > + if (!domain) { > + dev_err(dev, "failed to get a INTx IRQ domain\n"); > + if (need_put) > + of_node_put(intc); > + return ERR_PTR(-EINVAL); > + } > + > + return domain; > +} > + > #ifdef CONFIG_PCIEPORTBUS > extern bool pcie_ports_disabled; > #else > @@ -1683,6 +1753,10 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, > unsigned long *out_hwirq, > unsigned int *out_type) > { return -EINVAL; } > + > +static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev, > + void *host, bool general_xlate, const struct irq_domain_ops *ops) > +{ return ERR_PTR(-EINVAL); } > #endif /* CONFIG_PCI */ > > /* Include architecture-dependent settings and functions */ > -- > 1.9.1 > >
Hi Bjorn, On 2018/4/28 1:18, Bjorn Helgaas wrote: > s/PCI: Add new helper for allocating irq domain for INTx/ > PCI: Add pci_alloc_intx_irqd() for allocating IRQ domain > > On Fri, Apr 27, 2018 at 04:22:47PM +0800, Shawn Lin wrote: >> It seems host drivers which allocate irq domain for INTx > > s/irq/IRQ/ > >> and the related code block looks highly similar to each other. >> Add a new helper, pci_alloc_intx_irqd(), to avoid code duplication >> as much as possible. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> include/linux/pci.h | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 74 insertions(+) >> >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 73178a2..68a1994 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -31,6 +31,8 @@ >> #include <linux/device.h> >> #include <linux/interrupt.h> >> #include <linux/io.h> >> +#include <linux/irq.h> >> +#include <linux/of_irq.h> >> #include <linux/resource_ext.h> >> #include <uapi/linux/pci.h> >> >> @@ -1449,6 +1451,74 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, >> return 0; >> } >> >> +static int pcie_intx_map(struct irq_domain *domain, unsigned int irq, >> + irq_hw_number_t hwirq) >> +{ >> + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); >> + irq_set_chip_data(irq, domain->host_data); >> + >> + return 0; >> +} >> + >> +static struct irq_domain_ops pci_intx_domain_ops = { >> + .map = pcie_intx_map, >> +}; >> + >> +/** >> + * pci_alloc_intx_irqd() - PCI helper for allocating INTx irq domain > > s/PCI helper for allocating INTx irq domain/ > Allocate INTx IRQ domain > >> + * @dev: device associated with the PCI controller. >> + * @host: pointer to host specific data struct >> + * @general_xlate: flag for whether use pci_irqd_intx_xlate() helper >> + * @intx_domain_ops: pointer to driver specific struct irq_domain_ops >> + * @local_intc: pointer to driver specific interrupt controller node >> + * >> + * A simple helper for drivers to allocate irq domain for INTx. If >> + * intx_domain_ops is NULL, use pci_intx_domain_ops by defalut. And if >> + * local_intc is present, then use it firstly, otherwise, fallback to get >> + * interrupt controller node from @dev. > > s/irq/IRQ/ > s/defalut/default/ > >> + * Returns valid pointer of struct irq_domain on success, or PTR_ERR(-EINVAL) >> + * if failure occurred. >> + */ >> +static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev, >> + void *host, bool general_xlate, >> + const struct irq_domain_ops *intx_domain_ops, >> + struct device_node *local_intc) > > Why is this in the header file? This is not a performance path and I > don't want to deal with this __maybe_unused stuff. Ah, I saw pci_irqd_intx_xlate() was here, and instinctively keep the code close to it. Will move it to C file in v2. > >> +{ >> + struct device_node *intc = local_intc; >> + struct irq_domain *domain; >> + const struct irq_domain_ops *irqd_ops; >> + bool need_put = false; >> + >> + if (!intc) { >> + intc = of_get_next_child(dev->of_node, NULL); >> + if (!intc) { >> + dev_err(dev, "missing child interrupt-controller node\n"); >> + return ERR_PTR(-EINVAL); >> + } >> + need_put = true; >> + } >> + >> + if (intx_domain_ops) { >> + irqd_ops = intx_domain_ops; >> + } else { >> + if (general_xlate) >> + pci_intx_domain_ops.xlate = &pci_irqd_intx_xlate; >> + irqd_ops = &pci_intx_domain_ops; >> + } >> + >> + domain = irq_domain_add_linear(intc, PCI_NUM_INTX, >> + irqd_ops, host); >> + if (!domain) { >> + dev_err(dev, "failed to get a INTx IRQ domain\n"); >> + if (need_put) >> + of_node_put(intc); >> + return ERR_PTR(-EINVAL); >> + } >> + >> + return domain; >> +} >> + >> #ifdef CONFIG_PCIEPORTBUS >> extern bool pcie_ports_disabled; >> #else >> @@ -1683,6 +1753,10 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, >> unsigned long *out_hwirq, >> unsigned int *out_type) >> { return -EINVAL; } >> + >> +static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev, >> + void *host, bool general_xlate, const struct irq_domain_ops *ops) >> +{ return ERR_PTR(-EINVAL); } >> #endif /* CONFIG_PCI */ >> >> /* Include architecture-dependent settings and functions */ >> -- >> 1.9.1 >> >> > > >
Hi Shawn,
I love your patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v4.17-rc2 next-20180426]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Shawn-Lin/Add-new-helper-to-allocate-irq-domain-for-hosts/20180429-053656
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: i386-alldefconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All error/warnings (new ones prefixed by >>):
In file included from arch/x86//kernel/cpu/mtrr/main.c:46:0:
include/linux/pci.h: In function 'pci_alloc_intx_irqd':
>> include/linux/pci.h:1510:11: error: implicit declaration of function 'irq_domain_add_linear'; did you mean 'irq_domain_get_of_node'? [-Werror=implicit-function-declaration]
domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
^~~~~~~~~~~~~~~~~~~~~
irq_domain_get_of_node
>> include/linux/pci.h:1510:9: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
^
cc1: some warnings being treated as errors
vim +1510 include/linux/pci.h
1466
1467 /**
1468 * pci_alloc_intx_irqd() - PCI helper for allocating INTx irq domain
1469 * @dev: device associated with the PCI controller.
1470 * @host: pointer to host specific data struct
1471 * @general_xlate: flag for whether use pci_irqd_intx_xlate() helper
1472 * @intx_domain_ops: pointer to driver specific struct irq_domain_ops
1473 * @local_intc: pointer to driver specific interrupt controller node
1474 *
1475 * A simple helper for drivers to allocate irq domain for INTx. If
1476 * intx_domain_ops is NULL, use pci_intx_domain_ops by defalut. And if
1477 * local_intc is present, then use it firstly, otherwise, fallback to get
1478 * interrupt controller node from @dev.
1479 *
1480 * Returns valid pointer of struct irq_domain on success, or PTR_ERR(-EINVAL)
1481 * if failure occurred.
1482 */
1483 static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev,
1484 void *host, bool general_xlate,
1485 const struct irq_domain_ops *intx_domain_ops,
1486 struct device_node *local_intc)
1487 {
1488 struct device_node *intc = local_intc;
1489 struct irq_domain *domain;
1490 const struct irq_domain_ops *irqd_ops;
1491 bool need_put = false;
1492
1493 if (!intc) {
1494 intc = of_get_next_child(dev->of_node, NULL);
1495 if (!intc) {
1496 dev_err(dev, "missing child interrupt-controller node\n");
1497 return ERR_PTR(-EINVAL);
1498 }
1499 need_put = true;
1500 }
1501
1502 if (intx_domain_ops) {
1503 irqd_ops = intx_domain_ops;
1504 } else {
1505 if (general_xlate)
1506 pci_intx_domain_ops.xlate = &pci_irqd_intx_xlate;
1507 irqd_ops = &pci_intx_domain_ops;
1508 }
1509
> 1510 domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
1511 irqd_ops, host);
1512 if (!domain) {
1513 dev_err(dev, "failed to get a INTx IRQ domain\n");
1514 if (need_put)
1515 of_node_put(intc);
1516 return ERR_PTR(-EINVAL);
1517 }
1518
1519 return domain;
1520 }
1521
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Shawn,
I love your patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v4.17-rc2 next-20180426]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Shawn-Lin/Add-new-helper-to-allocate-irq-domain-for-hosts/20180429-053656
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: sparc-defconfig (attached as .config)
compiler: sparc-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc
All errors (new ones prefixed by >>):
In file included from arch/sparc/kernel/ioport.c:37:0:
include/linux/pci.h: In function 'pci_alloc_intx_irqd':
include/linux/pci.h:1510:11: error: implicit declaration of function 'irq_domain_add_linear'; did you mean 'irq_domain_get_of_node'? [-Werror=implicit-function-declaration]
domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
^~~~~~~~~~~~~~~~~~~~~
irq_domain_get_of_node
>> include/linux/pci.h:1510:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
^
cc1: all warnings being treated as errors
vim +1510 include/linux/pci.h
1466
1467 /**
1468 * pci_alloc_intx_irqd() - PCI helper for allocating INTx irq domain
1469 * @dev: device associated with the PCI controller.
1470 * @host: pointer to host specific data struct
1471 * @general_xlate: flag for whether use pci_irqd_intx_xlate() helper
1472 * @intx_domain_ops: pointer to driver specific struct irq_domain_ops
1473 * @local_intc: pointer to driver specific interrupt controller node
1474 *
1475 * A simple helper for drivers to allocate irq domain for INTx. If
1476 * intx_domain_ops is NULL, use pci_intx_domain_ops by defalut. And if
1477 * local_intc is present, then use it firstly, otherwise, fallback to get
1478 * interrupt controller node from @dev.
1479 *
1480 * Returns valid pointer of struct irq_domain on success, or PTR_ERR(-EINVAL)
1481 * if failure occurred.
1482 */
1483 static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev,
1484 void *host, bool general_xlate,
1485 const struct irq_domain_ops *intx_domain_ops,
1486 struct device_node *local_intc)
1487 {
1488 struct device_node *intc = local_intc;
1489 struct irq_domain *domain;
1490 const struct irq_domain_ops *irqd_ops;
1491 bool need_put = false;
1492
1493 if (!intc) {
1494 intc = of_get_next_child(dev->of_node, NULL);
1495 if (!intc) {
1496 dev_err(dev, "missing child interrupt-controller node\n");
1497 return ERR_PTR(-EINVAL);
1498 }
1499 need_put = true;
1500 }
1501
1502 if (intx_domain_ops) {
1503 irqd_ops = intx_domain_ops;
1504 } else {
1505 if (general_xlate)
1506 pci_intx_domain_ops.xlate = &pci_irqd_intx_xlate;
1507 irqd_ops = &pci_intx_domain_ops;
1508 }
1509
> 1510 domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
1511 irqd_ops, host);
1512 if (!domain) {
1513 dev_err(dev, "failed to get a INTx IRQ domain\n");
1514 if (need_put)
1515 of_node_put(intc);
1516 return ERR_PTR(-EINVAL);
1517 }
1518
1519 return domain;
1520 }
1521
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/include/linux/pci.h b/include/linux/pci.h index 73178a2..68a1994 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -31,6 +31,8 @@ #include <linux/device.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/irq.h> +#include <linux/of_irq.h> #include <linux/resource_ext.h> #include <uapi/linux/pci.h> @@ -1449,6 +1451,74 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, return 0; } +static int pcie_intx_map(struct irq_domain *domain, unsigned int irq, + irq_hw_number_t hwirq) +{ + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq); + irq_set_chip_data(irq, domain->host_data); + + return 0; +} + +static struct irq_domain_ops pci_intx_domain_ops = { + .map = pcie_intx_map, +}; + +/** + * pci_alloc_intx_irqd() - PCI helper for allocating INTx irq domain + * @dev: device associated with the PCI controller. + * @host: pointer to host specific data struct + * @general_xlate: flag for whether use pci_irqd_intx_xlate() helper + * @intx_domain_ops: pointer to driver specific struct irq_domain_ops + * @local_intc: pointer to driver specific interrupt controller node + * + * A simple helper for drivers to allocate irq domain for INTx. If + * intx_domain_ops is NULL, use pci_intx_domain_ops by defalut. And if + * local_intc is present, then use it firstly, otherwise, fallback to get + * interrupt controller node from @dev. + * + * Returns valid pointer of struct irq_domain on success, or PTR_ERR(-EINVAL) + * if failure occurred. + */ +static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev, + void *host, bool general_xlate, + const struct irq_domain_ops *intx_domain_ops, + struct device_node *local_intc) +{ + struct device_node *intc = local_intc; + struct irq_domain *domain; + const struct irq_domain_ops *irqd_ops; + bool need_put = false; + + if (!intc) { + intc = of_get_next_child(dev->of_node, NULL); + if (!intc) { + dev_err(dev, "missing child interrupt-controller node\n"); + return ERR_PTR(-EINVAL); + } + need_put = true; + } + + if (intx_domain_ops) { + irqd_ops = intx_domain_ops; + } else { + if (general_xlate) + pci_intx_domain_ops.xlate = &pci_irqd_intx_xlate; + irqd_ops = &pci_intx_domain_ops; + } + + domain = irq_domain_add_linear(intc, PCI_NUM_INTX, + irqd_ops, host); + if (!domain) { + dev_err(dev, "failed to get a INTx IRQ domain\n"); + if (need_put) + of_node_put(intc); + return ERR_PTR(-EINVAL); + } + + return domain; +} + #ifdef CONFIG_PCIEPORTBUS extern bool pcie_ports_disabled; #else @@ -1683,6 +1753,10 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d, unsigned long *out_hwirq, unsigned int *out_type) { return -EINVAL; } + +static __maybe_unused struct irq_domain *pci_alloc_intx_irqd(struct device *dev, + void *host, bool general_xlate, const struct irq_domain_ops *ops) +{ return ERR_PTR(-EINVAL); } #endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
It seems host drivers which allocate irq domain for INTx and the related code block looks highly similar to each other. Add a new helper, pci_alloc_intx_irqd(), to avoid code duplication as much as possible. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- include/linux/pci.h | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)