Skip to contentSkip to conversation

rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

PATCH In review Dan Carpenter 3 participants 5 messages Oct 16, 2014 – Oct 17, 2014

Subsystems (3)
Arm Qualcomm mailing list Kernel janitors Real time clock (RTC)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

We recently added a new error path to this function and it is missing an unlock.

Fixes 798187920a6e ('drivers/rtc/rtc-pm8xxx.c: rework to support pm8941 rtc')
Signed-off-by Dan Carpenter <[email protected]>
drivers/rtc/rtc-pm8xxx.c +4 -1
index af88817..dd98895 100644
@@ -86,6 +86,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	unsigned int ctrl_reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	int locked;
 
 	if (!rtc_dd->allow_set_time)
 		return -EACCES;

@@ -100,6 +101,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
 
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	locked = 1;
 
 	rc = regmap_read(rtc_dd->regmap, regs->ctrl, &ctrl_reg);
 	if (rc)

@@ -115,6 +117,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		}
 	} else {
 		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+		locked = 0;
 	}
 
 	/* Write 0 to Byte[0] */

@@ -149,7 +152,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	}
 
 rtc_rw_fail:
-	if (alarm_enabled)
+	if (locked)
 		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 
 	return rc;
#1 lore raw

Hi Dan,

Thanks for catching that.

Quoted 19 lines · Dan Carpenter

We recently added a new error path to this function and it is missing an unlock.

Fixes 798187920a6e ('drivers/rtc/rtc-pm8xxx.c: rework to support pm8941 rtc')
Signed-off-by Dan Carpenter <[email protected]>
drivers/rtc/rtc-pm8xxx.c +1 -0
index af88817..dd98895 100644
@@ -86,6 +86,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	unsigned int ctrl_reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	int locked;
 
 	if (!rtc_dd->allow_set_time)
 		return -EACCES;

drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 104

Quoted 4 lines
drivers/rtc/rtc-pm8xxx.c +1 -0
@@ -100,6 +101,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
 
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	locked = 1;

Sparse complaining in this function too:

warning: context imbalance in 'pm8xxx_rtc_set_time' - different lock contexts for basic block

I'm wondering for a better fix to this. Isn't better to avoid this conditional call to spin_unlock_irqrestore() and lock regmap writes every time without care is the alarm is enabled or not.

Quoted 21 lines
drivers/rtc/rtc-pm8xxx.c +2 -1
 	rc = regmap_read(rtc_dd->regmap, regs->ctrl, &ctrl_reg);
 	if (rc)

@@ -115,6 +117,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		}
 	} else {
 		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+		locked = 0;
 	}
 
 	/* Write 0 to Byte[0] */

@@ -149,7 +152,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	}
 
 rtc_rw_fail:
-	if (alarm_enabled)
+	if (locked)
 		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 
 	return rc;
Signature · 2 lines
regards,
Stan
author
#2 lore raw
In reply to Dan Carpenter
Quoted 10 lines · Stanimir Varbanov

drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 104

Quoted 5 lines
drivers/rtc/rtc-pm8xxx.c +1 -0
@@ -100,6 +101,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
 
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
+	locked = 1;

Sparse complaining in this function too:

warning: context imbalance in 'pm8xxx_rtc_set_time' - different lock contexts for basic block

Yeah. Smatch is a little more clever than Sparse this time. :) Smatch accepted any fix but Sparse complained regardless.

Quoted 5 lines

I'm wondering for a better fix to this. Isn't better to avoid this conditional call to spin_unlock_irqrestore() and lock regmap writes every time without care is the alarm is enabled or not.

I don't know. If you go that road, please give me a Reporte-by tag.

regards,
dan carpenter

#3 lore raw
In reply to Dan Carpenter
Quoted 3 lines · On Thu, 16 Oct 2014 16:57:07 +0300 Stanimir Varbanov <[email protected]>…

I'm wondering for a better fix to this. Isn't better to avoid this conditional call to spin_unlock_irqrestore() and lock regmap writes every time without care is the alarm is enabled or not.

That's what I was thinking. This?

drivers/rtc/rtc-pm8xxx.c renamed from drivers/rtc/rtc-pm8xxx.c~rtc-pm8xxx-rework-to-support-pm8941-rtc-fix +1 -4
@@ -113,8 +113,6 @@ static int pm8xxx_rtc_set_time(struct de
 			dev_err(dev, "Write to RTC control register failed\n");
 			goto rtc_rw_fail;
 		}
-	} else {
-		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 	}
 
 	/* Write 0 to Byte[0] */

@@ -149,8 +147,7 @@ static int pm8xxx_rtc_set_time(struct de
 	}
 
 rtc_rw_fail:
-	if (alarm_enabled)
-		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 
 	return rc;
 }

_

#4 lore raw
In reply to Dan Carpenter
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Thu, 16 Oct 2014 16:57:07 +0300 Stanimir Varbanov <[email protected]>…

I'm wondering for a better fix to this. Isn't better to avoid this conditional call to spin_unlock_irqrestore() and lock regmap writes every time without care is the alarm is enabled or not.

That's what I was thinking. This?

Yes, this should be the right fix.

Quoted 24 lines
drivers/rtc/rtc-pm8xxx.c renamed from drivers/rtc/rtc-pm8xxx.c~rtc-pm8xxx-rework-to-support-pm8941-rtc-fix +1 -4
@@ -113,8 +113,6 @@ static int pm8xxx_rtc_set_time(struct de
 			dev_err(dev, "Write to RTC control register failed\n");
 			goto rtc_rw_fail;
 		}
-	} else {
-		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 	}
 
 	/* Write 0 to Byte[0] */

@@ -149,8 +147,7 @@ static int pm8xxx_rtc_set_time(struct de
 	}
 
 rtc_rw_fail:
-	if (alarm_enabled)
-		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
+	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
 
 	return rc;
 }

_

Signature · 2 lines
regards,
Stan