LinuxPPS (RESUBMIT 2): the PPS Linux implementation. - Kernel

This is a discussion on LinuxPPS (RESUBMIT 2): the PPS Linux implementation. - Kernel ; On Thu, Mar 20, 2008 at 01:03:56PM -0700, Andrew Morton wrote: > On Thu, 6 Mar 2008 13:09:00 +0100 > Rodolfo Giometti wrote: > > > +pps_core-y := pps.o kapi.o sysfs.o > > Does it compile OK with CONFIG_SYSFS=n? Yes. ...

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2
Results 21 to 31 of 31

Thread: LinuxPPS (RESUBMIT 2): the PPS Linux implementation.

  1. Re: [PATCH 1/7] LinuxPPS core support.

    On Thu, Mar 20, 2008 at 01:03:56PM -0700, Andrew Morton wrote:
    > On Thu, 6 Mar 2008 13:09:00 +0100
    > Rodolfo Giometti wrote:
    >
    > > +pps_core-y := pps.o kapi.o sysfs.o

    >
    > Does it compile OK with CONFIG_SYSFS=n?


    Yes. Tested.

    > > +#include
    > > +#include
    > > +#include
    > > +#include
    > > +#include
    > > +#include
    > > +#include
    > > +#include
    > > +#include
    > > +
    > > +/*
    > > + * Global variables
    > > + */
    > > +
    > > +DEFINE_SPINLOCK(idr_lock);

    >
    > This name is insufficiently specific. Not only does it risk linkage
    > errors, it makes it ahrd for poeple to work out where the symbol came from.
    >
    > I renamed it to pps_idr_lock.


    Fixed.

    > > +DEFINE_IDR(pps_idr);
    > >
    > > ...
    > >
    > > +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);
    > > +}
    > > +EXPORT_SYMBOL(pps_unregister_source);

    >
    > The wait_event() stuff really shouldn't be here: it should be integral to
    > the refcounting:
    >
    > void pps_dev_put(struct pps_device *pps)
    > {
    > spin_lock_irq(&pps_lock);
    > if (atomic_dec_and_test(&pps->usage))
    > idr_remove(&pps_idr, pps->id);
    > else
    > pps = NULL;
    > spin_unlock_irq(&pps_lock);
    > if (pps) {
    > /*
    > * Might need to do the below via schedule_work() if
    > * pps_dev_put() is to be callable from atomic context
    > */
    > pps_sysfs_remove_source_entry(pps);
    > pps_unregister_cdev(pps);
    > kfree(pps);
    > }
    > }


    I don't understand where I should use this function... :'(

    >
    > As it stands, there might be deadlocks such as when a process which itself
    > holds a ref on the pps_device (with an open fd?) calls
    > pps_unregister_source.


    I can add a wait_event_interruptible in order to allow userland to
    continue by receiving a signal. It could be acceptable?

    > Also, we need to take care that all processes which were waiting in
    > pps_unregister_source() get to finish their cleanup before we permit rmmod
    > to proceed. Is that handled somewhere?


    I don't understand the problem... this code as been added in order to
    avoid the case where a pps_event() is called while a process executes
    the pps_unregister_source(). If more processes try to execute this
    code the first which enters will execute idr_remove() which prevents
    another process to reach the wait_event()... is that wrong? =:-o

    >
    > > +void pps_event(int source, int event, void *data)

    >
    > Please document the API in the kernel source. I realise there's a teeny
    > bit of documentation in pps.txt, but people don't think to look there and
    > it tends to go out of date.
    >
    > It doesn't have to be fancy formal kerneldoc - it's better to add *good*
    > comments which tell people what they need to know. For some reason people
    > seem to add useless obvious stuff when they do their comments in kerneldoc
    > form.


    Here my proposal. It could be enought?

    diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
    index 6cac7b8..34b3b22 100644
    --- a/drivers/pps/kapi.c
    +++ b/drivers/pps/kapi.c
    @@ -59,6 +59,18 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
    * Exported functions
    */

    +/* pps_register_source - add a PPS source in the system
    + *
    + * info: the PPS info struct
    + * default_params: the default PPS parameters of the new source
    + *
    + * This function is used to add a new PPS source in the system. The new
    + * source is described by info's fields and it will have, as default PPS
    + * parameters, the ones specified into default_params.
    + *
    + * The function returns, in case of success, the PPS source ID.
    + */
    +
    int pps_register_source(struct pps_source_info *info, int default_params)
    {
    struct pps_device *pps;
    @@ -151,6 +163,14 @@ pps_register_source_exit:
    }
    EXPORT_SYMBOL(pps_register_source);

    +/* pps_unregister_source - remove a PPS source from the system
    + *
    + * source: the PPS source ID
    + *
    + * This function is used to remove a previously registered PPS source from
    + * the system.
    + */
    +
    void pps_unregister_source(int source)
    {
    struct pps_device *pps;
    @@ -177,6 +197,20 @@ void pps_unregister_source(int source)
    }
    EXPORT_SYMBOL(pps_unregister_source);

    +/* pps_event - register a PPS event into the system
    + *
    + * source: the PPS source ID
    + * event: the event type
    + * data: userdef pointer
    + *
    + * This function is used by each PPS client in order to register a new
    + * PPS event into the system (it's usually called inside an IRQ handler).
    + *
    + * If an echo function is associated with the PPS source it will be called
    + * as:
    + * pps->info.echo(source, event, data);
    + */
    +
    void pps_event(int source, int event, void *data)
    {
    struct pps_device *pps;

    > > +{
    > > + struct pps_device *pps;
    > > + struct timespec __ts;
    > > + struct pps_ktime ts;
    > > + unsigned long flags;
    > > +
    > > + /* First of all we get the time stamp... */
    > > + getnstimeofday(&__ts);
    > > +
    > > + /* ... and translate it to PPS time data struct */
    > > + ts.sec = __ts.tv_sec;
    > > + ts.nsec = __ts.tv_nsec;
    > > +
    > > + if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
    > > + printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
    > > + event, source);
    > > + return;
    > > + }
    > > +
    > > + spin_lock_irqsave(&idr_lock, flags);
    > > + pps = idr_find(&pps_idr, source);
    > > +
    > > + /* If we find a valid PPS source we lock it before leaving
    > > + * the lock!
    > > + */
    > > + if (pps)
    > > + atomic_inc(&pps->usage);
    > > + spin_unlock_irqrestore(&idr_lock, flags);

    >
    > The above pattern is repeated rather a lot and could perhaps be extracted
    > into a nice pps_dev_get() helper.


    It could be... but, is it mandatory?

    > > + if (!pps)
    > > + return;
    > > +
    > > + pr_debug("PPS event on source %d at %llu.%06u\n",
    > > + pps->id, ts.sec, ts.nsec);
    > > +
    > > + spin_lock_irqsave(&pps->lock, flags);
    > > +
    > > + /* Must call the echo function? */
    > > + if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
    > > + pps->info.echo(source, event, data);
    > > +
    > > + /* Check the event */
    > > + pps->current_mode = pps->params.mode;
    > > + if (event & PPS_CAPTUREASSERT) {
    > > + /* We have to add an offset? */
    > > + if (pps->params.mode & PPS_OFFSETASSERT)
    > > + pps_add_offset(&ts, &pps->params.assert_off_tu);
    > > +
    > > + /* Save the time stamp */
    > > + pps->assert_tu = ts;
    > > + pps->assert_sequence++;
    > > + pr_debug("capture assert seq #%u for source %d\n",
    > > + pps->assert_sequence, source);
    > > + }
    > > + if (event & PPS_CAPTURECLEAR) {
    > > + /* We have to add an offset? */
    > > + if (pps->params.mode & PPS_OFFSETCLEAR)
    > > + pps_add_offset(&ts, &pps->params.clear_off_tu);
    > > +
    > > + /* Save the time stamp */
    > > + pps->clear_tu = ts;
    > > + pps->clear_sequence++;
    > > + pr_debug("capture clear seq #%u for source %d\n",
    > > + pps->clear_sequence, source);
    > > + }
    > > +
    > > + pps->go = ~0;
    > > + wake_up_interruptible(&pps->queue);
    > > +
    > > + kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
    > > +
    > > + spin_unlock_irqrestore(&pps->lock, flags);
    > > +
    > > + /* Now we can release the PPS source for (possible) deregistration */
    > > + spin_lock_irqsave(&idr_lock, flags);
    > > + atomic_dec(&pps->usage);
    > > + wake_up_all(&pps->usage_queue);
    > > + spin_unlock_irqrestore(&idr_lock, flags);
    > > +}
    > > +EXPORT_SYMBOL(pps_event);
    > >
    > > ...
    > >
    > > +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);

    >
    > That looks a bit odd. How can the pps_device not be registered in the IDR
    > tree at this stage?


    This is needed in this scenario: a process just enters into
    pps_unregister_source() while another one is executing pps_cdev_open()
    on the same PPS source. We cannot open an unregistered source...

    >
    >
    > > + if (!found)
    > > + return -ENODEV;
    > > +
    > > + file->private_data = pps;
    > > +
    > > + return 0;
    > > +}
    > >
    > > ...
    > >
    > > +/* Kernel consumers */
    > > +#define PPS_KC_HARDPPS 0 /* hardpps() (or equivalent) */
    > > +#define PPS_KC_HARDPPS_PLL 1 /* hardpps() constrained to
    > > + use a phase-locked loop */
    > > +#define PPS_KC_HARDPPS_FLL 2 /* hardpps() constrained to
    > > + use a frequency-locked loop */
    > > +/*
    > > + * Here begins the implementation-specific part!
    > > + */
    > > +
    > > +struct pps_fdata {
    > > + struct pps_kinfo info;
    > > + struct pps_ktime timeout;
    > > +};
    > > +
    > > +#include
    > > +
    > > +#define PPS_CHECK _IO('P', 0)
    > > +#define PPS_GETPARAMS _IOR('P', 1, struct pps_kparams *)
    > > +#define PPS_SETPARAMS _IOW('P', 2, struct pps_kparams *)
    > > +#define PPS_GETCAP _IOR('P', 3, int *)
    > > +#define PPS_FETCH _IOWR('P', 4, struct pps_fdata *)
    > > +
    > > +#ifdef __KERNEL__
    > > +
    > > +#include
    > > +#include
    > > +
    > > +#define PPS_VERSION "5.0.0"
    > > +#define PPS_MAX_SOURCES 16 /* should be enough... */

    >
    > It's nice to avoid sprinkling #includes throughout the file, please.
    > People expect to be able to see what's being included in the first
    > screenful of the file.


    Fixed.

    > > +/*
    > > + * Global defines
    > > + */
    > > +
    > > +/* The specific PPS source info */
    > > +struct pps_source_info {
    > > + char name[PPS_MAX_NAME_LEN]; /* simbolic name */
    > > + char path[PPS_MAX_NAME_LEN]; /* path of connected device */
    > > + int mode; /* PPS's allowed mode */
    > > +
    > > + void (*echo)(int source, int event, void *data); /* PPS echo function */
    > > +
    > > + struct module *owner;
    > > + struct device *dev;
    > > +};
    > > +

    >


    I just fixed all your suggestions (apart what reported above) and I'm
    ready to propose a new patch set.

    Please let me know what else should I fix and if the proposed inline
    documentation is ok.

    Ciao,

    Rodolfo

    --

    GNU/Linux Solutions e-mail: giometti@enneenne.com
    Linux Device Driver giometti@gnudd.com
    Embedded Systems giometti@linux.it
    UNIX programming phone: +39 349 2432127
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti wrote:

    > > > +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);
    > > > +}
    > > > +EXPORT_SYMBOL(pps_unregister_source);

    > >
    > > The wait_event() stuff really shouldn't be here: it should be integral to
    > > the refcounting:
    > >
    > > void pps_dev_put(struct pps_device *pps)
    > > {
    > > spin_lock_irq(&pps_lock);
    > > if (atomic_dec_and_test(&pps->usage))
    > > idr_remove(&pps_idr, pps->id);
    > > else
    > > pps = NULL;
    > > spin_unlock_irq(&pps_lock);
    > > if (pps) {
    > > /*
    > > * Might need to do the below via schedule_work() if
    > > * pps_dev_put() is to be callable from atomic context
    > > */
    > > pps_sysfs_remove_source_entry(pps);
    > > pps_unregister_cdev(pps);
    > > kfree(pps);
    > > }
    > > }

    >
    > I don't understand where I should use this function... :'(


    This is boilerplate standard linux kernel reference counting.

    > >
    > > As it stands, there might be deadlocks such as when a process which itself
    > > holds a ref on the pps_device (with an open fd?) calls
    > > pps_unregister_source.

    >
    > I can add a wait_event_interruptible in order to allow userland to
    > continue by receiving a signal. It could be acceptable?


    There should be no need to "wait" for anything. When the final reference
    to an object is released, that object is cleaned up. Just like we do for
    inodes, dentries, pages, files, and 100 other kernel objects.

    The need to wait for something else to go away is a big red flag with
    "busted refcounting" written on it.

    > > Also, we need to take care that all processes which were waiting in
    > > pps_unregister_source() get to finish their cleanup before we permit rmmod
    > > to proceed. Is that handled somewhere?

    >
    > I don't understand the problem... this code as been added in order to
    > avoid the case where a pps_event() is called while a process executes
    > the pps_unregister_source(). If more processes try to execute this
    > code the first which enters will execute idr_remove() which prevents
    > another process to reach the wait_event()... is that wrong? =:-o


    I was asking you!

    We should get the reference counting and object lifetimes sorted out first.
    There should be no "wait for to be released" code. Once that is
    in place, things like rmmod will also sort themselves out: it just won't be
    possible to remove the module while there are live references to objects.


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH 1/7] LinuxPPS core support.

    On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti wrote:

    > This patch adds the kernel side of the PPS support currently named
    > "LinuxPPS".


    powerpc:

    drivers/pps/pps.c: In function 'pps_cdev_ioctl':
    drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64'
    drivers/pps/kapi.c: In function 'pps_event':
    drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64'
    drivers/pps/sysfs.c: In function 'pps_show_assert':
    drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    drivers/pps/sysfs.c: In function 'pps_show_clear':
    drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH 1/7] LinuxPPS core support.

    On Thu, Mar 27, 2008 at 08:25:31PM -0700, Andrew Morton wrote:
    > On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti wrote:
    > > >
    > > > As it stands, there might be deadlocks such as when a process which itself
    > > > holds a ref on the pps_device (with an open fd?) calls
    > > > pps_unregister_source.

    > >
    > > I can add a wait_event_interruptible in order to allow userland to
    > > continue by receiving a signal. It could be acceptable?

    >
    > There should be no need to "wait" for anything. When the final reference
    > to an object is released, that object is cleaned up. Just like we do for
    > inodes, dentries, pages, files, and 100 other kernel objects.
    >
    > The need to wait for something else to go away is a big red flag with
    > "busted refcounting" written on it.
    >
    > > > Also, we need to take care that all processes which were waiting in
    > > > pps_unregister_source() get to finish their cleanup before we permit rmmod
    > > > to proceed. Is that handled somewhere?

    > >
    > > I don't understand the problem... this code as been added in order to
    > > avoid the case where a pps_event() is called while a process executes
    > > the pps_unregister_source(). If more processes try to execute this
    > > code the first which enters will execute idr_remove() which prevents
    > > another process to reach the wait_event()... is that wrong? =:-o

    >
    > I was asking you!
    >
    > We should get the reference counting and object lifetimes sorted out first.
    > There should be no "wait for to be released" code. Once that is
    > in place, things like rmmod will also sort themselves out: it just won't be
    > possible to remove the module while there are live references to objects.

    The problem is related to serial and parallel clients.

    The PPS source related to a serial port (or a parallel one) uses the
    serial (or parallel) IRQ to get PPS timestamps and it could be
    possible that a process tries to close the PPS source while another
    CPU is runnig the serial IRQ, so I cannot remove the PPS object until
    the IRQ handler is finished its job on the PPS object.

    For clients (currently none which define their own IRQ handler for
    PPS timestamps managing the problem doesn't arise at all.

    Ciao,

    Rodolfo

    --

    GNU/Linux Solutions e-mail: giometti@enneenne.com
    Linux Device Driver giometti@gnudd.com
    Embedded Systems giometti@linux.it
    UNIX programming phone: +39 349 2432127
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, 1 Apr 2008 10:42:14 +0200 Rodolfo Giometti wrote:

    > On Thu, Mar 27, 2008 at 08:25:31PM -0700, Andrew Morton wrote:
    > > On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti wrote:
    > > > >
    > > > > As it stands, there might be deadlocks such as when a process which itself
    > > > > holds a ref on the pps_device (with an open fd?) calls
    > > > > pps_unregister_source.
    > > >
    > > > I can add a wait_event_interruptible in order to allow userland to
    > > > continue by receiving a signal. It could be acceptable?

    > >
    > > There should be no need to "wait" for anything. When the final reference
    > > to an object is released, that object is cleaned up. Just like we do for
    > > inodes, dentries, pages, files, and 100 other kernel objects.
    > >
    > > The need to wait for something else to go away is a big red flag with
    > > "busted refcounting" written on it.
    > >
    > > > > Also, we need to take care that all processes which were waiting in
    > > > > pps_unregister_source() get to finish their cleanup before we permit rmmod
    > > > > to proceed. Is that handled somewhere?
    > > >
    > > > I don't understand the problem... this code as been added in order to
    > > > avoid the case where a pps_event() is called while a process executes
    > > > the pps_unregister_source(). If more processes try to execute this
    > > > code the first which enters will execute idr_remove() which prevents
    > > > another process to reach the wait_event()... is that wrong? =:-o

    > >
    > > I was asking you!
    > >
    > > We should get the reference counting and object lifetimes sorted out first.
    > > There should be no "wait for to be released" code. Once that is
    > > in place, things like rmmod will also sort themselves out: it just won't be
    > > possible to remove the module while there are live references to objects.
    >
    > The problem is related to serial and parallel clients.
    >
    > The PPS source related to a serial port (or a parallel one) uses the
    > serial (or parallel) IRQ to get PPS timestamps and it could be
    > possible that a process tries to close the PPS source while another
    > CPU is runnig the serial IRQ, so I cannot remove the PPS object until
    > the IRQ handler is finished its job on the PPS object.
    >
    > For clients (currently none which define their own IRQ handler for
    > PPS timestamps managing the problem doesn't arise at all.

    This can all be handled with suitable locking and refcounting. The device
    which is delivering PPS interrupts has a reference on the PPS data
    structures. If userspace has PPS open then it also has a reference.

    The thread of control which releases the last reference to the PPS data
    structures also frees them all up. This may require a schedule_work() if
    we need to support release-from-interrupt (as it appears that we do), but
    that's OK - we just need to be able to make the PPS data structures
    ineligible for new lookups while the schedule_work() is pending.

    There should be no need for any thread of control to wait for any other thread
    of control to do anything. Get the refcounting right and everything
    can be done synchronously.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [PATCH 1/7] LinuxPPS core support.

    On Fri, Mar 28, 2008 at 03:21:39AM -0700, Andrew Morton wrote:
    > On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti wrote:
    >
    > > This patch adds the kernel side of the PPS support currently named
    > > "LinuxPPS".

    >
    > powerpc:
    >
    > drivers/pps/pps.c: In function 'pps_cdev_ioctl':
    > drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64'
    > drivers/pps/kapi.c: In function 'pps_event':
    > drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64'
    > drivers/pps/sysfs.c: In function 'pps_show_assert':
    > drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > drivers/pps/sysfs.c: In function 'pps_show_clear':
    > drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'


    Oops! I have no powerpc system... how can I solve this issue? Using
    type __s64 is not compiling safe? =:-o

    Googling on the net I found:

    #include

    int64_t var;
    sprintf (buf, "%" PRId64, var);

    but this is not implemented into the kernel... maybe I can add it into
    my driver or should I propose a patch for the kernel?

    Ciao,

    Rodolfo

    --

    GNU/Linux Solutions e-mail: giometti@enneenne.com
    Linux Device Driver giometti@gnudd.com
    Embedded Systems giometti@linux.it
    UNIX programming phone: +39 349 2432127
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, 1 Apr 2008 10:59:50 +0200 Rodolfo Giometti wrote:

    > On Fri, Mar 28, 2008 at 03:21:39AM -0700, Andrew Morton wrote:
    > > On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti wrote:
    > >
    > > > This patch adds the kernel side of the PPS support currently named
    > > > "LinuxPPS".

    > >
    > > powerpc:
    > >
    > > drivers/pps/pps.c: In function 'pps_cdev_ioctl':
    > > drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64'
    > > drivers/pps/kapi.c: In function 'pps_event':
    > > drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64'
    > > drivers/pps/sysfs.c: In function 'pps_show_assert':
    > > drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > > drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > > drivers/pps/sysfs.c: In function 'pps_show_clear':
    > > drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > > drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'

    >
    > Oops! I have no powerpc system... how can I solve this issue?


    Use %lld (if these things can legitimately be negative - otherwise %llu)
    and cast the argument to (long long) or (unsigned long long).
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, Apr 01, 2008 at 02:09:46AM -0700, Andrew Morton wrote:
    > On Tue, 1 Apr 2008 10:59:50 +0200 Rodolfo Giometti wrote:
    >
    > > On Fri, Mar 28, 2008 at 03:21:39AM -0700, Andrew Morton wrote:
    > > > On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti wrote:
    > > >
    > > > > This patch adds the kernel side of the PPS support currently named
    > > > > "LinuxPPS".
    > > >
    > > > powerpc:
    > > >
    > > > drivers/pps/pps.c: In function 'pps_cdev_ioctl':
    > > > drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64'
    > > > drivers/pps/kapi.c: In function 'pps_event':
    > > > drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64'
    > > > drivers/pps/sysfs.c: In function 'pps_show_assert':
    > > > drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > > > drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > > > drivers/pps/sysfs.c: In function 'pps_show_clear':
    > > > drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'
    > > > drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'

    > >
    > > Oops! I have no powerpc system... how can I solve this issue?

    >
    > Use %lld (if these things can legitimately be negative - otherwise %llu)
    > and cast the argument to (long long) or (unsigned long long).


    Ok, here my modifications:

    diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
    index 34b3b22..d75c8c8 100644
    --- a/drivers/pps/kapi.c
    +++ b/drivers/pps/kapi.c
    @@ -245,7 +245,7 @@ void pps_event(int source, int event, void *data)
    return;

    pr_debug("PPS event on source %d at %llu.%06u\n",
    - pps->id, ts.sec, ts.nsec);
    + pps->id, (unsigned long long) ts.sec, ts.nsec);

    spin_lock_irqsave(&pps->lock, flags);

    diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
    index a82b1d8..5cbfeb9 100644
    --- a/drivers/pps/pps.c
    +++ b/drivers/pps/pps.c
    @@ -164,7 +164,8 @@ static int pps_cdev_ioctl(struct inode *inode, struct file *
    err = wait_event_interruptible(pps->queue, pps->go);
    else {
    pr_debug("timeout %lld.%09d\n",
    - fdata.timeout.sec, fdata.timeout.nsec);
    + (long long) fdata.timeout.sec,
    + fdata.timeout.nsec);
    ticks = fdata.timeout.sec * HZ;
    ticks += fdata.timeout.nsec / (NSEC_PER_SEC / HZ);

    diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
    index 3af773a..0520f62 100644
    --- a/drivers/pps/sysfs.c
    +++ b/drivers/pps/sysfs.c
    @@ -38,7 +38,7 @@ static ssize_t pps_show_assert(struct device *dev,
    return 0;

    return sprintf(buf, "%lld.%09d#%d\n",
    - pps->assert_tu.sec, pps->assert_tu.nsec,
    + (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
    pps->assert_sequence);
    }
    DEVICE_ATTR(assert, S_IRUGO, pps_show_assert, NULL);
    @@ -52,7 +52,7 @@ static ssize_t pps_show_clear(struct device *dev,
    return 0;

    return sprintf(buf, "%lld.%09d#%d\n",
    - pps->clear_tu.sec, pps->clear_tu.nsec,
    + (long long) pps->clear_tu.sec, pps->clear_tu.nsec,
    pps->clear_sequence);
    }
    DEVICE_ATTR(clear, S_IRUGO, pps_show_clear, NULL);

    This compile clearly on x86. I'm going to propose a new patch ASAP.

    Thanks,

    Rodolfo

    --

    GNU/Linux Solutions e-mail: giometti@enneenne.com
    Linux Device Driver giometti@gnudd.com
    Embedded Systems giometti@linux.it
    UNIX programming phone: +39 349 2432127
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, Apr 01, 2008 at 01:55:55AM -0700, Andrew Morton wrote:
    > On Tue, 1 Apr 2008 10:42:14 +0200 Rodolfo Giometti wrote:
    >
    > > On Thu, Mar 27, 2008 at 08:25:31PM -0700, Andrew Morton wrote:
    > > > On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti wrote:
    > > > > >
    > > > > > As it stands, there might be deadlocks such as when a process which itself
    > > > > > holds a ref on the pps_device (with an open fd?) calls
    > > > > > pps_unregister_source.
    > > > >
    > > > > I can add a wait_event_interruptible in order to allow userland to
    > > > > continue by receiving a signal. It could be acceptable?
    > > >
    > > > There should be no need to "wait" for anything. When the final reference
    > > > to an object is released, that object is cleaned up. Just like we do for
    > > > inodes, dentries, pages, files, and 100 other kernel objects.
    > > >
    > > > The need to wait for something else to go away is a big red flag with
    > > > "busted refcounting" written on it.
    > > >
    > > > > > Also, we need to take care that all processes which were waiting in
    > > > > > pps_unregister_source() get to finish their cleanup before we permit rmmod
    > > > > > to proceed. Is that handled somewhere?
    > > > >
    > > > > I don't understand the problem... this code as been added in order to
    > > > > avoid the case where a pps_event() is called while a process executes
    > > > > the pps_unregister_source(). If more processes try to execute this
    > > > > code the first which enters will execute idr_remove() which prevents
    > > > > another process to reach the wait_event()... is that wrong? =:-o
    > > >
    > > > I was asking you!
    > > >
    > > > We should get the reference counting and object lifetimes sorted out first.
    > > > There should be no "wait for to be released" code. Once that is
    > > > in place, things like rmmod will also sort themselves out: it just won't be
    > > > possible to remove the module while there are live references to objects.
    > >
    > > The problem is related to serial and parallel clients.
    > >
    > > The PPS source related to a serial port (or a parallel one) uses the
    > > serial (or parallel) IRQ to get PPS timestamps and it could be
    > > possible that a process tries to close the PPS source while another
    > > CPU is runnig the serial IRQ, so I cannot remove the PPS object until
    > > the IRQ handler is finished its job on the PPS object.
    > >
    > > For clients (currently none which define their own IRQ handler for
    > > PPS timestamps managing the problem doesn't arise at all.
    >
    > This can all be handled with suitable locking and refcounting. The device
    > which is delivering PPS interrupts has a reference on the PPS data
    > structures. If userspace has PPS open then it also has a reference.
    >
    > The thread of control which releases the last reference to the PPS data
    > structures also frees them all up. This may require a schedule_work() if
    > we need to support release-from-interrupt (as it appears that we do), but
    > that's OK - we just need to be able to make the PPS data structures
    > ineligible for new lookups while the schedule_work() is pending.
    >
    > There should be no need for any thread of control to wait for any other thread
    > of control to do anything. Get the refcounting right and everything
    > can be done synchronously.

    So, if I well understand your suggestion, I should manage the object
    clean-up into pps_cdev_release() when pps->usage reaches 0, so the
    pps_unregister_source() can do only the following two steps:

    pps_unregister_cdev(pps);
    kfree(pps);

    Is that right?

    Also, can you please suggest me an example (URL or filename) about
    schedule_work() usage in case of release-from-interrupt?

    Thanks,

    Rodolfo

    --

    GNU/Linux Solutions e-mail: giometti@enneenne.com
    Linux Device Driver giometti@gnudd.com
    Embedded Systems giometti@linux.it
    UNIX programming phone: +39 349 2432127
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  10. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, Apr 01, 2008 at 01:55:55AM -0700, Andrew Morton wrote:
    >
    > This can all be handled with suitable locking and refcounting. The device
    > which is delivering PPS interrupts has a reference on the PPS data
    > structures. If userspace has PPS open then it also has a reference.
    >
    > The thread of control which releases the last reference to the PPS data
    > structures also frees them all up. This may require a schedule_work() if
    > we need to support release-from-interrupt (as it appears that we do), but
    > that's OK - we just need to be able to make the PPS data structures
    > ineligible for new lookups while the schedule_work() is pending.
    >
    > There should be no need for any thread of control to wait for any other thread
    > of control to do anything. Get the refcounting right and everything
    > can be done synchronously.


    Here my solution by using get/put functions:

    diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
    index d75c8c8..61c1569 100644
    --- a/drivers/pps/kapi.c
    +++ b/drivers/pps/kapi.c
    @@ -59,6 +59,62 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
    * Exported functions
    */

    +/* pps_get_source - find a PPS source
    + *
    + * source: the PPS source ID.
    + *
    + * This function is used to find an already registered PPS source into the
    + * system.
    + *
    + * The function returns NULL if found nothing, otherwise it returns a pointer
    + * to the PPS source data struct (the refcounter is incremented by 1).
    + */
    +
    +struct pps_device *pps_get_source(int source)
    +{
    + struct pps_device *pps;
    + unsigned long flags;
    +
    + spin_lock_irqsave(&pps_idr_lock, flags);
    +
    + pps = idr_find(&pps_idr, source);
    + if (pps != NULL)
    + atomic_inc(&pps->usage);
    +
    + spin_unlock_irqrestore(&pps_idr_lock, flags);
    +
    + return pps;
    +}
    +
    +/* pps_put_source - free the PPS source data
    + *
    + * pps: a pointer to the PPS source.
    + *
    + * This function is used to free a PPS data struct if its refcount is 0.
    + */
    +
    +void pps_put_source(struct pps_device *pps)
    +{
    + unsigned long flags;
    +
    + spin_lock_irqsave(&pps_idr_lock, flags);
    +
    + BUG_ON(atomic_read(&pps->usage) == 0);
    +
    + if (!atomic_dec_and_test(&pps->usage))
    + goto exit;
    +
    + /* No more reference to the PPS source. We can safely remove the
    + * PPS data struct.
    + */
    + idr_remove(&pps_idr, pps->id);
    +
    + kfree(pps);
    +
    +exit:
    + spin_unlock_irqrestore(&pps_idr_lock, flags);
    +}
    +
    /* pps_register_source - add a PPS source in the system
    *
    * info: the PPS info struct
    @@ -133,8 +189,7 @@ int pps_register_source(struct pps_source_info *info, int default_params)

    init_waitqueue_head(&pps->queue);
    spin_lock_init(&pps->lock);
    - atomic_set(&pps->usage, 0);
    - init_waitqueue_head(&pps->usage_queue);
    + atomic_set(&pps->usage, 1);

    /* Create the char device */
    err = pps_register_cdev(pps);
    @@ -179,21 +234,14 @@ void pps_unregister_source(int source)
    pps = idr_find(&pps_idr, source);

    if (!pps) {
    + BUG();
    spin_unlock_irq(&pps_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(&pps_idr_lock);

    - wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
    -
    pps_unregister_cdev(pps);
    - kfree(pps);
    + pps_put_source(pps);
    }
    EXPORT_SYMBOL(pps_unregister_source);

    @@ -231,16 +279,7 @@ void pps_event(int source, int event, void *data)
    return;
    }

    - spin_lock_irqsave(&pps_idr_lock, flags);
    - pps = idr_find(&pps_idr, source);
    -
    - /* If we find a valid PPS source we lock it before leaving
    - * the lock!
    - */
    - if (pps)
    - atomic_inc(&pps->usage);
    - spin_unlock_irqrestore(&pps_idr_lock, flags);
    -
    + pps = pps_get_source(source);
    if (!pps)
    return;

    @@ -286,9 +325,6 @@ void pps_event(int source, int event, void *data)
    spin_unlock_irqrestore(&pps->lock, flags);

    /* Now we can release the PPS source for (possible) deregistration */
    - spin_lock_irqsave(&pps_idr_lock, flags);
    - atomic_dec(&pps->usage);
    - wake_up_all(&pps->usage_queue);
    - spin_unlock_irqrestore(&pps_idr_lock, flags);
    + pps_put_source(pps);
    }
    EXPORT_SYMBOL(pps_event);
    diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
    index 5cbfeb9..a46f8f4 100644
    --- a/drivers/pps/pps.c
    +++ b/drivers/pps/pps.c
    @@ -214,15 +214,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
    struct pps_device, cdev);
    int found;

    - spin_lock_irq(&pps_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(&pps_idr_lock);
    -
    + found = pps_get_source(pps->id) != 0;
    if (!found)
    return -ENODEV;

    @@ -236,8 +228,7 @@ static int pps_cdev_release(struct inode *inode, struct file *file)
    struct pps_device *pps = file->private_data;

    /* Free the PPS source and wake up (possible) deregistration */
    - atomic_dec(&pps->usage);
    - wake_up_all(&pps->usage_queue);
    + pps_put_source(pps);

    return 0;
    }
    diff --git a/include/linux/pps.h b/include/linux/pps.h
    index aca0e77..e23aaa6 100644
    --- a/include/linux/pps.h
    +++ b/include/linux/pps.h
    @@ -173,7 +173,6 @@ struct pps_device {
    spinlock_t lock;

    atomic_t usage; /* usage count */
    - wait_queue_head_t usage_queue;
    };

    /*
    @@ -189,6 +188,8 @@ extern struct device_attribute pps_attrs[];
    * Exported functions
    */

    +struct pps_device *pps_get_source(int source);
    +extern void pps_put_source(struct pps_device *pps);
    extern int pps_register_source(struct pps_source_info *info,
    int default_params);
    extern void pps_unregister_source(int source);


    I'll send a new patchset ASAP!

    Rodolfo

    --

    GNU/Linux Solutions e-mail: giometti@enneenne.com
    Linux Device Driver giometti@gnudd.com
    Embedded Systems giometti@linux.it
    UNIX programming phone: +39 349 2432127
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  11. Re: [PATCH 1/7] LinuxPPS core support.

    On Tue, 1 Apr 2008 23:45:22 +0200
    Rodolfo Giometti wrote:

    > On Tue, Apr 01, 2008 at 01:55:55AM -0700, Andrew Morton wrote:
    > >
    > > This can all be handled with suitable locking and refcounting. The device
    > > which is delivering PPS interrupts has a reference on the PPS data
    > > structures. If userspace has PPS open then it also has a reference.
    > >
    > > The thread of control which releases the last reference to the PPS data
    > > structures also frees them all up. This may require a schedule_work() if
    > > we need to support release-from-interrupt (as it appears that we do), but
    > > that's OK - we just need to be able to make the PPS data structures
    > > ineligible for new lookups while the schedule_work() is pending.
    > >
    > > There should be no need for any thread of control to wait for any other thread
    > > of control to do anything. Get the refcounting right and everything
    > > can be done synchronously.

    >
    > Here my solution by using get/put functions:


    That's looking promising.

    > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
    > index d75c8c8..61c1569 100644
    > --- a/drivers/pps/kapi.c
    > +++ b/drivers/pps/kapi.c
    > @@ -59,6 +59,62 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
    > * Exported functions
    > */
    >
    > +/* pps_get_source - find a PPS source
    > + *
    > + * source: the PPS source ID.
    > + *
    > + * This function is used to find an already registered PPS source into the
    > + * system.
    > + *
    > + * The function returns NULL if found nothing, otherwise it returns a pointer
    > + * to the PPS source data struct (the refcounter is incremented by 1).
    > + */
    > +
    > +struct pps_device *pps_get_source(int source)
    > +{
    > + struct pps_device *pps;
    > + unsigned long flags;
    > +
    > + spin_lock_irqsave(&pps_idr_lock, flags);
    > +
    > + pps = idr_find(&pps_idr, source);
    > + if (pps != NULL)
    > + atomic_inc(&pps->usage);
    > +
    > + spin_unlock_irqrestore(&pps_idr_lock, flags);
    > +
    > + return pps;
    > +}
    > +
    > +/* pps_put_source - free the PPS source data
    > + *
    > + * pps: a pointer to the PPS source.
    > + *
    > + * This function is used to free a PPS data struct if its refcount is 0.
    > + */
    > +
    > +void pps_put_source(struct pps_device *pps)
    > +{
    > + unsigned long flags;
    > +
    > + spin_lock_irqsave(&pps_idr_lock, flags);
    > +
    > + BUG_ON(atomic_read(&pps->usage) == 0);


    Please don't forget to use checkpatch.

    > + if (!atomic_dec_and_test(&pps->usage))
    > + goto exit;
    > +
    > + /* No more reference to the PPS source. We can safely remove the
    > + * PPS data struct.
    > + */
    > + idr_remove(&pps_idr, pps->id);
    > +
    > + kfree(pps);
    > +
    > +exit:
    > + spin_unlock_irqrestore(&pps_idr_lock, flags);
    > +}


    It'd be slightly moer efficient to move the kfree outside the spinlock:

    if (!atomic_dec_and_test(&pps->usage)) {
    pps = NULL;
    goto exit;
    }

    /* No more reference to the PPS source. We can safely remove the
    * PPS data struct.
    */
    idr_remove(&pps_idr, pps->id);
    exit:
    spin_unlock_irqrestore(&pps_idr_lock, flags);
    kfree(pps);
    }


    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/