| 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
| ||
|
Date: Thu, 15 Oct 2015 09:03:36 -0500
From: Suravee Suthikulanit <suravee.suthikulpanit@....com>
To: Tomasz Nowicki <tn@...ihalf.com>, <marc.zyngier@....com>,
<tglx@...utronix.de>, <jason@...edaemon.net>, <rjw@...ysocki.net>
CC: Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Will Deacon <will.deacon@....com>,
Catalin Marinas <Catalin.Marinas@....com>,
<hanjun.guo@...aro.org>, <graeme.gregory@...aro.org>,
<dhdang@....com>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH V2 6/6] gicv2m: acpi: Introducing GICv2m ACPI support
Hi Tomasz,
Thanks for the feedback.
On 10/15/2015 1:15 AM, Tomasz Nowicki wrote:
>> [..]
>> @@ -138,6 +140,11 @@ static int gicv2m_irq_gic_domain_alloc(struct
>> irq_domain *domain,
>> fwspec.param[0] = 0;
>> fwspec.param[1] = hwirq - 32;
>> fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
>> + } else if (is_fwnode_irqchip(domain->parent->fwnode)) {
>> + fwspec.fwnode = domain->parent->fwnode;
>> + fwspec.param_count = 2;
>> + fwspec.param[0] = hwirq;
>> + fwspec.param[1] = IRQ_TYPE_EDGE_RISING & IRQ_TYPE_SENSE_MASK;
> How about just:
> fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
Right.
>
>> } else {
>> return -EINVAL;
>> }
>> @@ -255,6 +262,8 @@ static void gicv2m_teardown(void)
>> kfree(v2m->bm);
>> iounmap(v2m->base);
>> of_node_put(to_of_node(v2m->fwnode));
>> + if (is_fwnode_irqchip(v2m->fwnode))
>> + irq_domain_free_fwnode(v2m->fwnode);
>> kfree(v2m);
>> }
>> }
>> @@ -359,6 +368,8 @@ static int __init gicv2m_init_one(struct
>> fwnode_handle *fwnode,
>>
>> if (to_of_node(fwnode))
>> name = to_of_node(fwnode)->name;
>> + else
>> + name = irq_domain_get_irqchip_fwnode_name(fwnode);
>>
>> pr_info("Frame %s: range[%#lx:%#lx], SPI[%d:%d]\n", name,
>> (unsigned long)res->start, (unsigned long)res->end,
>> @@ -415,3 +426,86 @@ int __init gicv2m_of_init(struct device_node
>> *node, struct irq_domain *parent)
>> gicv2m_teardown();
>> return ret;
>> }
>> +
>> +#ifdef CONFIG_ACPI
>> +static int acpi_num_msi;
>> +
>> +static struct fwnode_handle *gicv2m_get_fwnode(struct device *dev)
>> +{
>> + struct v2m_data *data;
>> +
>> + if (WARN_ON(acpi_num_msi <= 0))
>> + return NULL;
>> +
>> + /* We only return the fwnode of the first MSI frame. */
>> + data = list_first_entry_or_null(&v2m_nodes,
>> + struct v2m_data, entry);
> This can be one line and still fits within 80 characters.
Ok.
>> + if (!data)
>> + return NULL;
>> +
>> + return data->fwnode;
>> +}
>> +
>> +static int __init
>> +acpi_parse_madt_msi(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + int ret;
>> + struct resource res;
>> + u32 spi_start = 0, nr_spis = 0;
>> + struct acpi_madt_generic_msi_frame *m;
>> + struct fwnode_handle *fwnode = NULL;
>> +
>> + m = (struct acpi_madt_generic_msi_frame *)header;
>> + if (BAD_MADT_ENTRY(m, end))
>> + return -EINVAL;
>> +
>> + res.start = m->base_address;
>> + res.end = m->base_address + 0x1000;
>> +
>> + if (m->flags & ACPI_MADT_OVERRIDE_SPI_VALUES) {
>> + spi_start = m->spi_base;
>> + nr_spis = m->spi_count;
>> +
>> + pr_info("ACPI overriding V2M MSI_TYPER (base:%u, num:%u)\n",
>> + spi_start, nr_spis);
>> + }
>> +
>> + fwnode = irq_domain_alloc_fwnode((void *)m->base_address);
>> + if (!fwnode) {
>> + pr_err("Unable to allocate GICv2m domain token\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = gicv2m_init_one(fwnode, spi_start, nr_spis, &res);
> I case of error, we should call here:
> irq_domain_free_fwnode(fwnode);
This should have already been handled when returning from the
acpi_parse_madt_msi() in gicv2m_teardown() since we would need to
iterate through all existing MSI frame to clean up.
>> +
>> + return ret;
>> +}
>> +
>> +int __init gicv2m_acpi_init(struct irq_domain *parent)
>> +{
>> + int ret;
>> +
>> + if (acpi_num_msi > 0)
>> + return 0;
>> +
>> + acpi_num_msi =
>> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_MSI_FRAME,
>> + acpi_parse_madt_msi, 0);
>> +
>> + if (acpi_num_msi <= 0)
>> + goto err_out;
> If acpi_table_parse_madt return 0, then we don't need to call
> gicv2m_teardown(). Instead we can simply return, optionally add some
> pr_info. Well, gicv2m_teardown would do nothing, so this is just
> cosmetic and up to you.
I'd be hesitate to add pr_info here since V2m is optional, we already
print information for each frame found. I think I would just leave this
one the way it is.
>> [..]
>> diff --git a/include/linux/irqchip/arm-gic.h
>> b/include/linux/irqchip/arm-gic.h
>> index bae69e5..7398538 100644
>> --- a/include/linux/irqchip/arm-gic.h
>> +++ b/include/linux/irqchip/arm-gic.h
>> @@ -108,6 +108,12 @@ void gic_init(unsigned int nr, int start,
>>
>> int gicv2m_of_init(struct device_node *node, struct irq_domain
>> *parent);
>>
>> +#ifdef CONFIG_ACPI
>> +#include <linux/acpi.h>
> I think, we don't need this include here. You already added it to itq-gic.c
Right.
Thanks,
Suravee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists