ds1685_rtc_begin_ctrl_access/ds1685_rtc_end_ctrl_access aren't used,
so get rid of it.
Signed-off-by: Thomas Bogendoerfer <[email protected]>
---
drivers/rtc/rtc-ds1685.c | 36 ------------------------------------
1 file changed, 36 deletions(-)
diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 2f5194df239e..33781be58f16 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -192,42 +192,6 @@ ds1685_rtc_end_data_access(struct ds1685_priv *rtc)
}
/**
- * ds1685_rtc_begin_ctrl_access - prepare the rtc for ctrl access.
- * @rtc: pointer to the ds1685 rtc structure.
- * @flags: irq flags variable for spin_lock_irqsave.
- *
- * This takes several steps to prepare the rtc for access to read just the
- * control registers:
- * - Sets a spinlock on the rtc IRQ.
- * - Switches the rtc to bank 1. This allows access to the two extended
- * control registers.
- *
- * Only use this where you are certain another lock will not be held.
- */
-static inline void
-ds1685_rtc_begin_ctrl_access(struct ds1685_priv *rtc, unsigned long *flags)
-{
- spin_lock_irqsave(&rtc->lock, *flags);
- ds1685_rtc_switch_to_bank1(rtc);
-}
-
-/**
- * ds1685_rtc_end_ctrl_access - end ctrl access on the rtc.
- * @rtc: pointer to the ds1685 rtc structure.
- * @flags: irq flags variable for spin_unlock_irqrestore.
- *
- * This ends what was started by ds1685_rtc_begin_ctrl_access:
- * - Switches the rtc back to bank 0.
- * - Unsets the spinlock on the rtc IRQ.
- */
-static inline void
-ds1685_rtc_end_ctrl_access(struct ds1685_priv *rtc, unsigned long flags)
-{
- ds1685_rtc_switch_to_bank0(rtc);
- spin_unlock_irqrestore(&rtc->lock, flags);
-}
-
-/**
* ds1685_rtc_get_ssn - retrieve the silicon serial number.
* @rtc: pointer to the ds1685 rtc structure.
* @ssn: u8 array to hold the bits of the silicon serial number.
--
2.13.7
Handling of extended interrupts (kickstart, wake-up, ram-clear) was
moved off to a work queue, but the interrupts aren't acknowledged
in the interrupt handler. This leads to a deadlock, if driver
is used with interrupts. To fix this we use a threaded interrupt, get rid
of the work queue and do locking with just the rtc mutex lock.
Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
Signed-off-by: Thomas Bogendoerfer <[email protected]>
---
drivers/rtc/rtc-ds1685.c | 220 +++++++++++++++++++++------------------------
include/linux/rtc/ds1685.h | 2 -
2 files changed, 104 insertions(+), 118 deletions(-)
diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 33781be58f16..5f4328524183 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -510,10 +510,6 @@ static int
ds1685_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
struct ds1685_priv *rtc = dev_get_drvdata(dev);
- unsigned long flags = 0;
-
- /* Enable/disable the Alarm IRQ-Enable flag. */
- spin_lock_irqsave(&rtc->lock, flags);
/* Flip the requisite interrupt-enable bit. */
if (enabled)
@@ -525,7 +521,6 @@ ds1685_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
/* Read Control C to clear all the flag bits. */
rtc->read(rtc, RTC_CTRL_C);
- spin_unlock_irqrestore(&rtc->lock, flags);
return 0;
}
@@ -533,98 +528,18 @@ ds1685_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
/* ----------------------------------------------------------------------- */
-/* IRQ handler & workqueue. */
-
-/**
- * ds1685_rtc_irq_handler - IRQ handler.
- * @irq: IRQ number.
- * @dev_id: platform device pointer.
- */
-static irqreturn_t
-ds1685_rtc_irq_handler(int irq, void *dev_id)
-{
- struct platform_device *pdev = dev_id;
- struct ds1685_priv *rtc = platform_get_drvdata(pdev);
- u8 ctrlb, ctrlc;
- unsigned long events = 0;
- u8 num_irqs = 0;
-
- /* Abort early if the device isn't ready yet (i.e., DEBUG_SHIRQ). */
- if (unlikely(!rtc))
- return IRQ_HANDLED;
-
- /* Ctrlb holds the interrupt-enable bits and ctrlc the flag bits. */
- spin_lock(&rtc->lock);
- ctrlb = rtc->read(rtc, RTC_CTRL_B);
- ctrlc = rtc->read(rtc, RTC_CTRL_C);
-
- /* Is the IRQF bit set? */
- if (likely(ctrlc & RTC_CTRL_C_IRQF)) {
- /*
- * We need to determine if it was one of the standard
- * events: PF, AF, or UF. If so, we handle them and
- * update the RTC core.
- */
- if (likely(ctrlc & RTC_CTRL_B_PAU_MASK)) {
- events = RTC_IRQF;
-
- /* Check for a periodic interrupt. */
- if ((ctrlb & RTC_CTRL_B_PIE) &&
- (ctrlc & RTC_CTRL_C_PF)) {
- events |= RTC_PF;
- num_irqs++;
- }
-
- /* Check for an alarm interrupt. */
- if ((ctrlb & RTC_CTRL_B_AIE) &&
- (ctrlc & RTC_CTRL_C_AF)) {
- events |= RTC_AF;
- num_irqs++;
- }
-
- /* Check for an update interrupt. */
- if ((ctrlb & RTC_CTRL_B_UIE) &&
- (ctrlc & RTC_CTRL_C_UF)) {
- events |= RTC_UF;
- num_irqs++;
- }
-
- rtc_update_irq(rtc->dev, num_irqs, events);
- } else {
- /*
- * One of the "extended" interrupts was received that
- * is not recognized by the RTC core. These need to
- * be handled in task context as they can call other
- * functions and the time spent in irq context needs
- * to be minimized. Schedule them into a workqueue
- * and inform the RTC core that the IRQs were handled.
- */
- spin_unlock(&rtc->lock);
- schedule_work(&rtc->work);
- rtc_update_irq(rtc->dev, 0, 0);
- return IRQ_HANDLED;
- }
- }
- spin_unlock(&rtc->lock);
-
- return events ? IRQ_HANDLED : IRQ_NONE;
-}
+/* IRQ handler */
/**
- * ds1685_rtc_work_queue - work queue handler.
- * @work: work_struct containing data to work on in task context.
+ * ds1685_rtc_extended_irq - take care of extended interrupts
+ * @rtc: pointer to the ds1685 rtc structure.
+ * @pdev: platform device pointer.
*/
static void
-ds1685_rtc_work_queue(struct work_struct *work)
+ds1685_rtc_extended_irq(struct ds1685_priv *rtc, struct platform_device *pdev)
{
- struct ds1685_priv *rtc = container_of(work,
- struct ds1685_priv, work);
- struct platform_device *pdev = to_platform_device(&rtc->dev->dev);
- struct mutex *rtc_mutex = &rtc->dev->ops_lock;
u8 ctrl4a, ctrl4b;
- mutex_lock(rtc_mutex);
-
ds1685_rtc_switch_to_bank1(rtc);
ctrl4a = rtc->read(rtc, RTC_EXT_CTRL_4A);
ctrl4b = rtc->read(rtc, RTC_EXT_CTRL_4B);
@@ -703,8 +618,76 @@ ds1685_rtc_work_queue(struct work_struct *work)
"RAM-Clear IRQ just occurred!\n");
}
ds1685_rtc_switch_to_bank0(rtc);
+}
+
+/**
+ * ds1685_rtc_irq_handler - IRQ handler.
+ * @irq: IRQ number.
+ * @dev_id: platform device pointer.
+ */
+static irqreturn_t
+ds1685_rtc_irq_handler(int irq, void *dev_id)
+{
+ struct platform_device *pdev = dev_id;
+ struct ds1685_priv *rtc = platform_get_drvdata(pdev);
+ struct mutex *rtc_mutex;
+ u8 ctrlb, ctrlc;
+ unsigned long events = 0;
+ u8 num_irqs = 0;
+
+ /* Abort early if the device isn't ready yet (i.e., DEBUG_SHIRQ). */
+ if (unlikely(!rtc))
+ return IRQ_HANDLED;
+
+ rtc_mutex = &rtc->dev->ops_lock;
+ mutex_lock(rtc_mutex);
+
+ /* Ctrlb holds the interrupt-enable bits and ctrlc the flag bits. */
+ ctrlb = rtc->read(rtc, RTC_CTRL_B);
+ ctrlc = rtc->read(rtc, RTC_CTRL_C);
+
+ /* Is the IRQF bit set? */
+ if (likely(ctrlc & RTC_CTRL_C_IRQF)) {
+ /*
+ * We need to determine if it was one of the standard
+ * events: PF, AF, or UF. If so, we handle them and
+ * update the RTC core.
+ */
+ if (likely(ctrlc & RTC_CTRL_B_PAU_MASK)) {
+ events = RTC_IRQF;
+
+ /* Check for a periodic interrupt. */
+ if ((ctrlb & RTC_CTRL_B_PIE) &&
+ (ctrlc & RTC_CTRL_C_PF)) {
+ events |= RTC_PF;
+ num_irqs++;
+ }
+
+ /* Check for an alarm interrupt. */
+ if ((ctrlb & RTC_CTRL_B_AIE) &&
+ (ctrlc & RTC_CTRL_C_AF)) {
+ events |= RTC_AF;
+ num_irqs++;
+ }
+ /* Check for an update interrupt. */
+ if ((ctrlb & RTC_CTRL_B_UIE) &&
+ (ctrlc & RTC_CTRL_C_UF)) {
+ events |= RTC_UF;
+ num_irqs++;
+ }
+ } else {
+ /*
+ * One of the "extended" interrupts was received that
+ * is not recognized by the RTC core.
+ */
+ ds1685_rtc_extended_irq(rtc, pdev);
+ }
+ }
+ rtc_update_irq(rtc->dev, num_irqs, events);
mutex_unlock(rtc_mutex);
+
+ return events ? IRQ_HANDLED : IRQ_NONE;
}
/* ----------------------------------------------------------------------- */
@@ -833,11 +816,15 @@ static int ds1685_nvram_read(void *priv, unsigned int pos, void *val,
size_t size)
{
struct ds1685_priv *rtc = priv;
+ struct mutex *rtc_mutex = &rtc->dev->ops_lock;
ssize_t count;
- unsigned long flags = 0;
u8 *buf = val;
+ int err;
+
+ err = mutex_lock_interruptible(rtc_mutex);
+ if (err)
+ return err;
- spin_lock_irqsave(&rtc->lock, flags);
ds1685_rtc_switch_to_bank0(rtc);
/* Read NVRAM in time and bank0 registers. */
@@ -887,7 +874,7 @@ static int ds1685_nvram_read(void *priv, unsigned int pos, void *val,
ds1685_rtc_switch_to_bank0(rtc);
}
#endif /* !CONFIG_RTC_DRV_DS1689 */
- spin_unlock_irqrestore(&rtc->lock, flags);
+ mutex_unlock(rtc_mutex);
return 0;
}
@@ -896,11 +883,15 @@ static int ds1685_nvram_write(void *priv, unsigned int pos, void *val,
size_t size)
{
struct ds1685_priv *rtc = priv;
+ struct mutex *rtc_mutex = &rtc->dev->ops_lock;
ssize_t count;
- unsigned long flags = 0;
u8 *buf = val;
+ int err;
+
+ err = mutex_lock_interruptible(rtc_mutex);
+ if (err)
+ return err;
- spin_lock_irqsave(&rtc->lock, flags);
ds1685_rtc_switch_to_bank0(rtc);
/* Write NVRAM in time and bank0 registers. */
@@ -950,7 +941,7 @@ static int ds1685_nvram_write(void *priv, unsigned int pos, void *val,
ds1685_rtc_switch_to_bank0(rtc);
}
#endif /* !CONFIG_RTC_DRV_DS1689 */
- spin_unlock_irqrestore(&rtc->lock, flags);
+ mutex_unlock(rtc_mutex);
return 0;
}
@@ -1141,9 +1132,7 @@ ds1685_rtc_probe(struct platform_device *pdev)
if (pdata->plat_post_ram_clear)
rtc->post_ram_clear = pdata->plat_post_ram_clear;
- /* Init the spinlock, workqueue, & set the driver data. */
- spin_lock_init(&rtc->lock);
- INIT_WORK(&rtc->work, ds1685_rtc_work_queue);
+ /* set the driver data. */
platform_set_drvdata(pdev, rtc);
/* Turn the oscillator on if is not already on (DV1 = 1). */
@@ -1299,22 +1288,23 @@ ds1685_rtc_probe(struct platform_device *pdev)
*/
if (!pdata->no_irq) {
ret = platform_get_irq(pdev, 0);
- if (ret > 0) {
- rtc->irq_num = ret;
-
- /* Request an IRQ. */
- ret = devm_request_irq(&pdev->dev, rtc->irq_num,
- ds1685_rtc_irq_handler,
- IRQF_SHARED, pdev->name, pdev);
-
- /* Check to see if something came back. */
- if (unlikely(ret)) {
- dev_warn(&pdev->dev,
- "RTC interrupt not available\n");
- rtc->irq_num = 0;
- }
- } else
+ if (ret <= 0)
return ret;
+
+ rtc->irq_num = ret;
+
+ /* Request an IRQ. */
+ ret = devm_request_threaded_irq(&pdev->dev, rtc->irq_num,
+ NULL, ds1685_rtc_irq_handler,
+ IRQF_SHARED | IRQF_ONESHOT,
+ pdev->name, pdev);
+
+ /* Check to see if something came back. */
+ if (unlikely(ret)) {
+ dev_warn(&pdev->dev,
+ "RTC interrupt not available\n");
+ rtc->irq_num = 0;
+ }
}
rtc->no_irq = pdata->no_irq;
@@ -1361,8 +1351,6 @@ ds1685_rtc_remove(struct platform_device *pdev)
(rtc->read(rtc, RTC_EXT_CTRL_4A) &
~(RTC_CTRL_4A_RWK_MASK)));
- cancel_work_sync(&rtc->work);
-
return 0;
}
diff --git a/include/linux/rtc/ds1685.h b/include/linux/rtc/ds1685.h
index e6337a56d741..a00b332c505f 100644
--- a/include/linux/rtc/ds1685.h
+++ b/include/linux/rtc/ds1685.h
@@ -48,8 +48,6 @@ struct ds1685_priv {
u32 regstep;
resource_size_t baseaddr;
size_t size;
- spinlock_t lock;
- struct work_struct work;
int irq_num;
bool bcd_mode;
bool no_irq;
--
2.13.7
On 16/04/2019 11:34:04+0200, Thomas Bogendoerfer wrote:
> Handling of extended interrupts (kickstart, wake-up, ram-clear) was
> moved off to a work queue, but the interrupts aren't acknowledged
> in the interrupt handler. This leads to a deadlock, if driver
> is used with interrupts. To fix this we use a threaded interrupt, get rid
> of the work queue and do locking with just the rtc mutex lock.
>
> Fixes: aaaf5fbf56f1 ("rtc: add driver for DS1685 family of real time clocks")
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
> ---
> drivers/rtc/rtc-ds1685.c | 220 +++++++++++++++++++++------------------------
> include/linux/rtc/ds1685.h | 2 -
> 2 files changed, 104 insertions(+), 118 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 16/04/2019 11:34:03+0200, Thomas Bogendoerfer wrote:
> ds1685_rtc_begin_ctrl_access/ds1685_rtc_end_ctrl_access aren't used,
> so get rid of it.
>
> Signed-off-by: Thomas Bogendoerfer <[email protected]>
> ---
> drivers/rtc/rtc-ds1685.c | 36 ------------------------------------
> 1 file changed, 36 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com