[patch -mm] rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

[patch -mm] rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

[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 <dan.carpenter@oracle.com>

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index af88817..dd98895 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -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;

Re: [patch -mm] rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

[Stanimir Varbanov]

Hi Dan,

Thanks for catching that.

On 10/16/2014 10:55 AM, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index af88817..dd98895 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -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;

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.

>  
>  	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;
> 


-- 
regards,
Stan

Re: [patch -mm] rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

[Dan Carpenter]

On Thu, Oct 16, 2014 at 04:57:07PM +0300, Stanimir Varbanov wrote:
> > @@ -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.

> 
> 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

Re: [patch -mm] rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

[Andrew Morton]

On Thu, 16 Oct 2014 16:57:07 +0300 Stanimir Varbanov <svarbanov@mm-sol.com> wrote:

> 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?

--- a/drivers/rtc/rtc-pm8xxx.c~rtc-pm8xxx-rework-to-support-pm8941-rtc-fix
+++ a/drivers/rtc/rtc-pm8xxx.c
@@ -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;
 }
_

Re: [patch -mm] rtc: pm8xxx: unlock on error in pm8xxx_rtc_set_time()

[Stanimir Varbanov]

On 10/16/2014 11:01 PM, Andrew Morton wrote:
> On Thu, 16 Oct 2014 16:57:07 +0300 Stanimir Varbanov <svarbanov@mm-sol.com> wrote:
> 
>> 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.	

> 
> --- a/drivers/rtc/rtc-pm8xxx.c~rtc-pm8xxx-rework-to-support-pm8941-rtc-fix
> +++ a/drivers/rtc/rtc-pm8xxx.c
> @@ -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;
>  }
> _
> 


-- 
regards,
Stan