Hello,
here my last patch for PPS support.
Please, let me know if I have something to do for kernel inclusion.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Tue, Oct 23, 2007 at 08:04:18PM +0200, Rodolfo Giometti wrote:
Hi Rodolfo,
> here my last patch for PPS support.
>
> Please, let me know if I have something to do for kernel inclusion.
Thanks for trying to get this merged, we've had a few users asking
us to support this in the Fedora kernel.
>From a quick look through, it doesn't look too bad.
A couple of obvious things that jumped out..
* The example program in the Documentation dir.
There was some discussion recently about moving away
from doing this, and instead moving code to the samples/ dir
(or including it in util-linux instead if applicable).
> + static struct pps_source_info_s pps_ktimer_info = {
> + name : "ktimer",
> + path : "",
> + mode : PPS_CAPTUREASSERT | PPS_OFFSETASSERT | \
> + PPS_ECHOASSERT | \
> + PPS_CANWAIT | PPS_TSFMT_TSPEC,
> + echo : pps_ktimer_echo,
> + owner : THIS_MODULE,
> + };
should be
.name = "ktimer"
etc..
> +#include <linux/version.h>
This should be getting included automatically for you.
On the whole, asides from these (very) minor nits, quite a clean
submission compared to a lot of new driver submissions to lkml.
Might want to run it through scripts/checkpatch.pl too to see
if that picks up any more minor fluff.
Dave
--
http://www.codemonkey.org.uk
On Tue, Oct 23, 2007 at 04:17:50PM -0400, Dave Jones wrote:
> On Tue, Oct 23, 2007 at 08:04:18PM +0200, Rodolfo Giometti wrote:
>
> Hi Rodolfo,
Hi! :)
> > here my last patch for PPS support.
> >
> > Please, let me know if I have something to do for kernel inclusion.
>
> Thanks for trying to get this merged, we've had a few users asking
> us to support this in the Fedora kernel.
This sounds good! ;)
> >From a quick look through, it doesn't look too bad.
> A couple of obvious things that jumped out..
>
> * The example program in the Documentation dir.
> There was some discussion recently about moving away
> from doing this, and instead moving code to the samples/ dir
> (or including it in util-linux instead if applicable).
Mmm, I just consider them as simple userland examples. They don't need
to manage PPS devices.
I can remove them from Documentation/pps/, no problems, but in my
modest opinion they are just documentation. :)
> > + static struct pps_source_info_s pps_ktimer_info = {
> > + name : "ktimer",
> > + path : "",
> > + mode : PPS_CAPTUREASSERT | PPS_OFFSETASSERT | \
> > + PPS_ECHOASSERT | \
> > + PPS_CANWAIT | PPS_TSFMT_TSPEC,
> > + echo : pps_ktimer_echo,
> > + owner : THIS_MODULE,
> > + };
>
> should be
> .name = "ktimer"
> etc..
Fixed.
> > +#include <linux/version.h>
>
> This should be getting included automatically for you.
Fixed.
> On the whole, asides from these (very) minor nits, quite a clean
> submission compared to a lot of new driver submissions to lkml.
> Might want to run it through scripts/checkpatch.pl too to see
> if that picks up any more minor fluff.
Ok!
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Tue, Oct 23, 2007 at 04:17:50PM -0400, Dave Jones wrote:
>
> On the whole, asides from these (very) minor nits, quite a clean
> submission compared to a lot of new driver submissions to lkml.
> Might want to run it through scripts/checkpatch.pl too to see
> if that picks up any more minor fluff.
Here the ./scripts/checkpatch.pl output:
$ ./scripts/checkpatch.pl patch
WARNING: do not add new typedefs
#522: FILE: Documentation/pps/timepps.h:59:
+typedef int pps_handle_t; /* represents a PPS source */
WARNING: do not add new typedefs
#523: FILE: Documentation/pps/timepps.h:60:
+typedef unsigned long pps_seq_t; /* sequence number */
WARNING: do not add new typedefs
#524: FILE: Documentation/pps/timepps.h:61:
+typedef struct ntp_fp ntp_fp_t; /* NTP-compatible time stamp */
WARNING: do not add new typedefs
#525: FILE: Documentation/pps/timepps.h:62:
+typedef union pps_timeu pps_timeu_t; /* generic data type for time stamps */
WARNING: do not add new typedefs
#526: FILE: Documentation/pps/timepps.h:63:
+typedef struct pps_info pps_info_t;
WARNING: do not add new typedefs
#527: FILE: Documentation/pps/timepps.h:64:
+typedef struct pps_params pps_params_t;
These refere to an userland include file.
WARNING: line over 80 characters
#1882: FILE: drivers/serial/serial_core.c:871:
+ if (((old_flags ^ port->flags) & (UPF_SPD_MASK|UPF_HARDPPS_CD)) ||
ERROR: need consistent spacing around '|' (ctx:WxV)
#2191: FILE: include/linux/serial_core.h:288:
+ |UPF_HARDPPS_CD))
^
These refere to previous code.
ERROR: Missing Signed-off-by: line(s)
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Attached the new patch.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
A few comments:
> + dev_err(port->dev, "PPS support disabled due port \"%s\" is "
> + "in polling mode\n",
I think "because" instead of "due" is closer to standard English.
> + printk(KERN_ERR "pps: %s: too much PPS sources in the system\n",
> + info->name);
Similarly should be "many" instead of "much".
> + /* Get new ID for the new PPS source */
> + if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
> + err = -ENOMEM;
> + goto kfree_pps;
> + }
> +
> + spin_lock_irq(&idr_lock);
> + err = idr_get_new(&pps_idr, pps, &id);
> + spin_unlock_irq(&idr_lock);
> +
> + if (err < 0)
> + goto kfree_pps;
You usually can handle idr_get_new() returning -EAGAIN by jumping back
to the idr_pre_get(), to handle someone else coming in and stealing
the memory you just preallocated. In this case it may not matter
since it's pretty unlikely that a lot of contexts are using the idr at
the same time. But anyway...
> +void pps_unregister_source(int source)
> ...
> + wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
> +
> + pps_sysfs_remove_source_entry(pps);
> + pps_unregister_cdev(pps);
> + kfree(pps);
This reference counting looks dubious to me... later on in the code
you have:
> +static int pps_cdev_open(struct inode *inode, struct file *file)
> +{
> + struct pps_device *pps = container_of(inode->i_cdev,
> + struct pps_device, cdev);
> +
> + /* Lock the PPS source against (possible) deregistration */
> + atomic_inc(&pps->usage);
with no locking, so I see no reason why the atomic_inc() couldn't
happen right after the wait_event() sees a count of 0 and lets the
deregistration continue. Which would lead to use-after-free.
- R.
On Tue, Oct 23, 2007 at 02:08:19PM -0700, Roland Dreier wrote:
> A few comments:
>
> > + dev_err(port->dev, "PPS support disabled due port \"%s\" is "
> > + "in polling mode\n",
>
> I think "because" instead of "due" is closer to standard English.
Fixed.
> > + printk(KERN_ERR "pps: %s: too much PPS sources in the system\n",
> > + info->name);
>
> Similarly should be "many" instead of "much".
Fixed.
> > + /* Get new ID for the new PPS source */
> > + if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
> > + err = -ENOMEM;
> > + goto kfree_pps;
> > + }
> > +
> > + spin_lock_irq(&idr_lock);
> > + err = idr_get_new(&pps_idr, pps, &id);
> > + spin_unlock_irq(&idr_lock);
> > +
> > + if (err < 0)
> > + goto kfree_pps;
>
> You usually can handle idr_get_new() returning -EAGAIN by jumping back
> to the idr_pre_get(), to handle someone else coming in and stealing
> the memory you just preallocated. In this case it may not matter
> since it's pretty unlikely that a lot of contexts are using the idr at
> the same time. But anyway...
I don't understand what you mean. Can you please submit an example
code?
> > +void pps_unregister_source(int source)
> > ...
> > + wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
> > +
> > + pps_sysfs_remove_source_entry(pps);
> > + pps_unregister_cdev(pps);
> > + kfree(pps);
>
> This reference counting looks dubious to me... later on in the code
> you have:
>
> > +static int pps_cdev_open(struct inode *inode, struct file *file)
> > +{
> > + struct pps_device *pps = container_of(inode->i_cdev,
> > + struct pps_device, cdev);
> > +
> > + /* Lock the PPS source against (possible) deregistration */
> > + atomic_inc(&pps->usage);
>
> with no locking, so I see no reason why the atomic_inc() couldn't
> happen right after the wait_event() sees a count of 0 and lets the
> deregistration continue. Which would lead to use-after-free.
Mmm... you are right... can you please suggest to me how can I easily
fix this problem?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Tue, Oct 23, 2007 at 02:08:19PM -0700, Roland Dreier wrote:
> > +void pps_unregister_source(int source)
> > ...
> > + wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
> > +
> > + pps_sysfs_remove_source_entry(pps);
> > + pps_unregister_cdev(pps);
> > + kfree(pps);
>
> This reference counting looks dubious to me... later on in the code
> you have:
>
> > +static int pps_cdev_open(struct inode *inode, struct file *file)
> > +{
> > + struct pps_device *pps = container_of(inode->i_cdev,
> > + struct pps_device, cdev);
> > +
> > + /* Lock the PPS source against (possible) deregistration */
> > + atomic_inc(&pps->usage);
>
> with no locking, so I see no reason why the atomic_inc() couldn't
> happen right after the wait_event() sees a count of 0 and lets the
> deregistration continue. Which would lead to use-after-free.
Since pps_unregister_source() is defined as:
void pps_unregister_source(int source)
{
struct pps_device *pps;
spin_lock_irq(&idr_lock);
pps = idr_find(&pps_idr, source);
if (!pps) {
spin_unlock_irq(&idr_lock);
return;
}
/* This should be done first in order to deny IRQ handlers
* to access PPS structs
*/
idr_remove(&pps_idr, pps->id);
spin_unlock_irq(&idr_lock);
wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
pps_sysfs_remove_source_entry(pps);
pps_unregister_cdev(pps);
kfree(pps);
}
changing pps_cdev_open() as follow should resolve this problem:
static int pps_cdev_open(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode->i_cdev,
struct pps_device, cdev);
int found;
spin_lock_irq(&idr_lock);
found = idr_find(&pps_idr, pps->id) != NULL;
/* Lock the PPS source against (possible) deregistration */
if (found)
atomic_inc(&pps->usage);
spin_unlock_irq(&idr_lock);
if (!found)
return -ENODEV;
file->private_data = pps;
return 0;
}
Is that right?
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Hello,
here my last patch for PPS support.
I merged all notes received in these days, also I added a (possible?)
fix for the problem pointed out by Roland.
If you think it's ok, please let me know so I can release version
5.0.0 for inclusion.
On the userland side some NTPD's reference clock has been tested and
they are working without patches (but one)! :)
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127