The PPS_FETCH ioctl is blocking still the reception of a PPS
event. But, in some case, one may immediately need the last event
date. This patch allow to get the result of PPS_FETCH if the device
has the O_NONBLOCK flag set.
Signed-off-by: Paul Chavent <[email protected]>
Acked-by: Rodolfo Giometti <[email protected]>
---
drivers/pps/pps.c | 57 +++++++++++++++++++++++++++++--------------------------
1 file changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2f07cd6..983f50c 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -152,35 +152,38 @@ static long pps_cdev_ioctl(struct file *file,
if (err)
return -EFAULT;
- ev = pps->last_ev;
-
- /* Manage the timeout */
- if (fdata.timeout.flags & PPS_TIME_INVALID)
- err = wait_event_interruptible(pps->queue,
- ev != pps->last_ev);
- else {
- unsigned long ticks;
-
- dev_dbg(pps->dev, "timeout %lld.%09d\n",
- (long long) fdata.timeout.sec,
- fdata.timeout.nsec);
- ticks = fdata.timeout.sec * HZ;
- ticks += fdata.timeout.nsec / (NSEC_PER_SEC / HZ);
-
- if (ticks != 0) {
- err = wait_event_interruptible_timeout(
- pps->queue,
- ev != pps->last_ev,
- ticks);
- if (err == 0)
- return -ETIMEDOUT;
+ if (!(file->f_flags & O_NONBLOCK)) {
+ ev = pps->last_ev;
+
+ /* Manage the timeout */
+ if (fdata.timeout.flags & PPS_TIME_INVALID)
+ err = wait_event_interruptible(pps->queue,
+ ev != pps->last_ev);
+ else {
+ unsigned long ticks;
+
+ dev_dbg(pps->dev, "timeout %lld.%09d\n",
+ (long long) fdata.timeout.sec,
+ fdata.timeout.nsec);
+ ticks = fdata.timeout.sec * HZ;
+ ticks += fdata.timeout.nsec /
+ (NSEC_PER_SEC / HZ);
+
+ if (ticks != 0) {
+ err = wait_event_interruptible_timeout(
+ pps->queue,
+ ev != pps->last_ev,
+ ticks);
+ if (err == 0)
+ return -ETIMEDOUT;
+ }
}
- }
- /* Check for pending signals */
- if (err == -ERESTARTSYS) {
- dev_dbg(pps->dev, "pending signal caught\n");
- return -EINTR;
+ /* Check for pending signals */
+ if (err == -ERESTARTSYS) {
+ dev_dbg(pps->dev, "pending signal caught\n");
+ return -EINTR;
+ }
}
/* Return the fetched timestamp */
--
1.7.12.1
On Fri, 11 Oct 2013 14:40:32 +0200 Paul Chavent <[email protected]> wrote:
> The PPS_FETCH ioctl is blocking still the reception of a PPS
> event. But, in some case, one may immediately need the last event
> date. This patch allow to get the result of PPS_FETCH if the device
> has the O_NONBLOCK flag set.
Are the PPS ioctls actually documented anywhere?
Documentation/pps/pps.txt is silent.
That's a shame, because it would be nice to have a formal description
of the the PPS_FETCH semantics which leads to an understanding of why
things are the way they are, how PPS_FETCH is supposed to be used, etc.
Also, the presence of such documentation would permit me to bug you for
not having updated it! We need *some* channel for telling people about
the driver, and updates to it. Maybe linuxpps.org has it somewhere,
but I couldn't immediately find it.
Your implementation requires that the file be opened non-blocking. But
I'd have thought that adding a new and separate ioctl mode would be a
cleaner and more flexible implementation - that way an app which wants
both blocking and non-blocking behaviour doesn't need to open the file
twice.
Also, this is actually a non-backward-compatible change for any
application which happened to be opening the file with O_NONBLOCK!
Hopefully there aren't any such applications...
On Fri, Oct 11, 2013 at 12:47:20PM -0700, Andrew Morton wrote:
> On Fri, 11 Oct 2013 14:40:32 +0200 Paul Chavent <[email protected]> wrote:
>
> > The PPS_FETCH ioctl is blocking still the reception of a PPS
> > event. But, in some case, one may immediately need the last event
> > date. This patch allow to get the result of PPS_FETCH if the device
> > has the O_NONBLOCK flag set.
>
> Are the PPS ioctls actually documented anywhere?
> Documentation/pps/pps.txt is silent.
>
> That's a shame, because it would be nice to have a formal description
> of the the PPS_FETCH semantics which leads to an understanding of why
> things are the way they are, how PPS_FETCH is supposed to be used, etc.
>
> Also, the presence of such documentation would permit me to bug you for
> not having updated it! We need *some* channel for telling people about
> the driver, and updates to it. Maybe linuxpps.org has it somewhere,
> but I couldn't immediately find it.
Hi Andrew! Actually RFC 2783 doesn't use ioctls to get access to PPS
data but it defines some functions. LinuxPPS, that is the Linux PPS
implementation, uses ioctls to implement these functions.
If you like having an idea about how these functions are implemented
into LinuxPPS, you can see here:
http://www.linuxpps.org/gitweb/?p=pps-tools;a=blob;f=timepps.h;h=d2628d2d061ea2a3623e57990d9ada62623773cf;hb=5980a044bcdb4c1d1a8b1ecff986fa63719519b3
> Your implementation requires that the file be opened non-blocking. But
> I'd have thought that adding a new and separate ioctl mode would be a
> cleaner and more flexible implementation - that way an app which wants
> both blocking and non-blocking behaviour doesn't need to open the file
> twice.
>
> Also, this is actually a non-backward-compatible change for any
> application which happened to be opening the file with O_NONBLOCK!
> Hopefully there aren't any such applications...
The major application that I know using these layer is NTPD... however
all RFC compliant applications should not use ioctls to get access the
PPS data and this patch should be a "special" case.
Thanks for your time,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Tue, 15 Oct 2013 12:43:50 +0200 Rodolfo Giometti <[email protected]> wrote:
> On Fri, Oct 11, 2013 at 12:47:20PM -0700, Andrew Morton wrote:
> > On Fri, 11 Oct 2013 14:40:32 +0200 Paul Chavent <[email protected]> wrote:
> >
> > > The PPS_FETCH ioctl is blocking still the reception of a PPS
> > > event. But, in some case, one may immediately need the last event
> > > date. This patch allow to get the result of PPS_FETCH if the device
> > > has the O_NONBLOCK flag set.
> >
> > Are the PPS ioctls actually documented anywhere?
> > Documentation/pps/pps.txt is silent.
> >
> > That's a shame, because it would be nice to have a formal description
> > of the the PPS_FETCH semantics which leads to an understanding of why
> > things are the way they are, how PPS_FETCH is supposed to be used, etc.
> >
> > Also, the presence of such documentation would permit me to bug you for
> > not having updated it! We need *some* channel for telling people about
> > the driver, and updates to it. Maybe linuxpps.org has it somewhere,
> > but I couldn't immediately find it.
>
> Hi Andrew! Actually RFC 2783 doesn't use ioctls to get access to PPS
> data but it defines some functions. LinuxPPS, that is the Linux PPS
> implementation, uses ioctls to implement these functions.
>
> If you like having an idea about how these functions are implemented
> into LinuxPPS, you can see here:
>
> http://www.linuxpps.org/gitweb/?p=pps-tools;a=blob;f=timepps.h;h=d2628d2d061ea2a3623e57990d9ada62623773cf;hb=5980a044bcdb4c1d1a8b1ecff986fa63719519b3
>
> > Your implementation requires that the file be opened non-blocking. But
> > I'd have thought that adding a new and separate ioctl mode would be a
> > cleaner and more flexible implementation - that way an app which wants
> > both blocking and non-blocking behaviour doesn't need to open the file
> > twice.
> >
> > Also, this is actually a non-backward-compatible change for any
> > application which happened to be opening the file with O_NONBLOCK!
> > Hopefully there aren't any such applications...
>
> The major application that I know using these layer is NTPD... however
> all RFC compliant applications should not use ioctls to get access the
> PPS data and this patch should be a "special" case.
>
Thanks, but this doesn't really address my concerns.
- Where, if anywhere, is the Linux PPS API documented and how do we
communicate changes such as this one to kernel users?
- Why require open(O_NONBLOCK) for this instead of adding a separate
ioctl?
On 10/15/2013 09:55 PM, Andrew Morton wrote:
> On Tue, 15 Oct 2013 12:43:50 +0200 Rodolfo Giometti <[email protected]> wrote:
>
>> On Fri, Oct 11, 2013 at 12:47:20PM -0700, Andrew Morton wrote:
>>> On Fri, 11 Oct 2013 14:40:32 +0200 Paul Chavent <[email protected]> wrote:
>>>
>>>> The PPS_FETCH ioctl is blocking still the reception of a PPS
>>>> event. But, in some case, one may immediately need the last event
>>>> date. This patch allow to get the result of PPS_FETCH if the device
>>>> has the O_NONBLOCK flag set.
>>>
>>> Are the PPS ioctls actually documented anywhere?
>>> Documentation/pps/pps.txt is silent.
>>>
>>> That's a shame, because it would be nice to have a formal description
>>> of the the PPS_FETCH semantics which leads to an understanding of why
>>> things are the way they are, how PPS_FETCH is supposed to be used, etc.
>>>
>>> Also, the presence of such documentation would permit me to bug you for
>>> not having updated it! We need *some* channel for telling people about
>>> the driver, and updates to it. Maybe linuxpps.org has it somewhere,
>>> but I couldn't immediately find it.
>>
>> Hi Andrew! Actually RFC 2783 doesn't use ioctls to get access to PPS
>> data but it defines some functions. LinuxPPS, that is the Linux PPS
>> implementation, uses ioctls to implement these functions.
>>
>> If you like having an idea about how these functions are implemented
>> into LinuxPPS, you can see here:
>>
>> http://www.linuxpps.org/gitweb/?p=pps-tools;a=blob;f=timepps.h;h=d2628d2d061ea2a3623e57990d9ada62623773cf;hb=5980a044bcdb4c1d1a8b1ecff986fa63719519b3
>>
>>> Your implementation requires that the file be opened non-blocking. But
>>> I'd have thought that adding a new and separate ioctl mode would be a
>>> cleaner and more flexible implementation - that way an app which wants
>>> both blocking and non-blocking behaviour doesn't need to open the file
>>> twice.
>>>
>>> Also, this is actually a non-backward-compatible change for any
>>> application which happened to be opening the file with O_NONBLOCK!
>>> Hopefully there aren't any such applications...
>>
>> The major application that I know using these layer is NTPD... however
>> all RFC compliant applications should not use ioctls to get access the
>> PPS data and this patch should be a "special" case.
>>
>
> Thanks, but this doesn't really address my concerns.
>
> - Where, if anywhere, is the Linux PPS API documented and how do we
> communicate changes such as this one to kernel users?
>
> - Why require open(O_NONBLOCK) for this instead of adding a separate
> ioctl?
>
I would also prefer the separate ioctl. As you said it, it's a bit
annoying to switch from blocking mode to non blocking mode if we need
both mode. But i was not sure about the preferences of the maintainer :
(i) change the api, or (ii) change the behavior with a widely supported
interface (O_NONBLOCK).
I'm certainly not the best person to make the final decision, but i
would like to help you if you need me (write doc, or change this patch).
Cheers.
Paul.
On Wed, Oct 16, 2013 at 08:52:47AM +0200, Paul Chavent wrote:
>
> I would also prefer the separate ioctl. As you said it, it's a bit
> annoying to switch from blocking mode to non blocking mode if we
> need both mode. But i was not sure about the preferences of the
> maintainer : (i) change the api, or (ii) change the behavior with a
> widely supported interface (O_NONBLOCK).
As already stated the PPS RFC doesn't use ioctls to manage PPS data so
we can modify ioctls according our needs!
In fact we can modify the LinuxPPS wrapper functions to still remain
RFC compliant. :)
> I'm certainly not the best person to make the final decision, but i
> would like to help you if you need me (write doc, or change this
> patch).
In this scenario I think we can do as Andrew suggests modifying
LinuxPPS docs accordingly... maybe we can add a new file into
linux/Documentation/pps directory describing Linux PPS ioctls and how
they interact with PPS RFC functions.
Andrew, could this be an acceptable solution?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Wed, 16 Oct 2013 09:29:58 +0200 Rodolfo Giometti <[email protected]> wrote:
> On Wed, Oct 16, 2013 at 08:52:47AM +0200, Paul Chavent wrote:
> >
> > I would also prefer the separate ioctl. As you said it, it's a bit
> > annoying to switch from blocking mode to non blocking mode if we
> > need both mode. But i was not sure about the preferences of the
> > maintainer : (i) change the api, or (ii) change the behavior with a
> > widely supported interface (O_NONBLOCK).
>
> As already stated the PPS RFC doesn't use ioctls to manage PPS data so
> we can modify ioctls according our needs!
>
> In fact we can modify the LinuxPPS wrapper functions to still remain
> RFC compliant. :)
Sure. I do think the new ioctl is better than O_NONBLOCK. Are you OK with
that?
> > I'm certainly not the best person to make the final decision, but i
> > would like to help you if you need me (write doc, or change this
> > patch).
>
> In this scenario I think we can do as Andrew suggests modifying
> LinuxPPS docs accordingly... maybe we can add a new file into
> linux/Documentation/pps directory describing Linux PPS ioctls and how
> they interact with PPS RFC functions.
>
> Andrew, could this be an acceptable solution?
Sounds great, thanks.