linux-kernel - Re: [PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after servicing interrupt

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160609144740.GG2385@dell>
Date:	Thu, 9 Jun 2016 15:47:40 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	broonie@...nel.org, gregkh@...uxfoundation.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mfd: max77620: Add pre/post irq handler before/after
 servicing interrupt

On Wed, 08 Jun 2016, Laxman Dewangan wrote:

> Hi Lee,
> 
> On Wednesday 08 June 2016 08:11 PM, Lee Jones wrote:
> >On Fri, 20 May 2016, Laxman Dewangan wrote:
> >
> >>+ * MAX77620 and MAX20024 has the following steps of the interrupt handling
> >>+ * for TOP interrupts:
> >>+ * 1. When interrupt occurs from PMIC, mask the PMIC interrupt by setting GLBLM.
> >>+ * 2. Read IRQTOP and service the interrupt.
> >>+ * 3. Once all interrupts has been checked and serviced, the interrupt service
> >>+ *    routine un-masks the hardware interrupt line by clearing GLBLM.
> >>+ */
> >>+static int max77620_top_irq_chip_pre_irq_handler(void *irq_drv_data)
> >>+{
> >>+	struct max77620_chip *chip = irq_drv_data;
> >>+	int ret;
> >>+
> >>+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
> >>+				 MAX77620_GLBLM_MASK, MAX77620_GLBLM_MASK);
> >>+	if (ret < 0)
> >>+		dev_err(chip->dev, "Failed to set GLBLM: %d\n", ret);
> >>+
> >>+	return ret;
> >>+}
> >>+
> >>+static int max77620_top_irq_chip_post_irq_handler(void *irq_drv_data)
> >>+{
> >>+	struct max77620_chip *chip = irq_drv_data;
> >>+	int ret;
> >>+
> >>+	ret = regmap_update_bits(chip->rmap, MAX77620_REG_INTENLBT,
> >>+				 MAX77620_GLBLM_MASK, 0);
> >>+	if (ret < 0)
> >>+		dev_err(chip->dev, "Failed to reset GLBLM: %d\n", ret);
> >>+
> >>+	return ret;
> >>+}
> >This seems massively over compacted.  All you're effectively doing
> >here is masking and unmasking the IRQs, which we do almost
> >ubiquitously with interrupt controllers.  Can't you just call the
> >functions "max77629_{un}mask_irqs()"?
> >
> >
> 
> Actually, per PMIC HW design, we need to toggle this bit on ISRs. Before
> reading the status, need to set 1 and then after handling it need to set 0.
> This cannot be done by any other bit toggling or masking/unmasking interrupt
> controller interrupt.
> 
> This is hard requirement from the PMIC chip.

I'm fine with the implementation, just rename the functions to something
less descriptive.  I suggested a suitable simplified alternative above.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog