| 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: <92b37bf7-654f-14c0-1f1c-64a1cd12427a@codeaurora.org>
Date: Mon, 23 Aug 2021 13:25:58 +0530
From: Maulik Shah <mkshah@...eaurora.org>
To: Marc Zyngier <maz@...nel.org>
Cc: tglx@...utronix.de, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-gpio@...r.kernel.org,
bjorn.andersson@...aro.org, linus.walleij@...aro.org,
tkjos@...gle.com, lsrao@...eaurora.org
Subject: Re: [PATCH v2 3/3] irqchip/qcom-pdc: Start getting rid of the
GPIO_NO_WAKE_IRQ
Hi,
On 8/20/2021 9:08 PM, Marc Zyngier wrote:
> On Thu, 19 Aug 2021 12:53:13 +0100,
> Maulik Shah <mkshah@...eaurora.org> wrote:
>> From: Marc Zyngier <maz@...nel.org>
>>
>> gpio_to_irq() reports error at irq_domain_trim_hierarchy() for non
>> wakeup capable GPIOs that do not have dedicated interrupt at GIC.
>>
>> Since PDC irqchip do not allocate irq at parent GIC domain for such
>> GPIOs indicate same by using irq_domain_disconnect_hierarchy() for
>> PDC and parent GIC domains.
>>
>> Signed-off-by: Marc Zyngier <maz@...nel.org>
>> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
>> [mkshah: Add loop to disconnect for all parents]
>> Tested-by: Maulik Shah <mkshah@...eaurora.org>
>> ---
>> drivers/irqchip/qcom-pdc.c | 75 +++++++++++-----------------------------------
>> 1 file changed, 18 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
>> index 32d5920..696afca 100644
>> --- a/drivers/irqchip/qcom-pdc.c
>> +++ b/drivers/irqchip/qcom-pdc.c
>> @@ -53,26 +53,6 @@ static u32 pdc_reg_read(int reg, u32 i)
>> return readl_relaxed(pdc_base + reg + i * sizeof(u32));
>> }
>>
>> -static int qcom_pdc_gic_get_irqchip_state(struct irq_data *d,
>> - enum irqchip_irq_state which,
>> - bool *state)
>> -{
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return 0;
>> -
>> - return irq_chip_get_parent_state(d, which, state);
>> -}
>> -
>> -static int qcom_pdc_gic_set_irqchip_state(struct irq_data *d,
>> - enum irqchip_irq_state which,
>> - bool value)
>> -{
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return 0;
>> -
>> - return irq_chip_set_parent_state(d, which, value);
>> -}
>> -
>> static void pdc_enable_intr(struct irq_data *d, bool on)
>> {
>> int pin_out = d->hwirq;
>> @@ -91,38 +71,16 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
>>
>> static void qcom_pdc_gic_disable(struct irq_data *d)
>> {
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> -
>> pdc_enable_intr(d, false);
>> irq_chip_disable_parent(d);
>> }
>>
>> static void qcom_pdc_gic_enable(struct irq_data *d)
>> {
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> -
>> pdc_enable_intr(d, true);
>> irq_chip_enable_parent(d);
>> }
>>
>> -static void qcom_pdc_gic_mask(struct irq_data *d)
>> -{
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> -
>> - irq_chip_mask_parent(d);
>> -}
>> -
>> -static void qcom_pdc_gic_unmask(struct irq_data *d)
>> -{
>> - if (d->hwirq == GPIO_NO_WAKE_IRQ)
>> - return;
>> -
>> - irq_chip_unmask_parent(d);
>> -}
>> -
>> /*
>> * GIC does not handle falling edge or active low. To allow falling edge and
>> * active low interrupts to be handled at GIC, PDC has an inverter that inverts
>> @@ -159,14 +117,10 @@ enum pdc_irq_config_bits {
>> */
>> static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>> {
>> - int pin_out = d->hwirq;
>> enum pdc_irq_config_bits pdc_type;
>> enum pdc_irq_config_bits old_pdc_type;
>> int ret;
>>
>> - if (pin_out == GPIO_NO_WAKE_IRQ)
>> - return 0;
>> -
>> switch (type) {
>> case IRQ_TYPE_EDGE_RISING:
>> pdc_type = PDC_EDGE_RISING;
>> @@ -191,8 +145,8 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>> return -EINVAL;
>> }
>>
>> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, pin_out);
>> - pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>
>> ret = irq_chip_set_type_parent(d, type);
>> if (ret)
>> @@ -216,12 +170,12 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>> static struct irq_chip qcom_pdc_gic_chip = {
>> .name = "PDC",
>> .irq_eoi = irq_chip_eoi_parent,
>> - .irq_mask = qcom_pdc_gic_mask,
>> - .irq_unmask = qcom_pdc_gic_unmask,
>> + .irq_mask = irq_chip_mask_parent,
>> + .irq_unmask = irq_chip_unmask_parent,
>> .irq_disable = qcom_pdc_gic_disable,
>> .irq_enable = qcom_pdc_gic_enable,
>> - .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
>> - .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
>> + .irq_get_irqchip_state = irq_chip_get_parent_state,
>> + .irq_set_irqchip_state = irq_chip_set_parent_state,
>> .irq_retrigger = irq_chip_retrigger_hierarchy,
>> .irq_set_type = qcom_pdc_gic_set_type,
>> .flags = IRQCHIP_MASK_ON_SUSPEND |
>> @@ -282,7 +236,7 @@ static int qcom_pdc_alloc(struct irq_domain *domain, unsigned int virq,
>>
>> parent_hwirq = get_parent_hwirq(hwirq);
>> if (parent_hwirq == PDC_NO_PARENT_IRQ)
>> - return 0;
>> + return irq_domain_disconnect_hierarchy(domain->parent, virq);
>>
>> if (type & IRQ_TYPE_EDGE_BOTH)
>> type = IRQ_TYPE_EDGE_RISING;
>> @@ -314,22 +268,29 @@ static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
>> irq_hw_number_t hwirq, parent_hwirq;
>> unsigned int type;
>> int ret;
>> + struct irq_domain *parent;
>>
>> ret = qcom_pdc_translate(domain, fwspec, &hwirq, &type);
>> if (ret)
>> return ret;
>>
>> + if (hwirq == GPIO_NO_WAKE_IRQ) {
>> + for (parent = domain; parent; parent = parent->parent) {
>> + ret = irq_domain_disconnect_hierarchy(parent, virq);
>> + if (ret)
>> + return ret;
>> + }
>> + return 0;
>> + }
>> +
> No, this is wrong. Please read the documentation for
> irq_domain_disconnect_hierarchy(): the disconnect can only take place
> *once* per interrupt, right at the point where you need to terminate
> the hierarchy.
>
> irq_domain_trim_hierarchy() should already do the right thing by
> iterating over the domains and free the unused irq_data.
>
> M.
Thanks for the review. Seems we need disconnect only once per interrupt.
with this i see patch 2 in this series also of no use since it already
takes care of removing irq_data for all the parents.
Addressed in v3 series.
Thanks,
Maulik
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Powered by blists - more mailing lists