[PATCH] mfd: ezx-pcap: replace mutex_lock with spin_lock

[PATCH] mfd: ezx-pcap: replace mutex_lock with spin_lock

[Fuqian Huang]

As mutex_lock might sleep.
Function pcap_adc_irq is an interrupt handler.
The use of mutex_lock in pcap_adc_irq may cause sleep
in IRQ context.
Replace mutex_lock with spin_lock to avoid this.

Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
 drivers/mfd/ezx-pcap.c | 53 ++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index f505e3e1274b..70fa18b04ad2 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -35,7 +35,7 @@ struct pcap_chip {
 
 	/* IO */
 	u32 buf;
-	struct mutex io_mutex;
+	spinlock_t io_lock;
 
 	/* IRQ */
 	unsigned int irq_base;
@@ -48,7 +48,7 @@ struct pcap_chip {
 	struct pcap_adc_request *adc_queue[PCAP_ADC_MAXQ];
 	u8 adc_head;
 	u8 adc_tail;
-	struct mutex adc_mutex;
+	spinlock_t adc_lock;
 };
 
 /* IO */
@@ -76,14 +76,15 @@ static int ezx_pcap_putget(struct pcap_chip *pcap, u32 *data)
 
 int ezx_pcap_write(struct pcap_chip *pcap, u8 reg_num, u32 value)
 {
+	unsigned long flags;
 	int ret;
 
-	mutex_lock(&pcap->io_mutex);
+	spin_lock_irqsave(&pcap->io_lock, flags);
 	value &= PCAP_REGISTER_VALUE_MASK;
 	value |= PCAP_REGISTER_WRITE_OP_BIT
 		| (reg_num << PCAP_REGISTER_ADDRESS_SHIFT);
 	ret = ezx_pcap_putget(pcap, &value);
-	mutex_unlock(&pcap->io_mutex);
+	spin_unlock_irqrestore(&pcap->io_lock, flags);
 
 	return ret;
 }
@@ -91,14 +92,15 @@ EXPORT_SYMBOL_GPL(ezx_pcap_write);
 
 int ezx_pcap_read(struct pcap_chip *pcap, u8 reg_num, u32 *value)
 {
+	unsigned long flags;
 	int ret;
 
-	mutex_lock(&pcap->io_mutex);
+	spin_lock_irqsave(&pcap->io_lock, flags);
 	*value = PCAP_REGISTER_READ_OP_BIT
 		| (reg_num << PCAP_REGISTER_ADDRESS_SHIFT);
 
 	ret = ezx_pcap_putget(pcap, value);
-	mutex_unlock(&pcap->io_mutex);
+	spin_unlock_irqrestore(&pcap->io_lock, flags);
 
 	return ret;
 }
@@ -106,11 +108,12 @@ EXPORT_SYMBOL_GPL(ezx_pcap_read);
 
 int ezx_pcap_set_bits(struct pcap_chip *pcap, u8 reg_num, u32 mask, u32 val)
 {
+	unsigned long flags;
 	int ret;
 	u32 tmp = PCAP_REGISTER_READ_OP_BIT |
 		(reg_num << PCAP_REGISTER_ADDRESS_SHIFT);
 
-	mutex_lock(&pcap->io_mutex);
+	spin_lock_irqsave(&pcap->io_lock, flags);
 	ret = ezx_pcap_putget(pcap, &tmp);
 	if (ret)
 		goto out_unlock;
@@ -121,7 +124,7 @@ int ezx_pcap_set_bits(struct pcap_chip *pcap, u8 reg_num, u32 mask, u32 val)
 
 	ret = ezx_pcap_putget(pcap, &tmp);
 out_unlock:
-	mutex_unlock(&pcap->io_mutex);
+	spin_unlock_irqrestore(&pcap->io_lock, flags);
 
 	return ret;
 }
@@ -212,14 +215,15 @@ static void pcap_irq_handler(struct irq_desc *desc)
 /* ADC */
 void pcap_set_ts_bits(struct pcap_chip *pcap, u32 bits)
 {
+	unsigned long flags;
 	u32 tmp;
 
-	mutex_lock(&pcap->adc_mutex);
+	spin_lock_irqsave(&pcap->adc_lock, flags);
 	ezx_pcap_read(pcap, PCAP_REG_ADC, &tmp);
 	tmp &= ~(PCAP_ADC_TS_M_MASK | PCAP_ADC_TS_REF_LOWPWR);
 	tmp |= bits & (PCAP_ADC_TS_M_MASK | PCAP_ADC_TS_REF_LOWPWR);
 	ezx_pcap_write(pcap, PCAP_REG_ADC, tmp);
-	mutex_unlock(&pcap->adc_mutex);
+	spin_unlock_irqrestore(&pcap->adc_lock, flags);
 }
 EXPORT_SYMBOL_GPL(pcap_set_ts_bits);
 
@@ -234,15 +238,16 @@ static void pcap_disable_adc(struct pcap_chip *pcap)
 
 static void pcap_adc_trigger(struct pcap_chip *pcap)
 {
+	unsigned long flags;
 	u32 tmp;
 	u8 head;
 
-	mutex_lock(&pcap->adc_mutex);
+	spin_lock_irqsave(&pcap->adc_lock, flags);
 	head = pcap->adc_head;
 	if (!pcap->adc_queue[head]) {
 		/* queue is empty, save power */
 		pcap_disable_adc(pcap);
-		mutex_unlock(&pcap->adc_mutex);
+		spin_unlock_irqrestore(&pcap->adc_lock, flags);
 		return;
 	}
 	/* start conversion on requested bank, save TS_M bits */
@@ -254,7 +259,7 @@ static void pcap_adc_trigger(struct pcap_chip *pcap)
 		tmp |= PCAP_ADC_AD_SEL1;
 
 	ezx_pcap_write(pcap, PCAP_REG_ADC, tmp);
-	mutex_unlock(&pcap->adc_mutex);
+	spin_unlock_irqrestore(&pcap->adc_lock, flags);
 	ezx_pcap_write(pcap, PCAP_REG_ADR, PCAP_ADR_ASC);
 }
 
@@ -265,11 +270,11 @@ static irqreturn_t pcap_adc_irq(int irq, void *_pcap)
 	u16 res[2];
 	u32 tmp;
 
-	mutex_lock(&pcap->adc_mutex);
+	spin_lock(&pcap->adc_lock);
 	req = pcap->adc_queue[pcap->adc_head];
 
 	if (WARN(!req, "adc irq without pending request\n")) {
-		mutex_unlock(&pcap->adc_mutex);
+		spin_unlock(&pcap->adc_lock);
 		return IRQ_HANDLED;
 	}
 
@@ -285,7 +290,7 @@ static irqreturn_t pcap_adc_irq(int irq, void *_pcap)
 
 	pcap->adc_queue[pcap->adc_head] = NULL;
 	pcap->adc_head = (pcap->adc_head + 1) & (PCAP_ADC_MAXQ - 1);
-	mutex_unlock(&pcap->adc_mutex);
+	spin_unlock(&pcap->adc_lock);
 
 	/* pass the results and release memory */
 	req->callback(req->data, res);
@@ -301,6 +306,7 @@ int pcap_adc_async(struct pcap_chip *pcap, u8 bank, u32 flags, u8 ch[],
 						void *callback, void *data)
 {
 	struct pcap_adc_request *req;
+	unsigned long irq_flags;
 
 	/* This will be freed after we have a result */
 	req = kmalloc(sizeof(struct pcap_adc_request), GFP_KERNEL);
@@ -314,15 +320,15 @@ int pcap_adc_async(struct pcap_chip *pcap, u8 bank, u32 flags, u8 ch[],
 	req->callback = callback;
 	req->data = data;
 
-	mutex_lock(&pcap->adc_mutex);
+	spin_lock_irqsave(&pcap->adc_lock, irq_flags);
 	if (pcap->adc_queue[pcap->adc_tail]) {
-		mutex_unlock(&pcap->adc_mutex);
+		spin_unlock_irqrestore(&pcap->adc_lock, irq_flags);
 		kfree(req);
 		return -EBUSY;
 	}
 	pcap->adc_queue[pcap->adc_tail] = req;
 	pcap->adc_tail = (pcap->adc_tail + 1) & (PCAP_ADC_MAXQ - 1);
-	mutex_unlock(&pcap->adc_mutex);
+	spin_unlock_irqrestore(&pcap->adc_lock, irq_flags);
 
 	/* start conversion */
 	pcap_adc_trigger(pcap);
@@ -389,16 +395,17 @@ static int pcap_add_subdev(struct pcap_chip *pcap,
 static int ezx_pcap_remove(struct spi_device *spi)
 {
 	struct pcap_chip *pcap = spi_get_drvdata(spi);
+	unsigned long flags;
 	int i;
 
 	/* remove all registered subdevs */
 	device_for_each_child(&spi->dev, NULL, pcap_remove_subdev);
 
 	/* cleanup ADC */
-	mutex_lock(&pcap->adc_mutex);
+	spin_lock_irqsave(&pcap->adc_lock, flags);
 	for (i = 0; i < PCAP_ADC_MAXQ; i++)
 		kfree(pcap->adc_queue[i]);
-	mutex_unlock(&pcap->adc_mutex);
+	spin_unlock_irqrestore(&pcap->adc_lock, flags);
 
 	/* cleanup irqchip */
 	for (i = pcap->irq_base; i < (pcap->irq_base + PCAP_NIRQS); i++)
@@ -426,8 +433,8 @@ static int ezx_pcap_probe(struct spi_device *spi)
 		goto ret;
 	}
 
-	mutex_init(&pcap->io_mutex);
-	mutex_init(&pcap->adc_mutex);
+	spin_lock_init(&pcap->io_lock);
+	spin_lock_init(&pcap->adc_lock);
 	INIT_WORK(&pcap->isr_work, pcap_isr_work);
 	INIT_WORK(&pcap->msr_work, pcap_msr_work);
 	spi_set_drvdata(spi, pcap);
-- 
2.11.0

Re: [PATCH] mfd: ezx-pcap: replace mutex_lock with spin_lock

[Lee Jones]

On Tue, 13 Aug 2019, Fuqian Huang wrote:

> As mutex_lock might sleep.
> Function pcap_adc_irq is an interrupt handler.
> The use of mutex_lock in pcap_adc_irq may cause sleep
> in IRQ context.
> Replace mutex_lock with spin_lock to avoid this.

No one has complained explaining why this might be a bad idea and the
premise is sound enough.

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog