| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0a8abf10-bf0b-88ae-64c5-5ad1815cbec6@sigmadesigns.com>
Date: Wed, 31 May 2017 09:32:12 +0200
From: Marc Gonzalez <marc_gonzalez@...madesigns.com>
To: Marc Zyngier <marc.zyngier@....com>,
Thomas Gleixner <tglx@...utronix.de>
CC: Bjorn Helgaas <helgaas@...nel.org>,
Robin Murphy <robin.murphy@....com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Liviu Dudau <liviu.dudau@....com>,
David Laight <david.laight@...lab.com>,
linux-pci <linux-pci@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
Thibaud Cornic <thibaud_cornic@...madesigns.com>,
Phuong Nguyen <phuong_nguyen@...madesigns.com>,
LKML <linux-kernel@...r.kernel.org>, Mason <slash.tmp@...e.fr>
Subject: Re: [PATCH v4 1/2] PCI: Add tango MSI controller support
On 25/05/2017 10:37, Marc Zyngier wrote:
> On 20/04/17 15:28, Marc Gonzalez wrote:
>
>> The MSI controller in Tango supports 256 message-signaled interrupts,
>> and a single doorbell address.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@...madesigns.com>
>> ---
>> drivers/pci/host/pcie-tango.c | 232 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 232 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-tango.c b/drivers/pci/host/pcie-tango.c
>> new file mode 100644
>> index 000000000000..ada8d35066ab
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-tango.c
>
> As Bjorn mentioned elsewhere, the ordering of the patch is backward.
> Please have the PCIe host controller driver in the first patch, then the
> MSI stuff. Otherwise, it is impossible to see how the various bits are
> wired.
Done.
> And please have a separate patch the the DT binding, which needs
> separate Acks from the DT folks.
Done.
>> @@ -0,0 +1,232 @@
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/pci-ecam.h>
>> +#include <linux/delay.h>
>> +#include <linux/msi.h>
>> +
>> +#define MSI_MAX 256
>> +
>> +struct tango_pcie {
>> + DECLARE_BITMAP(bitmap, MSI_MAX);
>> + spinlock_t lock;
>> + void __iomem *mux;
>> + void __iomem *msi_status;
>> + void __iomem *msi_enable;
>> + phys_addr_t msi_doorbell;
>
> Init of these three fields should be in this patch.
Done.
>> + struct irq_domain *irq_dom;
>> + struct irq_domain *msi_dom;
>> + int irq;
>> +};
>> +
>> +/*** MSI CONTROLLER SUPPORT ***/
>> +
>> +static void dispatch(struct tango_pcie *pcie, unsigned long status, int base)
>> +{
>> + unsigned int pos, virq;
>> +
>> + for_each_set_bit(pos, &status, 32) {
>> + virq = irq_find_mapping(pcie->irq_dom, base + pos);
>> + generic_handle_irq(virq);
>> + }
>> +}
>> +
>> +static void tango_msi_isr(struct irq_desc *desc)
>> +{
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct tango_pcie *pcie = irq_desc_get_handler_data(desc);
>> + unsigned int status, base, pos = 0;
>> +
>> + chained_irq_enter(chip, desc);
>> +
>> + while ((pos = find_next_bit(pcie->bitmap, MSI_MAX, pos)) < MSI_MAX) {
>> + base = round_down(pos, 32);
>> + status = readl_relaxed(pcie->msi_status + base / 8);
>> + dispatch(pcie, status, base);
>
> Just inline dispatch here.
Done.
>> + pos = base + 32;
>> + }
>> +
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void tango_ack(struct irq_data *data)
>> +{
>> + u32 bit = BIT(data->hwirq);
>
> How does this work when hwirq is >= 32 (from what I gather, it can range
> from 0 to 255...).
It blows up. Fixed now.
>> + struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
>> +
>> + writel_relaxed(bit, pcie->msi_status);
>> +}
>> +
>> +static void update_msi_enable(struct irq_data *data, bool unmask)
>> +{
>> + unsigned long flags;
>> + u32 val, bit = BIT(data->hwirq % 32);
>> + int byte_offset = (data->hwirq / 32) * 4;
>> + struct tango_pcie *pcie = data->chip_data;
>> +
>> + spin_lock_irqsave(&pcie->lock, flags);
>> + val = readl_relaxed(pcie->msi_enable + byte_offset);
>
> As I already mentioned in one of the previous series, this is a fairly
> pointless MMIO access. You could maintain a SW version of the enable
> register, update that copy and write it.
>
> But that's only a performance optimization, and it won't affect the
> behaviour. Your call.
Thanks for pointing it out.
I'll implement the optimization in a future patch.
>> + val = unmask ? val | bit : val & ~bit;
>> + writel_relaxed(val, pcie->msi_enable + byte_offset);
>> + spin_unlock_irqrestore(&pcie->lock, flags);
>> +}
>> +
>> +static void tango_mask(struct irq_data *data)
>> +{
>> + update_msi_enable(data, false);
>> +}
>> +
>> +static void tango_unmask(struct irq_data *data)
>> +{
>> + update_msi_enable(data, true);
>> +}
>> +
>> +static int tango_set_affinity(struct irq_data *data,
>> + const struct cpumask *mask, bool force)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static void tango_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>> +{
>> + struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
>> +
>> + msg->address_lo = lower_32_bits(pcie->msi_doorbell);
>> + msg->address_hi = upper_32_bits(pcie->msi_doorbell);
>> + msg->data = data->hwirq;
>> +}
>> +
>> +static struct irq_chip tango_chip = {
>> + .irq_ack = tango_ack,
>> + .irq_mask = tango_mask,
>> + .irq_unmask = tango_unmask,
>> + .irq_set_affinity = tango_set_affinity,
>> + .irq_compose_msi_msg = tango_compose_msi_msg,
>> +};
>> +
>> +static void msi_ack(struct irq_data *data)
>> +{
>> + irq_chip_ack_parent(data);
>> +}
>> +
>> +static void msi_mask(struct irq_data *data)
>> +{
>> + pci_msi_mask_irq(data);
>> + irq_chip_mask_parent(data);
>> +}
>> +
>> +static void msi_unmask(struct irq_data *data)
>> +{
>> + pci_msi_unmask_irq(data);
>> + irq_chip_unmask_parent(data);
>> +}
>> +
>> +static struct irq_chip msi_chip = {
>> + .name = "MSI",
>> + .irq_ack = msi_ack,
>> + .irq_mask = msi_mask,
>> + .irq_unmask = msi_unmask,
>> +};
>> +
>> +static struct msi_domain_info msi_dom_info = {
>> + .flags = MSI_FLAG_PCI_MSIX
>> + | MSI_FLAG_USE_DEF_DOM_OPS
>> + | MSI_FLAG_USE_DEF_CHIP_OPS,
>> + .chip = &msi_chip,
>> +};
>> +
>> +static int find_free_msi(struct irq_domain *dom, unsigned int virq)
>> +{
>> + unsigned int pos;
>> + struct tango_pcie *pcie = dom->host_data;
>> +
>> + pos = find_first_zero_bit(pcie->bitmap, MSI_MAX);
>> + if (pos >= MSI_MAX)
>> + return -ENOSPC;
>> + __set_bit(pos, pcie->bitmap);
>> +
>> + irq_domain_set_info(dom, virq, pos, &tango_chip, pcie,
>> + handle_edge_irq, NULL, NULL);
>> +
>> + return 0;
>> +}
>> +
>> +static int tango_irq_domain_alloc(struct irq_domain *dom,
>> + unsigned int virq, unsigned int nr_irqs, void *args)
>> +{
>> + int err;
>> + unsigned long flags;
>> + struct tango_pcie *pcie = dom->host_data;
>> +
>> + spin_lock_irqsave(&pcie->lock, flags);
>> + err = find_free_msi(dom, virq);
>
> Just inline find_free_msi here. it is not called from anywhere else, and
> seeing the locking close to the use of the bitmap is definitely better.
Done.
>> + spin_unlock_irqrestore(&pcie->lock, flags);
>> +
>> + return err;
>> +}
>> +
>> +static void tango_irq_domain_free(struct irq_domain *dom,
>> + unsigned int virq, unsigned int nr_irqs)
>> +{
>> + unsigned long flags;
>> + struct irq_data *data = irq_domain_get_irq_data(dom, virq);
>> + struct tango_pcie *pcie = irq_data_get_irq_chip_data(data);
>> +
>> + spin_lock_irqsave(&pcie->lock, flags);
>> + __clear_bit(data->hwirq, pcie->bitmap);
>> + spin_unlock_irqrestore(&pcie->lock, flags);
>> +}
>> +
>> +static const struct irq_domain_ops irq_dom_ops = {
>> + .alloc = tango_irq_domain_alloc,
>> + .free = tango_irq_domain_free,
>> +};
>> +
>> +static int tango_msi_remove(struct platform_device *pdev)
>> +{
>> + struct tango_pcie *pcie = platform_get_drvdata(pdev);
>> +
>> + irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
>> + irq_domain_remove(pcie->msi_dom);
>> + irq_domain_remove(pcie->irq_dom);
>> +
>> + return 0;
>> +}
>> +
>> +static int tango_msi_probe(struct platform_device *pdev, struct tango_pcie *pcie)
>> +{
>> + int i, virq;
>> + struct irq_domain *msi_dom, *irq_dom;
>> + struct fwnode_handle *fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +
>> + spin_lock_init(&pcie->lock);
>> + for (i = 0; i < MSI_MAX / 32; ++i)
>> + writel_relaxed(0, pcie->msi_enable + i * 4);
>> +
>> + irq_dom = irq_domain_create_linear(fwnode, MSI_MAX, &irq_dom_ops, pcie);
>> + if (!irq_dom) {
>> + pr_err("Failed to create IRQ domain\n");
>> + return -ENOMEM;
>> + }
>> +
>> + msi_dom = pci_msi_create_irq_domain(fwnode, &msi_dom_info, irq_dom);
>> + if (!msi_dom) {
>> + pr_err("Failed to create MSI domain\n");
>> + irq_domain_remove(irq_dom);
>> + return -ENOMEM;
>> + }
>> +
>> + virq = platform_get_irq(pdev, 1);
>> + if (virq <= 0) {
>> + pr_err("Failed to map IRQ\n");
>> + irq_domain_remove(msi_dom);
>> + irq_domain_remove(irq_dom);
>> + return -ENXIO;
>> + }
>
> Maybe start the probe with this. No need to start allocating data
> structures if the DT is wrong.
Done.
Powered by blists - more mailing lists