[PATCH net] ptp: ocp: have adjtime handle negative delta_ns correctly

[PATCH net] ptp: ocp: have adjtime handle negative delta_ns correctly

[Jonathan Lemon]

delta_ns is a s64, but it was being passed ptp_ocp_adjtime_coarse
as an u64.  Also, it turns out that timespec64_add_ns() only handles
positive values, so the math needs to be updated.

Fix by passing in the correct signed value, then adding to a
nanosecond version of the timespec.

Fixes: '90f8f4c0e3ce ("ptp: ocp: Add ptp_ocp_adjtime_coarse for large adjustments")'
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index dd45471f6780..65e592ec272e 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -841,16 +841,18 @@ __ptp_ocp_adjtime_locked(struct ptp_ocp *bp, u32 adj_val)
 }
 
 static void
-ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, u64 delta_ns)
+ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, s64 delta_ns)
 {
 	struct timespec64 ts;
 	unsigned long flags;
 	int err;
+	s64 ns;
 
 	spin_lock_irqsave(&bp->lock, flags);
 	err = __ptp_ocp_gettime_locked(bp, &ts, NULL);
 	if (likely(!err)) {
-		timespec64_add_ns(&ts, delta_ns);
+		ns = timespec64_to_ns(&ts) + delta_ns;
+		ts = ns_to_timespec64(ns);
 		__ptp_ocp_settime_locked(bp, &ts);
 	}
 	spin_unlock_irqrestore(&bp->lock, flags);
-- 
2.31.1

Re: [PATCH net] ptp: ocp: have adjtime handle negative delta_ns correctly

[Vadim Fedorenko]

On 06.05.2022 00:40, Jonathan Lemon wrote:
> delta_ns is a s64, but it was being passed ptp_ocp_adjtime_coarse
> as an u64.  Also, it turns out that timespec64_add_ns() only handles
> positive values, so the math needs to be updated.
> 
> Fix by passing in the correct signed value, then adding to a
> nanosecond version of the timespec.
> 
> Fixes: '90f8f4c0e3ce ("ptp: ocp: Add ptp_ocp_adjtime_coarse for large adjustments")'
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
>   drivers/ptp/ptp_ocp.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index dd45471f6780..65e592ec272e 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -841,16 +841,18 @@ __ptp_ocp_adjtime_locked(struct ptp_ocp *bp, u32 adj_val)
>   }
>   
>   static void
> -ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, u64 delta_ns)
> +ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, s64 delta_ns)
>   {
>   	struct timespec64 ts;
>   	unsigned long flags;
>   	int err;
> +	s64 ns;
>   
>   	spin_lock_irqsave(&bp->lock, flags);
>   	err = __ptp_ocp_gettime_locked(bp, &ts, NULL);
>   	if (likely(!err)) {
> -		timespec64_add_ns(&ts, delta_ns);
> +		ns = timespec64_to_ns(&ts) + delta_ns;
> +		ts = ns_to_timespec64(ns);

Maybe use set_normalized_timespec64 instead of this ugly transformations and 
additional variable?

>   		__ptp_ocp_settime_locked(bp, &ts);
>   	}
>   	spin_unlock_irqrestore(&bp->lock, flags);

Re: [PATCH net] ptp: ocp: have adjtime handle negative delta_ns correctly

[Jonathan Lemon]

On Sat, May 07, 2022 at 01:19:54AM +0100, Vadim Fedorenko wrote:
> On 06.05.2022 00:40, Jonathan Lemon wrote:
> > delta_ns is a s64, but it was being passed ptp_ocp_adjtime_coarse
> > as an u64.  Also, it turns out that timespec64_add_ns() only handles
> > positive values, so the math needs to be updated.
> > 
> > Fix by passing in the correct signed value, then adding to a
> > nanosecond version of the timespec.
> > 
> > Fixes: '90f8f4c0e3ce ("ptp: ocp: Add ptp_ocp_adjtime_coarse for large adjustments")'
> > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> > ---
> >   drivers/ptp/ptp_ocp.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> > index dd45471f6780..65e592ec272e 100644
> > --- a/drivers/ptp/ptp_ocp.c
> > +++ b/drivers/ptp/ptp_ocp.c
> > @@ -841,16 +841,18 @@ __ptp_ocp_adjtime_locked(struct ptp_ocp *bp, u32 adj_val)
> >   }
> >   static void
> > -ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, u64 delta_ns)
> > +ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, s64 delta_ns)
> >   {
> >   	struct timespec64 ts;
> >   	unsigned long flags;
> >   	int err;
> > +	s64 ns;
> >   	spin_lock_irqsave(&bp->lock, flags);
> >   	err = __ptp_ocp_gettime_locked(bp, &ts, NULL);
> >   	if (likely(!err)) {
> > -		timespec64_add_ns(&ts, delta_ns);
> > +		ns = timespec64_to_ns(&ts) + delta_ns;
> > +		ts = ns_to_timespec64(ns);
> 
> Maybe use set_normalized_timespec64 instead of this ugly transformations and
> additional variable?

I don't see how that would work - set_normalized_timespec64 just sets
the ts from a <sec>.<nsec> value.  In this case, delta_ns need to be
added in to the ts value, but timespec64_add_ns() doeesn't handle
negative values, hence the conversion / add / reconversion.
-- 
Jonathan

Re: [PATCH net] ptp: ocp: have adjtime handle negative delta_ns correctly

[Vadim Fedorenko]

On 08.05.2022 05:55, Jonathan Lemon wrote:
> On Sat, May 07, 2022 at 01:19:54AM +0100, Vadim Fedorenko wrote:
>> On 06.05.2022 00:40, Jonathan Lemon wrote:
>>> delta_ns is a s64, but it was being passed ptp_ocp_adjtime_coarse
>>> as an u64.  Also, it turns out that timespec64_add_ns() only handles
>>> positive values, so the math needs to be updated.
>>>
>>> Fix by passing in the correct signed value, then adding to a
>>> nanosecond version of the timespec.
>>>
>>> Fixes: '90f8f4c0e3ce ("ptp: ocp: Add ptp_ocp_adjtime_coarse for large adjustments")'
>>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>>> ---
>>>    drivers/ptp/ptp_ocp.c | 6 ++++--
>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>>> index dd45471f6780..65e592ec272e 100644
>>> --- a/drivers/ptp/ptp_ocp.c
>>> +++ b/drivers/ptp/ptp_ocp.c
>>> @@ -841,16 +841,18 @@ __ptp_ocp_adjtime_locked(struct ptp_ocp *bp, u32 adj_val)
>>>    }
>>>    static void
>>> -ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, u64 delta_ns)
>>> +ptp_ocp_adjtime_coarse(struct ptp_ocp *bp, s64 delta_ns)
>>>    {
>>>    	struct timespec64 ts;
>>>    	unsigned long flags;
>>>    	int err;
>>> +	s64 ns;
>>>    	spin_lock_irqsave(&bp->lock, flags);
>>>    	err = __ptp_ocp_gettime_locked(bp, &ts, NULL);
>>>    	if (likely(!err)) {
>>> -		timespec64_add_ns(&ts, delta_ns);
>>> +		ns = timespec64_to_ns(&ts) + delta_ns;
>>> +		ts = ns_to_timespec64(ns);
>>
>> Maybe use set_normalized_timespec64 instead of this ugly transformations and
>> additional variable?
> 
> I don't see how that would work - set_normalized_timespec64 just sets
> the ts from a <sec>.<nsec> value.  In this case, delta_ns need to be
> added in to the ts value, but timespec64_add_ns() doeesn't handle
> negative values, hence the conversion / add / reconversion.

Could be like:

-	timespec64_add_ns(&ts, delta_ns);
+	set_normalized_timespec64(&ts, ts.tv_sec, ts.tv_nsec + delta_ns);

That's actually without multiplication and division caused by two conversions.