| 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: <20091115222235.GB8367@sortiz.org>
Date: Sun, 15 Nov 2009 23:22:36 +0100
From: Samuel Ortiz <sameo@...ux.intel.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Balaji Rao <balajirrao@...nmoko.org>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mfd: pcf50633 - use threaded interrupts
Hi Dmitry,
On Mon, Nov 09, 2009 at 01:25:21AM -0800, Dmitry Torokhov wrote:
> Switch to using theraded IRQs instead of implementing custom solution
> with regular inettrup dandler and a custom workqueue.
Same as the first patch, this one contains both a fairly intrusibe change
along with comment and whitespace cleanups. I'd appreciate if you could remove
those cleanup parts from this patch.
Also, Balaji could you please give this patch a try before I apply it ?
Cheers,
Samuel.
> Signed-off-by: Dmitry Torokhov <dtor@...l.ru>
> ---
>
> drivers/mfd/pcf50633-core.c | 87 +++++++++++--------------------------
> include/linux/mfd/pcf50633/core.h | 6 +--
> 2 files changed, 29 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
> index a3d0226..4aeefa2 100644
> --- a/drivers/mfd/pcf50633-core.c
> +++ b/drivers/mfd/pcf50633-core.c
> @@ -126,7 +126,6 @@ int pcf50633_reg_set_bit_mask(struct pcf50633 *pcf, u8 reg, u8 mask, u8 val)
>
> out:
> mutex_unlock(&pcf->lock);
> -
> return ret;
> }
> EXPORT_SYMBOL_GPL(pcf50633_reg_set_bit_mask);
> @@ -324,13 +323,13 @@ static void pcf50633_irq_call_handler(struct pcf50633 *pcf, int irq)
> /* Maximum amount of time ONKEY is held before emergency action is taken */
> #define PCF50633_ONKEY1S_TIMEOUT 8
>
> -static void pcf50633_irq_worker(struct work_struct *work)
> +static irqreturn_t pcf50633_irq(int irq, void *data)
> {
> - struct pcf50633 *pcf;
> + struct pcf50633 *pcf = data;
> int ret, i, j;
> u8 pcf_int[5], chgstat;
>
> - pcf = container_of(work, struct pcf50633, irq_work);
> + dev_dbg(pcf->dev, "pcf50633_irq\n");
>
> /* Read the 5 INT regs in one transaction */
> ret = pcf50633_read_block(pcf, PCF50633_REG_INT1,
> @@ -345,8 +344,10 @@ static void pcf50633_irq_worker(struct work_struct *work)
> goto out;
> }
>
> - /* We immediately read the usb and adapter status. We thus make sure
> - * only of USBINS/USBREM IRQ handlers are called */
> + /*
> + * We immediately read the usb and adapter status. We thus make sure
> + * only of USBINS/USBREM IRQ handlers are called
> + */
> if (pcf_int[0] & (PCF50633_INT1_USBINS | PCF50633_INT1_USBREM)) {
> chgstat = pcf50633_reg_read(pcf, PCF50633_REG_MBCS2);
> if (chgstat & (0x3 << 4))
> @@ -364,15 +365,17 @@ static void pcf50633_irq_worker(struct work_struct *work)
> pcf_int[0] &= ~(1 << PCF50633_INT1_ADPINS);
> }
>
> - dev_dbg(pcf->dev, "INT1=0x%02x INT2=0x%02x INT3=0x%02x "
> - "INT4=0x%02x INT5=0x%02x\n", pcf_int[0],
> - pcf_int[1], pcf_int[2], pcf_int[3], pcf_int[4]);
> + dev_dbg(pcf->dev,
> + "INT1=0x%02x INT2=0x%02x INT3=0x%02x INT4=0x%02x INT5=0x%02x\n",
> + pcf_int[0], pcf_int[1], pcf_int[2], pcf_int[3], pcf_int[4]);
>
> - /* Some revisions of the chip don't have a 8s standby mode on
> - * ONKEY1S press. We try to manually do it in such cases. */
> + /*
> + * Some revisions of the chip don't have a 8s standby mode on
> + * ONKEY1S press. We try to manually do it in such cases.
> + */
> if ((pcf_int[0] & PCF50633_INT1_SECOND) && pcf->onkey1s_held) {
> - dev_info(pcf->dev, "ONKEY1S held for %d secs\n",
> - pcf->onkey1s_held);
> + dev_info(pcf->dev,
> + "ONKEY1S held for %d secs\n", pcf->onkey1s_held);
> if (pcf->onkey1s_held++ == PCF50633_ONKEY1S_TIMEOUT)
> if (pcf->pdata->force_shutdown)
> pcf->pdata->force_shutdown(pcf);
> @@ -409,8 +412,8 @@ static void pcf50633_irq_worker(struct work_struct *work)
> }
>
> /* Have we just resumed ? */
> - if (pcf->is_suspended) {
> - pcf->is_suspended = 0;
> + if (unlikely(pcf->is_suspended)) {
> + pcf->is_suspended = false;
>
> /* Set the resume reason filtering out non resumers */
> for (i = 0; i < ARRAY_SIZE(pcf_int); i++)
> @@ -432,20 +435,6 @@ static void pcf50633_irq_worker(struct work_struct *work)
> }
>
> out:
> - put_device(pcf->dev);
> - enable_irq(pcf->irq);
> -}
> -
> -static irqreturn_t pcf50633_irq(int irq, void *data)
> -{
> - struct pcf50633 *pcf = data;
> -
> - dev_dbg(pcf->dev, "pcf50633_irq\n");
> -
> - get_device(pcf->dev);
> - disable_irq_nosync(pcf->irq);
> - queue_work(pcf->work_queue, &pcf->irq_work);
> -
> return IRQ_HANDLED;
> }
>
> @@ -538,13 +527,9 @@ static int pcf50633_suspend(struct device *dev, pm_message_t state)
>
> pcf = dev_get_drvdata(dev);
>
> - /* Make sure our interrupt handlers are not called
> - * henceforth */
> + /* Make sure our interrupt handlers are not called henceforth. */
> disable_irq(pcf->irq);
>
> - /* Make sure that any running IRQ worker has quit */
> - cancel_work_sync(&pcf->irq_work);
> -
> /* Save the masks */
> ret = pcf50633_read_block(pcf, PCF50633_REG_INT1M,
> ARRAY_SIZE(pcf->suspend_irq_masks),
> @@ -565,7 +550,7 @@ static int pcf50633_suspend(struct device *dev, pm_message_t state)
> goto out;
> }
>
> - pcf->is_suspended = 1;
> + pcf->is_suspended = true;
>
> out:
> return ret;
> @@ -573,11 +558,9 @@ out:
>
> static int pcf50633_resume(struct device *dev)
> {
> - struct pcf50633 *pcf;
> + struct pcf50633 *pcf = dev_get_drvdata(dev);
> int ret;
>
> - pcf = dev_get_drvdata(dev);
> -
> /* Write the saved mask registers */
> ret = pcf50633_write_block(pcf, PCF50633_REG_INT1M,
> ARRAY_SIZE(pcf->suspend_irq_masks),
> @@ -585,16 +568,11 @@ static int pcf50633_resume(struct device *dev)
> if (ret < 0)
> dev_err(pcf->dev, "Error restoring saved suspend masks\n");
>
> - /* Restore regulators' state */
> -
> -
> - get_device(pcf->dev);
> -
> /*
> * Clear any pending interrupts and set resume reason if any.
> - * This will leave with enable_irq()
> */
> - pcf50633_irq_worker(&pcf->irq_work);
> + pcf50633_irq(pcf->irq, pcf);
> + enable_irq(pcf->irq);
>
> return 0;
> }
> @@ -628,21 +606,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
> pcf->i2c_client = client;
> pcf->irq = client->irq;
>
> - pcf->work_queue = create_singlethread_workqueue("pcf50633");
> - if (!pcf->work_queue) {
> - dev_err(pcf->dev, "Failed to create workqueue\n");
> - error = -ENOMEM;
> - goto err_free_mem;
> - }
> -
> - INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
> -
> version = pcf50633_reg_read(pcf, 0);
> variant = pcf50633_reg_read(pcf, 1);
> if (version < 0 || variant < 0) {
> dev_err(pcf->dev, "Unable to probe pcf50633\n");
> error = -ENODEV;
> - goto err_destroy_wq;
> + goto err_free_mem;
> }
>
> dev_info(pcf->dev, "Probed device version %d variant %d\n",
> @@ -682,8 +651,9 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
> goto err_free_pdevs;
> }
>
> - error = request_irq(client->irq, pcf50633_irq,
> - IRQF_TRIGGER_LOW, "pcf50633", pcf);
> + error = request_threaded_irq(client->irq, NULL, pcf50633_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "pcf50633", pcf);
> if (error) {
> dev_err(pcf->dev, "Failed to request IRQ %d\n", error);
> goto err_free_pdevs;
> @@ -709,8 +679,6 @@ err_free_irq:
> free_irq(pcf->irq, pcf);
> err_free_pdevs:
> pcf50633_unregister_sub_devices(pcf);
> -err_destroy_wq:
> - destroy_workqueue(pcf->work_queue);
> err_free_mem:
> kfree(pcf);
> return error;
> @@ -722,7 +690,6 @@ static int __devexit pcf50633_remove(struct i2c_client *client)
>
>
> free_irq(pcf->irq, pcf);
> - destroy_workqueue(pcf->work_queue);
>
> pcf50633_unregister_sub_devices(pcf);
>
> diff --git a/include/linux/mfd/pcf50633/core.h b/include/linux/mfd/pcf50633/core.h
> index 0c21222..04990c4 100644
> --- a/include/linux/mfd/pcf50633/core.h
> +++ b/include/linux/mfd/pcf50633/core.h
> @@ -13,8 +13,8 @@
> #ifndef __LINUX_MFD_PCF50633_CORE_H
> #define __LINUX_MFD_PCF50633_CORE_H
>
> +#include <linux/types.h>
> #include <linux/i2c.h>
> -#include <linux/workqueue.h>
> #include <linux/regulator/driver.h>
> #include <linux/regulator/machine.h>
> #include <linux/power_supply.h>
> @@ -141,15 +141,13 @@ struct pcf50633 {
> struct pcf50633_platform_data *pdata;
> int irq;
> struct pcf50633_irq irq_handler[PCF50633_NUM_IRQ];
> - struct work_struct irq_work;
> - struct workqueue_struct *work_queue;
> struct mutex lock;
>
> u8 mask_regs[5];
>
> u8 suspend_irq_masks[5];
> u8 resume_reason[5];
> - int is_suspended;
> + bool is_suspended;
>
> int onkey1s_held;
>
>
--
Intel Open Source Technology Centre
http://oss.intel.com/
--
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