| 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: <51EFD896.2010204@ti.com>
Date: Wed, 24 Jul 2013 16:37:26 +0300
From: Grygorii Strashko <grygorii.strashko@...com>
To: Lee Jones <lee.jones@...aro.org>
CC: Samuel Ortiz <sameo@...ux.intel.com>,
Kevin Hilman <khilman@...aro.org>,
Graeme Gregory <gg@...mlogic.co.uk>,
<linux-omap@...r.kernel.org>,
Ruslan Bilovol <ruslan.bilovol@...com>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/4] mfd: twl6030-irq: convert to use linear irq_domain
On 07/24/2013 02:35 PM, Lee Jones wrote:
> On Tue, 23 Jul 2013, Grygorii Strashko wrote:
>
>> Since the TWL6030 PMIC is used with OMAP4 SoCs only and OMAP4 legacy
>> boot is dropped there are no needs to allocate the range of IRQ
>> descriptors during system boot to support TWL6030 IRQs.
>>
>> Hence, convert it to use linear irq_domain and move IRQ configuration in
>> .map()/.unmap() callbacks of irq_domain. So, IRQ mapping and descriptors
>> allocation will be performed dynamically basing on DT configuration.
>>
>> The error message will be reported in case if unmapped IRQ is received by
>> TWL6030 (virq==0).
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
>> ---
>> drivers/mfd/twl6030-irq.c | 114 ++++++++++++++++++++++++---------------------
>> 1 file changed, 61 insertions(+), 53 deletions(-)
>>
>> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
>> index 790cc28..89f130b 100644
>> --- a/drivers/mfd/twl6030-irq.c
>> +++ b/drivers/mfd/twl6030-irq.c
>> @@ -138,6 +138,7 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>> static irqreturn_t twl6030_irq_thread(int irq, void *data)
>> {
>> int i, ret;
>> + struct irq_domain *irq_domain = (struct irq_domain *)data;
>> union {
>> u8 bytes[4];
>> u32 int_sts;
>> @@ -161,9 +162,14 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>
>> for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>> if (sts.int_sts & 0x1) {
>> - int module_irq = twl6030_irq_base +
>> - twl6030_interrupt_mapping[i];
>> - handle_nested_irq(module_irq);
>> + int module_irq =
>> + irq_find_mapping(irq_domain,
>> + twl6030_interrupt_mapping[i]);
>> + if (module_irq)
>> + handle_nested_irq(module_irq);
>> + else
>> + pr_err("%s: Unmapped PIH ISR %u detected\n",
>> + __func__, i);
>
> Same here. Does the user really care which function failed?
>
> Please consider replacing with the device name.
ok.
>
>> pr_debug("%s: PIH ISR %u, virq%u\n",
>> __func__, i, module_irq);
>> }
>> @@ -186,19 +192,6 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>>
>> /*----------------------------------------------------------------------*/
>>
>> -static inline void activate_irq(int irq)
>> -{
>> -#ifdef CONFIG_ARM
>> - /* ARM requires an extra step to clear IRQ_NOREQUEST, which it
>> - * sets on behalf of every irq_chip. Also sets IRQ_NOPROBE.
>> - */
>> - set_irq_flags(irq, IRQF_VALID);
>> -#else
>> - /* same effect on other architectures */
>> - irq_set_noprobe(irq);
>> -#endif
>> -}
>> -
>> static int twl6030_irq_set_wake(struct irq_data *d, unsigned int on)
>> {
>> if (on)
>> @@ -308,28 +301,54 @@ int twl6030_mmc_card_detect(struct device *dev, int slot)
>> }
>> EXPORT_SYMBOL(twl6030_mmc_card_detect);
>>
>> +static struct irq_chip twl6030_irq_chip;
>> +
>> +static int twl6030_irq_map(struct irq_domain *d, unsigned int virq,
>> + irq_hw_number_t hwirq)
>> +{
>> + irq_set_chip_data(virq, &twl6030_irq_chip);
>> + irq_set_chip_and_handler(virq, &twl6030_irq_chip, handle_simple_irq);
>> + irq_set_nested_thread(virq, true);
>> +
>> +#ifdef CONFIG_ARM
>> + /*
>> + * ARM requires an extra step to clear IRQ_NOREQUEST, which it
>> + * sets on behalf of every irq_chip. Also sets IRQ_NOPROBE.
>> + */
>> + set_irq_flags(virq, IRQF_VALID);
>> +#else
>> + /* same effect on other architectures */
>> + irq_set_noprobe(virq);
>> +#endif
>> +
>> + return 0;
>> +}
>> +
>> +static void twl6030_irq_unmap(struct irq_domain *d, unsigned int virq)
>> +{
>> +#ifdef CONFIG_ARM
>> + set_irq_flags(virq, 0);
>> +#endif
>> + irq_set_chip_and_handler(virq, NULL, NULL);
>> + irq_set_chip_data(virq, NULL);
>> +}
>> +
>> +static struct irq_domain_ops twl6030_irq_domain_ops = {
>> + .map = twl6030_irq_map,
>> + .unmap = twl6030_irq_unmap,
>> + .xlate = irq_domain_xlate_onetwocell,
>> +};
>> +
>> int twl6030_init_irq(struct device *dev, int irq_num)
>> {
>> struct device_node *node = dev->of_node;
>> - int nr_irqs, irq_base, irq_end;
>> - static struct irq_chip twl6030_irq_chip;
>> + int nr_irqs;
>> int status;
>> - int i;
>> u8 mask[3];
>> + struct irq_domain *irq_domain;
>>
>> nr_irqs = TWL6030_NR_IRQS;
>>
>> - irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
>> - if (IS_ERR_VALUE(irq_base)) {
>> - dev_err(dev, "Fail to allocate IRQ descs\n");
>> - return irq_base;
>> - }
>> -
>> - irq_domain_add_legacy(node, nr_irqs, irq_base, 0,
>> - &irq_domain_simple_ops, NULL);
>> -
>> - irq_end = irq_base + nr_irqs;
>> -
>> mask[0] = 0xFF;
>> mask[1] = 0xFF;
>> mask[2] = 0xFF;
>> @@ -346,8 +365,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>> return status;
>> }
>>
>> - twl6030_irq_base = irq_base;
>> -
>> /*
>> * install an irq handler for each of the modules;
>> * clone dummy irq_chip since PIH can't *do* anything
>> @@ -357,20 +374,18 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>> twl6030_irq_chip.irq_set_type = NULL;
>> twl6030_irq_chip.irq_set_wake = twl6030_irq_set_wake;
>>
>> - for (i = irq_base; i < irq_end; i++) {
>> - irq_set_chip_and_handler(i, &twl6030_irq_chip,
>> - handle_simple_irq);
>> - irq_set_chip_data(i, (void *)irq_num);
>> - irq_set_nested_thread(i, true);
>> - activate_irq(i);
>> + irq_domain = irq_domain_add_linear(node, nr_irqs,
>> + &twl6030_irq_domain_ops, NULL);
>> + if (!irq_domain) {
>> + dev_err(dev, "Can't add irq_domain\n");
>> + return -ENOMEM;
>> }
>>
>> - dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
>> - irq_num, irq_base, irq_end);
>> + dev_info(dev, "PIH (irq %d) nested IRQs\n", irq_num);
>>
>> /* install an irq handler to demultiplex the TWL6030 interrupt */
>> status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
>> - IRQF_ONESHOT, "TWL6030-PIH", NULL);
>> + IRQF_ONESHOT, "TWL6030-PIH", irq_domain);
>> if (status < 0) {
>> dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>> goto fail_irq;
>> @@ -378,26 +393,19 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>>
>> twl_irq = irq_num;
>> register_pm_notifier(&twl6030_irq_pm_notifier_block);
>> - return irq_base;
>> + return irq_num;
>
> I think you need to change twl-core to now expect the total number of
> IRQs rather than the base one now.
Would it be ok if twl6030_init_irq() will return 0 on success or error
code on failure?
The change of twl6030_init_irq() will not affect twl-core in case of
TWL6030/32 DT-boot --- not DT boot (legacy mode) seems need to be
dropped form twl-core for TWL6030/32.
>
>> fail_irq:
>> - for (i = irq_base; i < irq_end; i++)
>> - irq_set_chip_and_handler(i, NULL, NULL);
>> -
>> + irq_domain_remove(irq_domain);
>
> Why do you kill the irqdomain here, but not in exit()?
this is a failure path and twl_irq == 0, so exit will do nothing,
and it safe to delete irq_domain here, because no child devices will be
created.
>
>> return status;
>> }
>>
>> int twl6030_exit_irq(void)
>> {
>> - unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>> -
>> - if (!twl6030_irq_base) {
>> - pr_err("twl6030: can't yet clean up IRQs?\n");
>> - return -ENOSYS;
>> + if (twl_irq) {
>> + unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>> + free_irq(twl_irq, NULL);
In general, I need to add irq_domain_remove(irq_domain) here too
But, there is a big BUT!
>> }
>
> Ah yes, that's better.
>
>> -
>> - free_irq(twl_irq, NULL);
>> -
>> return 0;
>> }
>>
>
Unfortunately, I'm not fully understand how to dispose IRQ mapping and
free IRQ domain/descriptors correctly.
- IRQ mapping is done when devices are being created from DT (including
IRQ desc allocation)
- there is API irq_dispose_mapping(), but It's not been called from
irq_domain_remove() and OF and Driver cores.
- there is no of_platform_unpopulate() yet
So, I can call irq_dispose_mapping() manually from twl6030_exit_irq, but
child devices will still keep mapped IRQ numbers as their Resources.
:((
More over, in this case I can't use devm_*() API to request IRQ, because
free_irq() will be called after irq_domian de-initialization.
That's why I've not added irq_domian de-initialization in
twl6030_exit_irq() and not used devm_*() to request IRQ.
Regards,
- grygorii
--
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