Adding ISRs in 2 Dig-Input drivers

[Der Alte]

Hi folks,

I added Interrupt-Service-Routines in adv_pci_dio.c and adl_pci7x3c.c but now Im stuck.

The ISRs trigger on IDI0 and IDI1 rising edges and only make some printk so far and acknowledge the IRQ in the hardware. I have to synchronize to a  20 usec pulse after 1 second of silence and immediately wake up a user process waiting in select() or read()

(-> poll_wait() or wait_event_interruptible() for a wait_queue_head_t ).

Some wake_up_interruptible() from out of the ISR should do the task.

I hope, the async stuff in CoMeDI is based on such mechanisms and would like to call

    //comedi_buf_write_samples(sd1_p, &sd1_p->state, 1);  /* 2020_08_21: noch illegal ! */
    //comedi_handle_events(dev, sd1_p);  /*  ->async == null ! */

in my ISRs as I found in other CoMeDI ISRs for AI channels  but something is still missing.

The subdevice->async member is still NULL on both drivers and I can't seem to find out, what to do, to make _DI  subdevices with valid ->async -pointers.

The example driver comedi_test.c seems to have useable ->async members ( != NULL ) in it's subdevices and I hoped, they would be provided if I fill in some more members during  auto_attach()

                s->len_chanlist    = 1;
                s->do_cmdtest    = pci_dio_asy_cmdtest;
                s->do_cmd    = pci_dio_asy_cmd;
                s->cancel    = pci_dio_asy_cancel;

But that didn't do the trick.

comedi_parport.c seems to do almost what I imagine, using the ACK Pin but if I load the module, nothing really happens. The mainboards don't have parports anymore and my old ISA cards don't fit in anymore. :-O

Ciao and TIA,

Bernd

[Der Alte]

Hi folks,

seems like I am one step further now. The ->async pointer is != NULL now! :-)

But I'm wondering again! I have additional _subd# - Inodes now in /dev/:

crw-rw---- 1 root dialout 98,  0 Aug 28 07:10 /dev/comedi0
crw-rw---- 1 root dialout 98, 48 Aug 28 07:10 /dev/comedi0_subd0
crw-rw---- 1 root dialout 98,  1 Aug 28 07:10 /dev/comedi1
crw-rw---- 1 root dialout 98, 49 Aug 28 07:10 /dev/comedi1_subd0
crw-rw---- 1 root dialout 98, 50 Aug 28 07:10 /dev/comedi1_subd1

The problem was, that I used a yet invalid  if(dev->irq) condition around the assignments shown above.

Since I really assign   s->do_cmd    = pci_dio_asy_cmd;  , I have 'valid' async pointers in the comedi_subdevice.

In the ISR I do :

  async_p = sd0_p->async;

  But calling

     if(async_p)
     {

        comedi_buf_write_samples(sd0_p, &sd0_p->state, 1);
        comedi_handle_events(dev, sd0_p);

     }
in the ISR still crashes the kernel. :-(

Do I have to use other, new subdevices in my ISR now, as I have new Inodes?

Are there additional comedi_subdevice structs somewhere now?

Ciao, Bernd

[Ian Abbott]

On 28/08/2020 09:59, Der Alte wrote:
> Hi folks,
> seems like I am one step further now. The ->async pointer is != NULL
> now! :-)
> But I'm wondering again! I have additional _subd# - Inodes now in /dev/:
>
> crw-rw---- 1 root dialout 98,  0 Aug 28 07:10 /dev/comedi0
> crw-rw---- 1 root dialout 98, 48 Aug 28 07:10 /dev/comedi0_subd0
> crw-rw---- 1 root dialout 98,  1 Aug 28 07:10 /dev/comedi1
> crw-rw---- 1 root dialout 98, 49 Aug 28 07:10 /dev/comedi1_subd0
> crw-rw---- 1 root dialout 98, 50 Aug 28 07:10 /dev/comedi1_subd1
>
> The problem was, that I used a yet invalid  if(dev->irq) condition
> around the assignments shown above.
> Since I really assign   s->do_cmd    = pci_dio_asy_cmd;  , I have
> 'valid' async pointers in the comedi_subdevice.
>
> In the ISR I do :
>
>   async_p = sd0_p->async;
>
>   But calling
>      if(async_p)
>      {
>         comedi_buf_write_samples(sd0_p, &sd0_p->state, 1);
>         comedi_handle_events(dev, sd0_p);
>      }
> in the ISR still crashes the kernel. :-(

For PCI devices (using IRQF_SHARED interrupt flag), it is possible for
the ISR to be called before the Comedi device is fully configured. Some
Comedi drivers test 'dev->attached' in their ISR. It is also possible
for your ISR to be called due to an interrupt raised by a different PCI
device sharing the same IRQ number, so the ISR should check that the
device really is interrupting.

> Do I have to use other, new subdevices in my ISR now, as I have new Inodes?
> Are there additional comedi_subdevice structs somewhere now?

Those /dev/comediX_subdY files are created dynamically for subdevices
that support asynchronous commands. They behave the same as the main
/dev/comediX files except the "read subdevice" or "write subdevice"
(used for the read and write file operations) may differ from the those
of main /dev/comediX file.

In the version of Comedi included in the Linux kernel sources, the
/dev/comediX_subdY files are a bit redundant now since it supports
additional methods of choosing the read or write subdevice on a per open
file basis (the COMEDI_SETRSUBD and COMEDI_SETWSUBD ioctls, used by the
comedi_set_read_subdevice() and comedi_set_write_subdevice() functions
in Comedilib). But those ioctls are not yet supported by the
"out-of-tree" version of Comedi.

--
-=( Ian Abbott <abb...@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268. Registered address: )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

[Der Alte]

Hi Ian,

thanks for your help. :-)

After loading the modules my ISRs are only triggered if I press a button on a cable so far. But indeed, on reboot the

pci_dio_isr()  in the adv_pci_dio.c is gets called some 3 during startup. The ISR checks if the IRQ-source was  the PLX9052 chip on the  Digital-IO card and does nothing but return IRQ_NONE in that case.

Meanwhile I have tried a bit more carefully to report the IRQ towards comedi and it seems, that I may call comedi_handle_events().

The ISR Codes look something like this so far:

drivers/adl_pci7x3x.c  ( subdev[0] of 2 is isolated input)

---------------------------------------------------------------------------------------

    intcsr = inw(dev_private->lcr_io_base + ADL_OP_LINTCSR);

...

   async_p = sd0_p->async;
    if((intcsr & LINTi1_EN_ACT_IDI0) == LINTi1_EN_ACT_IDI0)  /* 0x0005 LINTi1 is Enabled && IDI0 is 1 */
    {
      ISRprintf(" IDI0 ");

      if(async_p)
      {
        //async_p->cur_chan = 0;
        //spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
        //comedi_buf_write_samples(sd0_p, &sd0_p->state, 1);
        comedi_handle_events(dev, sd0_p);  /* 2020_08_21: noch illegal! */
        //spin_lock_irqsave(&dev->spinlock, cpu_flags);
      }
    }

    if((intcsr & LINTi2_EN_ACT_IDI1) == LINTi2_EN_ACT_IDI1)  /* 0x0028 LINTi2 is Enabled && IDI1 is  1 */
    {
      ISRprintf(" IDI1 ");

      if(async_p)
      {
        //async_p->cur_chan = 0;
        //spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
        //comedi_buf_write_samples(sd0_p, &sd0_p->state, 1);
        comedi_handle_events(dev, sd0_p);  /* 2020_08_21: noch illegal! */
        //spin_lock_irqsave(&dev->spinlock, cpu_flags);
      }
    }
    outb(0x00, dev->iobase + ADL_PT_CLRIRQ);

================================================

drivers/adv_pci_dio.c ( subdev[1] of 5 is isolated input)

---------------------------------------------------------------------------------------

    irqflags = inb(dev->iobase + PCI173X_INT_FLAG_REG);

...

   async_p = sd1_p->async;
    if(irqflags & 0x01)  /* IDI0 edge occurred */
    {
      ISRprintf(" IDI0 ");
      outb(irqflags, dev->iobase + PCI173X_INT_CLR_REG);

      if(async_p)
      {
        //async_p->cur_chan = 0;
        //spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
        //comedi_buf_write_samples(sd1_p, &sd1_p->state, 1);
        comedi_handle_events(dev, sd1_p);  /* 2020_08_21: noch illegal! */
      }
    }

    if(irqflags & 0x02)  /* IDI1 edge occurred */
    {
      ISRprintf(" IDI1 ");
      outb(irqflags, dev->iobase + PCI173X_INT_CLR_REG);

      if(async_p)
      {
        //async_p->cur_chan = 1;
        //spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
        //comedi_buf_write_samples(sd1_p, &sd1_p->state, 1);
        comedi_handle_events(dev, sd1_p);  /* 2020_08_21: noch illegal! */
      }
    }
    outb(irqflags, dev->iobase + PCI173X_INT_CLR_REG);

I guess, calling comedi_buf_write_samples() crashed the previous version.

Ciao, Bernd

[Der Alte]

What I forgot to write: The sources come with  openSuse15.1 and openSuse 15.2

Current Kernels:  4.12.14-lp151.28.59-default and 5.3.18-lp152.36-default

Ciao, Bernd

[Der Alte]

Hi experts,

I still have problems with what to do  / what is allowed in the new ISRs in adv_pci_dio.c and adl_pci_7x3x.c  .

I adapted the structure of the driver more to the pattern off comedi_parport.c becaus it was the only example I could find where a rising / falling edge of a digital input signal is used to generate an interrupt.

Because the digial input ports of the hardware have 2 pins which can generate interrupts, I created 2 new asynchronous 1 bit dig input subdevices per port to be able do possibly allow parallel select() calls from individual processes for each Interrupt source.

4 devices are reserved for legacy hardware as found in comedilib.pdf.

Node-01:~ # more /etc/modprobe.d/99-comedi.conf
options comedi comedi_num_legacy_minors=4

I did

comedi_config -v /dev/comedi1 comedi_parport 0x378,7

after that my comedi devices look like this in the moment:

Node-01:~ # ll /dev/comedi*
crw-rw---- 1 root dialout 98,  0 Sep 23 08:47 /dev/comedi0
crw-rw---- 1 root dialout 98,  1 Sep 23 08:47 /dev/comedi1
crw-rw---- 1 root dialout 98, 54 Sep 23 08:55 /dev/comedi1_subd3
crw-rw---- 1 root dialout 98,  2 Sep 23 08:47 /dev/comedi2
crw-rw---- 1 root dialout 98,  3 Sep 23 08:47 /dev/comedi3
crw-rw---- 1 root dialout 98,  4 Sep 23 08:47 /dev/comedi4
crw-rw---- 1 root dialout 98, 48 Sep 23 08:47 /dev/comedi4_subd2
crw-rw---- 1 root dialout 98, 49 Sep 23 08:47 /dev/comedi4_subd3
crw-rw---- 1 root dialout 98,  5 Sep 23 08:47 /dev/comedi5
crw-rw---- 1 root dialout 98, 50 Sep 23 08:47 /dev/comedi5_subd5
crw-rw---- 1 root dialout 98, 51 Sep 23 08:47 /dev/comedi5_subd6
crw-rw---- 1 root dialout 98, 52 Sep 23 08:47 /dev/comedi5_subd7
crw-rw---- 1 root dialout 98, 53 Sep 23 08:47 /dev/comedi5_subd8
Node-01:~ #

comedi4   is an Adlink LPCIe 7230 card, which has 1  16 Bit digital input port

comedi5   is an Advantech PCI 1730 card, which has 2  16 Bit digital input ports

So ffar, so good...

What I don't know is what exactly to do for CoMeDI in the ISR after an interrupt has occurred.

in comedi_parport.c the ISR looks like this:

static irqreturn_t parport_interrupt(int irq, void *d)
{
    struct comedi_device *dev = d;
    struct comedi_subdevice *s = dev->read_subdev;
    unsigned int ctrl;

  CFuncId("parport_interrupt() nsd=%d ", dev->n_subdevices);

    ctrl = inb(dev->iobase + PARPORT_CTRL_REG);
    if (!(ctrl & PARPORT_CTRL_IRQ_ENA))
        return IRQ_NONE;

    comedi_buf_write_samples(s, &s->state, 1);   /* ----------------------------- Will this work if I cause a real IRQ?? */
    comedi_handle_events(dev, s);

    return IRQ_HANDLED;
}

My ISRs have to look like this so far:

static irqreturn_t adl_pci7x3x_interrupt(int irq, void *p_device)
{
    struct comedi_device * const dev = p_device;
    struct adl_pci7x3x_private_data * const dev_private = dev->private;

    struct comedi_subdevice * sd_p = &dev->subdevices[2];

    struct comedi_async *   async_p = NULL;
    struct comedi_cmd *     ccmd_p = NULL;

    unsigned long cpu_flags;
    unsigned short intcsr;
    unsigned int subdev = 2;

  CFuncId("adl_pci7x3x_isr() nsd=%d ", dev->n_subdevices);

    if (!dev->attached)
    {
      return IRQ_NONE;
    }

    if(adl_pci7x3x_boards[dev_private->boardtype].di_nchan == 0)
    {   
      /* keine Inputs => Keine IRQs. */
      return IRQ_NONE;
    }

    /*  Check if we are source of interrupt */

    intcsr = inw(dev_private->lcr_io_base + ADL_OP_LINTCSR);

    ISRprintf(" IFL=%04X ", intcsr);
    if(!(intcsr & PLX9052_INTCSR_PCIENAB))  /* 0x0040 9052 PCI IRQ active? */
    {
      ISRprintf(" nix los ");
      return IRQ_NONE;
    }


    /* SubDev 2, 3 = Isolated DigIn , auf "SCSI2" Buchse!*/

    if((intcsr & LINTi1_EN_ACT_IDI0) == LINTi1_EN_ACT_IDI0)  /* 0x0005 LINTi1 is Enabled && IDI0 is 1 */
    {

      subdev = 2;
      ISRprintf(" IDI0 ");
    }
    /*endif((intcsr & LINTi1_EN_ACT_IDI0) == LINTi1_EN_ACT_IDI0)*/

    if((intcsr & LINTi2_EN_ACT_IDI1) == LINTi2_EN_ACT_IDI1)  /* 0x0028 LINTi2 is Enabled && IDI1 is  1 */
    {

      subdev = 3;
      ISRprintf(" IDI1 ");
    }
    /*endif((intcsr & LINTi2_EN_ACT_IDI1) == LINTi2_EN_ACT_IDI1)*/


    sd_p = &dev->subdevices[subdev];
    async_p = sd_p->async;
    ISRprintf(" sd%u async_p=%8p ", subdev, async_p);
    if(async_p)
    {
      //comedi_buf_write_samples(sd_p, &sd_p->state, 1);  /* 2020_09_21: Freezes the kernel !! ------------------------ !! */
      comedi_handle_events(dev, sd_p);   /* allowed since async_p is valid */
    }

    /* clear all current interrupt flags */

    outb(0x00, dev->iobase + ADL_PT_CLRIRQ);

  CFuncDone(" IRQ_HANDLED.\n");
    return IRQ_HANDLED;
}

Why does a call to   comedi_buf_write_samples()   crash the system in my drivers?

Is such a call (with zero data) needed to satisfy the CoMeDI framework?

So far I haven' been able to really test the comedi_parport driver with real interrupts because most of our mainboards don't have any real parports and an old Supermicro X7-DAL only has an internal connector and I haven't found the ribbon cable with DB25 female so far. :-(

Ciao, Bernd

[Der Alte]

Hi Ian,

I just had a thought on   comedi_buf_write_samples():

Is it only allowed to call that function if a device / subdevice is opened?

Or if a command is running?

My devices have always been closed so far or at maximum, monitor was running on the main device.

TIA,

[Ian Abbott]

On 25/09/2020 13:56, Der Alte wrote:
> Hi Ian,
> I just had a thought on   comedi_buf_write_samples():
> Is it only allowed to call that function if a device / subdevice is opened?
> Or if a command is running?
>
> My devices have always been closed so far or at maximum, monitor was
> running on the main device.
>
> TIA,
> Bernd

comedi_buf_write_samples() tries to update the scan progress for the
command. That could result in a divide-by-zero error if no command has
been set up.

The intended behaviour is that the driver should not add anything to the
buffer until the start trigger for the command occurs (if start_src is
TRIG_NOW, that will be when the subdevice's ->do_cmd() handler is
called), and it shouldn't add anything after the subdevice's ->cancel()
handler is called.

Note: the cancel() handler might be called from the interrupt handler
itself, via comedi_handle_events(), and it might also be called from
outside the interrupt handler for various reasons (e.g. the
COMEDI_CANCEL ioctl, or the device file being closed). If it uses a
spin-lock, it should use spin_lock_irqsave() and
spin_unlock_irqrestore(), and if the interrupt handler also uses that
spin-lock then it needs to be unlocked before calling
comedi_handle_events() to avoid a deadlock.

--
-=( Ian Abbott <abb...@mev.co.uk> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-

[Der Alte]

Hi Ian!

GREAT, GREAT, GREAT!!!

Suddenly all 3 drivers seem to behave / work like desired! :-)))

ledclock indeed seems to be near to the use case I have in mind, select() ing for an external trigger without polling oe busy-waiting.

After learning, that I don't have to use    ledclock -f /dev//dev/comedi2_subd3

but ledclock -f /dev/comedi2 -s 3   (for comedi_parport as conedi2) , I finally saw the calls to s->do_cmdtest, s->do_cmd and s->cancel come through. (I had added printk()s before.)

I also added fprintf()s in ledclock.c before every significant call to trace the flow, especially before the select() and the do_toggle() calls. With a select-tmeout of > 8 seconds I can well see if the flow continues or sleeps. To trigger the external events I press buttons or switches or on the parport I connect or disconnect a small pin plug.

Select sleeps long and instantly wakes up as soon as the signal edges occur. :-)

Although I haven't understood completely  why the user program has to look the way it looks now, especially the use of comedi_fileno(device) instead of opening the Unix device

crw-rw---- 1 root dialout 98, 48 Oct  1 11:13 /dev/comedi2_subd3
is magic to me. But I also have to analyze and understand the rest of the source code.

E.g. why the cmd has to look like it looks in the ledclock example so far.

Thanks and till later, Bernd

[Der Alte]

Hi Ian,

with me test program trig_ext.c, which works similar to ledclock but simply times the device & subdevice

(prints time and delta-time since last event plus the date read from the device) I have anhanced and improved the new auto_attach()  and ISRs etc. in  adv_pci_dio and adl_pci7x3x drivers from our OpenSuse distros in use.

They now only enable the IRQ on a Digital Input Line in the sd->do_cmd() and disable it in the sd->cancel() operations.

If I understand correctly, you are the maintainer of CoMeDI and I assume you would be the instance to give the new code or diffs to.

From my point of view it would be best if the changes would be publihed. But I have to check that with my employer who payed the hours I spent to implement the changes.

Further I guess that Suse is somewhat in the past with it's kernel-versions and I would have to apply the changes on the newest CoMeDI versions from github.

After that we would still have to wait for the main linux maintainers to take  your newest code base in and after that we would still not see it in Suse or RedHat kernels for some time, right?

[Ian Abbott]

Hello Bernd,

On 05/10/2020 13:28, Der Alte wrote:
> Hi Ian,
>

> with me test program trig_ext.c, which works similar to ledclock but
> simply times the device & subdevice
> (prints time and delta-time since last event plus the date read from the
> device) I have anhanced and improved the new auto_attach()  and ISRs
> etc. in  adv_pci_dio and adl_pci7x3x drivers from our OpenSuse distros
> in use.
>
> They now only enable the IRQ on a Digital Input Line in the sd->do_cmd()
> and disable it in the sd->cancel() operations.
>
> If I understand correctly, you are the maintainer of CoMeDI and I assume
> you would be the instance to give the new code or diffs to.

I can certainly review Comedi patches for the Linux kernel as I am one
of the maintainers, although Greg Kroah-Hartman is in overall control of
the "staging" area of the the Linux kernel where the Comedi subsystem
lives (for the fork of Comedi within the Linux sources, not the one on
GitHub - see below). Comedi patches for the Linux kernel sources get
committed to Greg's "staging" branches first, and then make their way
upwards to the mainline Linux sources for release.

> From my point of view it would be best if the changes would be
> publihed. But I have to check that with my employer who payed the hours
> I spent to implement the changes.

If you distribute modified binaries without making the source code
available, you would be violating the GPL. But apart from that,
publishing the changes would reduce your maintenance burden if the
changes are already incorporated in future kernels (but see my remark
below about distro kernels).

> Further I guess that Suse is somewhat in the past with it's
> kernel-versions and I would have to apply the changes on the newest
> CoMeDI versions from github.

There are two main forks of the Comedi kernel code that have diverged in
their internals and in some of the features presented to user-space
(although both are compatible with Comedilib). The version on github is
the original fork and doesn't use the new 'auto_attach()' handler (it
uses the older 'attach()' handler in a kind of klunky way, simulating
manual attachment). The version included within the Linux kernel
sources is the newer fork, using the 'auto_attach()' handler to
interface more cleanly with Linux's various bus driver subsystems.

Newer development tends to happen in the Linux kernel source version of
Comedi, although occasionally new features are added to github version
or ported over from the Linux kernel source version (especially bug fixes).

> After that we would still have to wait for the main linux maintainers to
> take  your newest code base in and after that we would still not see it
> in Suse or RedHat kernels for some time, right?

New features (things that are not bug fixes) added to the Linux kernel
tend not to get back-ported to the "stable" kernel series maintained by
Greh Kroah-Hartman and Sasha Levin. The distro kernel maintainers at
Suse and Redhat, etc. decide for themselves what to back-port to their
own maintained kernel series.

I suppose one advantage of the github version is that changes made apply
to all supported kernels since it lives outside the kernel sources.

Development on the github version can be done by pull requests or by
submitting patches on this mailing list.

Development on the Linux kernel source version is done by submitting
patches to at least the following:

To: de...@driverdev.osuosl.org
Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Ian Abbott <abb...@mev.co.uk>
Cc: H Hartley Sweeten <hswe...@visionengravers.com>

and optionally, but not really necessary:

Cc: linux-...@vger.kernel.org

Patches should apply cleanly to Greg's 'staging-next' or
'staging-testing' git branches on:

git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git

Patches against Linus's latest tree will usually apply fine to Greg's
"staging" tree, since there isn't very much active development within
the Comedi area of the Linux kernel sources these days.

The guidelines for submitting patches to the Linux kernel should be
followed, see:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

(I cannot overemphasize that enough.)

If you want me to cast my critical eye over your patches before
submission to the kernel mailing lists, then that is fine too.

Best regards,
Ian

> <http://www.mev.co.uk> )=-
>
> --
> You received this message because you are subscribed to the Google
> Groups "Comedi: Linux Control and Measurement Device Interface" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to comedi_list...@googlegroups.com
> <mailto:comedi_list...@googlegroups.com>.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/comedi_list/9ff402d1-84d8-45ac-8a9d-5012a1daa45fn%40googlegroups.com
> <https://groups.google.com/d/msgid/comedi_list/9ff402d1-84d8-45ac-8a9d-5012a1daa45fn%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Der Alte]

Hi Ian,

good that you wrote this:

> The version on github is
> the original fork and doesn't use the new 'auto_attach()' handler (it
> uses the older 'attach()' handler in a kind of klunky way, simulating
> manual attachment). The version included within the Linux kernel
> sources is the newer fork, using the 'auto_attach()' handler to
> interface more cleanly with Linux's various bus driver subsystems.

I thought that the ghithub stuff was more advanced than the kernel stuff because in a

Linux-Comedi-comedi-v0_7_76-619-g231e61e.tar.gz from Aug 24th from  github I found separate

adl_pci7230.c, adl_pci7432.c,  etc. instead of adl_pci7x3x.c and assumed that individual Drivers were the new concept. ??

Then I would probably have to apply my changes to the separate adl_pci7230 source again, right?

In which direction is the code flow after all? Do you collect changes from the kernel / Greg's branch or do the Kernel guys collect changes from the CoMeDI sources on github?

Or is it even more complex?

Ciao, Bernd

[Ian Abbott]

Hello Bernd,

On 14/10/2020 09:48, Der Alte wrote:
> Hi Ian,
>

> good that you wrote this:
>
> > The version on github is
> > the original fork and doesn't use the new 'auto_attach()' handler (it
> > uses the older 'attach()' handler in a kind of klunky way, simulating
> > manual attachment). The version included within the Linux kernel
> > sources is the newer fork, using the 'auto_attach()' handler to
> > interface more cleanly with Linux's various bus driver subsystems.
>
> I thought that the ghithub stuff was more advanced than the kernel
> stuff because in a Linux-Comedi-comedi-v0_7_76-619-g231e61e.tar.gz
> from Aug 24th from github I found separate adl_pci7230.c,
> adl_pci7432.c, etc. instead of adl_pci7x3x.c and assumed that
> individual Drivers were the new concept. ??

It's just that the code bases have diverged a lot. There was a push
from Greg Kroah-Hartman to get as much external driver stuff into
"staging" as possible, so Comedi ended up in the staging area of the
2.6.29 kernel where it could be worked on. This involved stripping out
things that the mainline kernel maintainers didn't care for, such as
support for real-time extensions (RTAI and RTLinux), lots of coding
style changes, and making things fit into the Linux driver model better.
There was a huge amount of activity on the Comedi sources in the Linux
kernel between 2012 and 2016, mostly by H Hartley Sweeten. So much so,
that Hartley was consistently making it to the list of top contributors
to the Linux kernel by number of patches, and the vast majority of those
patches were Comedi-related! There was no way all that stuff was going
to get back-ported to the original Comedi code base (the stuff on GitHub).

> Then I would probably have to apply my changes to the separate
> adl_pci7230 source again, right?

If you want the functionality in both code bases, then yes. But I could
port the changes over to the github version for you if they are
reasonably self-contained.

> In which direction is the code flow after all? Do you collect changes
> from the kernel / Greg's branch or do the Kernel guys collect changes
> from the CoMeDI sources on github?
> Or is it even more complex?

The kernel guys don't collect anything from the comedi sources on
github, but some things do get ported over in both directions,
especially bug fixes. Occasionally, someone will add some functionality
to a driver on either Github or in the Linux sources, and if it's easy
to do and backwards compatible with the old stuff, I'll port it over.
(There have been some backwards incompatible changes to some drivers on
the Linux side, mostly those that should never have been added to Comedi
in their original state in the first place due to breaking all the
Comedi interface rules (especially the ADDI-DATA drivers). Very
occasionally, some larger change gets ported over. For example, earlier
this month I added support for the COMEDI_SETRSUBD and COMEDI_SETWSUBD
ioctls that have been in the Linux sources since kernel 3.19.

Best regards,
Ian

[Der Alte]

Hi Ian,

> I can certainly review Comedi patches for the Linux kernel as I am one
> of the maintainers,

how would you like me to transfer the diffs to you for a review and comments?

The unified diffs are not so small as they  contain some

compile-time switchable,  fast *print macros.

-rw-r--r-- 1 harries users 16953 14. Okt 12:33 adl_pci7x3x.c.diff

-rw-r--r-- 1 harries users 21088 14. Okt 12:33 adv_pci_dio.c.diff

Maybe posting them here could cause some confusion as they may not be

final enough for you (or Greg) . But they work - even if I torture them a bit ;-) 

I could also send the files to your email address.

Ciao, Bernd

Ian Abbott schrieb am Dienstag, 13. Oktober 2020 um 12:47:04 UTC+2:

[Der Alte]

Hi Folks,

the diffs won't get so much smaller if I remove the trace prints

-rw-r--r-- 1 harries users 14418 15. Okt 11:50 adl_pci7x3x.c_sil.udiff
-rw-r--r-- 1 harries users 17778 15. Okt 11:50 adv_pci_dio.c_sil.udiff

but much less interresting and follow-able if one has the real hardware

and wants to see how it works.

Does anyone of you have an Advantech PCI 1730 or an Adlink PCI 7230  card or even one the other cards that are driven by one of the 2 family-drivers? Then it would really make sense to post the diffs here and someone could test them in real live and give some more feedback.

Ciao, Bernd

[Ian Abbott]

Hi Bernd,

On 15/10/2020 10:14, Der Alte wrote:
> Hi Ian,
>

> > I can certainly review Comedi patches for the Linux kernel as I am one
> > of the maintainers,
>
> how would you like me to transfer the diffs to you for a review and
> comments?
> The unified diffs are not so small as they  contain some
> compile-time switchable,  fast *print macros.
> -rw-r--r-- 1 harries users 16953 14. Okt 12:33 adl_pci7x3x.c.diff
> -rw-r--r-- 1 harries users 21088 14. Okt 12:33 adv_pci_dio.c.diff
>
> Maybe posting them here could cause some confusion as they may not be
> final enough for you (or Greg) . But they work - even if I torture them
> a bit ;-)

To be honest, most if not all of the low-level tracing stuff (anything
that would be produced during normal operation) is likely to get
stripped out before it ends up in the kernel sources.

> I could also send the files to your email address.

I don't mind which. If you send them privately, I'll respond privately,
but if you send them publicly, I'll respond publicly.

> Ciao, Bernd

Also bear in mind that "Der Alte" would be unacceptable for the
"Signed-off-by:" lines in the patches sent to Greg. Real names and
email addresses only!

[Der Alte]

Hi all,

ok. This is the verbose u-diff for the smaller one of the 2 drivers

/usr/src/linux-4.12.14-lp151.28.67/drivers/staging/comedi/drivers/adl_pci_7x3x.c

for commenting. It was taken from openSuse leap 15.1. It also fits for leap15.2 ( 5.3.18 kernel) with some lines offset.

I already have silent versions and diffs but perhaps some folks here have the HW and can test it.

The larger adv_pci_dio.c is modified in the same style and I can also post it here if desired.

Hope it helps and have fun, Bernd Harries

BtW: my name may already / still be in the Linux source tree through the atyfb driver for ATI-Mach64 etc.

graphics cards in Atari and PPC boxes I worked on with Geert Uytterhoeven back in the "last millenium". :-)

--- /usr/src/linux/drivers/staging/comedi/drivers/adl_pci7x3x.c    2020-09-04 17:23:21.000000000 +0200
+++ ./adl_pci7x3x.c    2020-10-14 12:30:06.537000000 +0200
@@ -56,13 +56,54 @@
 
 #include "../comedi_pci.h"
 
+#include "plx9052.h"
+
+
 /*
  * Register I/O map (32-bit access only)
  */
-#define PCI7X3X_DIO_REG        0x00
-#define PCI743X_DIO_REG        0x04
+#define PCI7X3X_DIO_REG        0x0000    /* im DigIO Port Bereich */
+#define PCI743X_DIO_REG        0x0004
+
+#define ADL_PT_CLRIRQ           0x0040    /* im DigIO Port Bereich */
+
+#define ADL_OP_LINTCSR          0x004C    /* im LCR Bereich */
+#define ADL_OP_CNTRL            0x0050    /* im LCR Bereich */
+#define ADL_OP_EEPROM           0x0053    /* im LCR Bereich */
+
+#define LINTi1_EN_HI (PLX9052_INTCSR_LI1ENAB | PLX9052_INTCSR_LI1POL)
+#define LINTi2_EN_HI (PLX9052_INTCSR_LI2ENAB | PLX9052_INTCSR_LI2POL)
+
+#define LINTi1_EN_ACT_IDI0 (PLX9052_INTCSR_LI1ENAB | PLX9052_INTCSR_LI1STAT)
+#define LINTi2_EN_ACT_IDI1 (PLX9052_INTCSR_LI2ENAB | PLX9052_INTCSR_LI2STAT)
+
+#define EN_PCI_LINT2EH_LINT1EH (PLX9052_INTCSR_PCIENAB | LINTi2_EN_HI | LINTi1_EN_HI)
+#define EN_PCI_LINT2H_LINT1H (PLX9052_INTCSR_PCIENAB | PLX9052_INTCSR_LI2POL | PLX9052_INTCSR_LI1POL)
+
 
-enum apci1516_boardid {
+#define  FuncId         if( adl_pci7x3x_debug &  0x00000100) printk
+#define  FuncDone       if( adl_pci7x3x_debug &  0x00000200) printk
+#define  CFuncId        if( adl_pci7x3x_debug &  0x00000400) printk
+#define  CFuncDone      if( adl_pci7x3x_debug &  0x00000800) printk
+#define  Pprintf        if( adl_pci7x3x_debug &  0x00100000) printk
+#define  BHAprintf      if( adl_pci7x3x_debug &  0x00000020) printk
+#define  BHMprintf      if( adl_pci7x3x_debug &  0x00000010) printk
+
+#define  ISRprintf      if( adl_pci7x3x_debug &  0x80000000) printk
+
+#define  Dprintf        if( adl_pci7x3x_debug &  0x00800000) printk
+
+#define  EDprintf       if((adl_pci7x3x_debug &  0x00808000) == 0x00808000) printk
+
+#define  Mprintf        if( adl_pci7x3x_debug &  0x00001000) printk
+#define  Iprintf        if( adl_pci7x3x_debug &  0x00002000) printk
+#define  Wprintf        if( adl_pci7x3x_debug &  0x00008000) printk
+#define  Eprintf        if( adl_pci7x3x_debug &  0x00010000) printk
+#define  Fprintf        if( adl_pci7x3x_debug &  0x00020000) printk
+
+#define DATETIME        "2020_10_14_12:25 "
+
+enum adl_pci7x3x_boardid {
     BOARD_PCI7230,
     BOARD_PCI7233,
     BOARD_PCI7234,
@@ -76,14 +117,16 @@
     int nsubdevs;
     int di_nchan;
     int do_nchan;
+    int irq_nchan;
 };
 
 static const struct adl_pci7x3x_boardinfo adl_pci7x3x_boards[] = {
     [BOARD_PCI7230] = {
         .name        = "adl_pci7230",
-        .nsubdevs    = 2,
+        .nsubdevs    = 4,  /* IDI, IDO, IRQ_IDI0, IRQ_IDI1 */
         .di_nchan    = 16,
         .do_nchan    = 16,
+        .irq_nchan    = 2,
     },
     [BOARD_PCI7233] = {
         .name        = "adl_pci7233",
@@ -113,13 +156,129 @@
     }
 };
 
+
+static u32                      adl_pci7x3x_debug = 0x80838300;
+
+struct adl_pci7x3x_dev_private_data
+{
+    unsigned long lcr_io_base;
+    int boardtype;
+    int dio;
+    short int int_ctrl;
+
+};
+
+
+struct adl_pci7x3x_sd_private_data
+{
+    unsigned long port_offset;
+    short int sd_idx;
+    short int cmd_running;
+
+};
+
+
+static irqreturn_t adl_pci7x3x_interrupt(int irq, void *p_device)
+{
+    struct comedi_device * const dev = p_device;
+    struct adl_pci7x3x_dev_private_data * const dev_private = dev->private;
+
+    //struct comedi_subdevice *s = dev->read_subdev;
+
+    char *   pin_name = NULL;
+
+    unsigned long cpu_flags;
+    unsigned short intcsr;
+    //unsigned short clr_reg;
+    unsigned int subdev = 2;
+
+  CFuncId("adl_pci7x3x_isr() nsd=%d ", dev->n_subdevices);
+
+    if (!dev->attached)
+    {
+      /*  Ignore interrupt before device fully attached. */
+      /*  Might not even have allocated subdevices yet! */
+      return IRQ_NONE;
+    }
+
+    if(adl_pci7x3x_boards[dev_private->boardtype].di_nchan == 0)
+    {   
+      /* keine Inputs => Keine IRQs. */
+      return IRQ_NONE;
+    }
+   
+    /*  Check if we are source of interrupt */
+
+    spin_lock_irqsave(&dev->spinlock, cpu_flags);
+    intcsr = inw(dev_private->lcr_io_base + ADL_OP_LINTCSR);
+    //ISRprintf(" adl IFL=%04X ", intcsr);
+    if(!(intcsr & PLX9052_INTCSR_PCIENAB))  /* 0x0040 9052 PCI IRQ active? */
+    {
+      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
+      ISRprintf(" adl IFL=%04X nix los ", intcsr);
+      return IRQ_NONE;
+    }
+
+    /*clr_reg = inb(dev->iobase + ADL_PT_CLRIRQ);
+    ISRprintf(" ADL_CLRIRQ=%04X ", clr_reg);*/
+    /* clear all current interrupt flags */
+    outb(0x00, dev->iobase + ADL_PT_CLRIRQ); /* Fixme: Reset all 2 Int Flags */
+    spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
+
+    /* SubDev 2, 3 = Isolated DigIn , auf "SCSI2" Buchse!*/
+
+    if((intcsr & LINTi1_EN_ACT_IDI0) == LINTi1_EN_ACT_IDI0)  /* 0x0005 LINTi1 is Enabled && IDI0 is 1 */
+    {
+      subdev = 2;
+      pin_name = "IDI0";
+    }
+    /*endif((intcsr & LINTi1_EN_ACT_IDI0) == LINTi1_EN_ACT_IDI0)*/
+
+    if((intcsr & LINTi2_EN_ACT_IDI1) == LINTi2_EN_ACT_IDI1)  /* 0x0028 LINTi2 is Enabled && IDI1 is  1 */
+    {
+      subdev = 3;
+      pin_name = "IDI1";
+    }
+    /*endif((intcsr & LINTi2_EN_ACT_IDI1) == LINTi2_EN_ACT_IDI1)*/
+
+
+    /*block*/
+    {
+      struct comedi_subdevice * sd_p = &dev->subdevices[subdev];
+      struct adl_pci7x3x_sd_private_data * sd_priv = sd_p->private;
+      unsigned long reg = (unsigned long)sd_priv->port_offset;
+
+      struct comedi_async *   async_p = sd_p->async;
+
+      ISRprintf(" adl IFL=%04X %s sd%u r_of=%02lX cmd=%d ", intcsr, pin_name, subdev, reg, sd_priv->cmd_running);
+      if(async_p)
+      {
+        sd_p->state = inw(dev->iobase + reg);
+        if(sd_priv->cmd_running)
+        {
+          comedi_buf_write_samples(sd_p, &sd_p->state, 1);  /* 2020_09_21: Freezes the kernel without cmd running */
+        }
+        comedi_handle_events(dev, sd_p);  /* allowed since async_p is valid */
+      }
+      /*endif(async_p)*/
+    }
+    /*endblock*/
+
+  CFuncDone(" IRQ_HANDLED.\n");
+    return IRQ_HANDLED;
+}
+/*endproc adl_pci7x3x_interrupt() */
+
+
 static int adl_pci7x3x_do_insn_bits(struct comedi_device *dev,
                     struct comedi_subdevice *s,
                     struct comedi_insn *insn,
                     unsigned int *data)
 {
-    unsigned long reg = (unsigned long)s->private;
+    struct adl_pci7x3x_sd_private_data * sd_priv = s->private;
+    unsigned long reg = (unsigned long)sd_priv->port_offset;
 
+    Dprintf(" do_insn_bits() sd_p=%8p ->async=%8p \n", s, s->async);
     if (comedi_dio_update_state(s, data)) {
         unsigned int val = s->state;
 
@@ -145,43 +304,195 @@
                     struct comedi_insn *insn,
                     unsigned int *data)
 {
-    unsigned long reg = (unsigned long)s->private;
+    struct adl_pci7x3x_sd_private_data * sd_priv = s->private;
+    unsigned long reg = (unsigned long)sd_priv->port_offset;
 
+    Dprintf(" di_insn_bits() sd_p=%8p ->async=%8p \n", s, s->async);
     data[1] = inl(dev->iobase + reg);
 
     return insn->n;
 }
 
+static int adl_pci7x3x_asy_cmdtest(struct comedi_device *dev,
+                struct comedi_subdevice *s,
+                struct comedi_cmd *cmd)
+{
+    int err = 0;
+
+    FuncId("adl_pci7x3x_asy_cmdtest() \n");
+    /* Step 1 : check if triggers are trivially valid */
+
+    err |= comedi_check_trigger_src(&cmd->start_src, TRIG_NOW);
+    err |= comedi_check_trigger_src(&cmd->scan_begin_src, TRIG_EXT);
+    err |= comedi_check_trigger_src(&cmd->convert_src, TRIG_FOLLOW);
+    err |= comedi_check_trigger_src(&cmd->scan_end_src, TRIG_COUNT);
+    err |= comedi_check_trigger_src(&cmd->stop_src, TRIG_NONE);
+
+    if (err)
+        return 1;
+
+    /* Step 2a : make sure trigger sources are unique */
+    /* Step 2b : and mutually compatible */
+
+    /* Step 3: check if arguments are trivially valid */
+
+    err |= comedi_check_trigger_arg_is(&cmd->start_arg, 0);
+    err |= comedi_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
+    err |= comedi_check_trigger_arg_is(&cmd->convert_arg, 0);
+    err |= comedi_check_trigger_arg_is(&cmd->scan_end_arg,
+                       cmd->chanlist_len);
+    err |= comedi_check_trigger_arg_is(&cmd->stop_arg, 0);
+
+    if (err)
+        return 3;
+
+    /* Step 4: fix up any arguments */
+
+    /* Step 5: check channel list if it exists */
+
+    return 0;
+}
+
+static int adl_pci7x3x_asy_cmd(struct comedi_device *dev,
+                struct comedi_subdevice *s)
+{
+    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;
+    struct adl_pci7x3x_sd_private_data * sd_priv = s->private;
+    unsigned long cpu_flags;
+
+    FuncId("adl_pci7x3x_asy_cmd() \n");
+    if(sd_priv->sd_idx == 2)
+    {
+      Dprintf(" enable IDI CH 0 IRQ... \n");
+      spin_lock_irqsave(&dev->spinlock, cpu_flags);
+      dev_private->int_ctrl |= PLX9052_INTCSR_LI1ENAB;  /* enable LINTi1 == IDI sdi[0] Ch 0 IRQ ActHigh */
+      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
+    }
+    if(sd_priv->sd_idx == 3)
+    {
+      Dprintf(" enable IDI CH 1 IRQ... \n");
+      spin_lock_irqsave(&dev->spinlock, cpu_flags);
+      dev_private->int_ctrl |= PLX9052_INTCSR_LI2ENAB;  /* enable LINTi2 == IDI sdi[0] Ch 1 IRQ ActHigh */
+      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
+    }
+    //outb(EN_PCI_LINT2EH_LINT1EH, dev_private->lcr_io_base + ADL_OP_LINTCSR);  /* 0x5B enable PCI + IDI sdi[0] Ch 1 Ch 0 IRQ On ActHigh */
+    outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);
+
+    sd_priv->cmd_running = 1;
+
+    return 0;
+}
+
+static int adl_pci7x3x_asy_cancel(struct comedi_device *dev,
+                   struct comedi_subdevice *s)
+{
+    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;
+    struct adl_pci7x3x_sd_private_data * sd_priv = s->private;
+    unsigned long cpu_flags;
+
+    FuncId("adl_pci7x3x_asy_cancel() \n");
+    sd_priv->cmd_running = 0;
+    /* disable Interrupts */
+    if(sd_priv->sd_idx == 2)
+    {
+      Dprintf(" disable IDI CH 0 IRQ... \n");
+      spin_lock_irqsave(&dev->spinlock, cpu_flags);
+      dev_private->int_ctrl &= ~PLX9052_INTCSR_LI1ENAB;  /* Disable LINTi1 */
+      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
+    }
+    if(sd_priv->sd_idx == 3)
+    {
+      Dprintf(" disable IDI CH 1 IRQ... \n");
+      spin_lock_irqsave(&dev->spinlock, cpu_flags);
+      dev_private->int_ctrl &= ~PLX9052_INTCSR_LI2ENAB;  /* Disable LINTi2 */
+      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
+    }
+    //outb(0x00, dev_private->lcr_io_base + ADL_OP_LINTCSR);  /* Disable PCI + LINTi2 + LINTi1 */
+    outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);
+
+    return 0;
+}
+
+static int adl_pci7x3x_reset(struct comedi_device *dev)
+{
+    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;
+
+    /* disable Interrupts */
+    dev_private->int_ctrl = 0x00;  /* Disable PCI + LINTi2 + LINTi1 */
+    outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);
+
+    return 0;
+}
+
+
 static int adl_pci7x3x_auto_attach(struct comedi_device *dev,
                    unsigned long context)
 {
-    struct pci_dev *pcidev = comedi_to_pci_dev(dev);
-    const struct adl_pci7x3x_boardinfo *board = NULL;
-    struct comedi_subdevice *s;
+    struct pci_dev * pcidev = comedi_to_pci_dev(dev);
+    struct adl_pci7x3x_dev_private_data * dev_private = dev->private;  /* wahrscheinlich noch 0! */
+
+    const struct adl_pci7x3x_boardinfo * board_p = NULL;
+    struct comedi_subdevice * s;
+    struct adl_pci7x3x_sd_private_data * sd_priv = NULL;
     int subdev;
+    int ic;
     int nchan;
     int ret;
+    unsigned short intcsr;
+
+    FuncId("adl_pci7x3x_auto_attach() %s \n", DATETIME);
 
     if (context < ARRAY_SIZE(adl_pci7x3x_boards))
-        board = &adl_pci7x3x_boards[context];
-    if (!board)
+        board_p = &adl_pci7x3x_boards[context];
+    if (!board_p)
         return -ENODEV;
-    dev->board_ptr = board;
-    dev->board_name = board->name;
+    dev->board_ptr = board_p;
+    dev->board_name = board_p->name;
+
+    /* BHA 2020_08: collect extra Data f. ISR ...  */
+    dev_private = comedi_alloc_devpriv(dev, sizeof(*dev_private));
+    if (!dev_private)
+    {
+      return -ENOMEM;
+    }
 
     ret = comedi_pci_enable(dev);
     if (ret)
         return ret;
     dev->iobase = pci_resource_start(pcidev, 2);
+    dev_private->lcr_io_base = pci_resource_start(pcidev, 1);  /* oder 0? */
+    dev_private->boardtype = context;
+
+    adl_pci7x3x_reset(dev);
 
-    ret = comedi_alloc_subdevices(dev, board->nsubdevs);
+
+    Dprintf(" ISR anmelden...\n");
+
+    /* BHA 2020_08: discard all evtl. old  IRQs  */
+    outb(0x00, dev->iobase + ADL_PT_CLRIRQ); /* Fixme: Reset all 2 Int Flags */
+   
+    if (pcidev->irq)
+    {
+      ret = request_irq(pcidev->irq, adl_pci7x3x_interrupt, IRQF_SHARED, dev->board_name, dev);
+      if (ret == 0)
+      {
+        dev->irq = pcidev->irq;  /* muss gesetzt werden, bevor es abgefragt wird! */
+        Dprintf(" prep IDI CH 0 HI + CH 1 HI IRQs... ");
+        dev_private->int_ctrl = EN_PCI_LINT2H_LINT1H;  /* 0x52 enable PCI + IDI sdi[0] Ch 1 Ch 0 IRQ Off ActHigh */
+        outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);
+        intcsr = inw(dev_private->lcr_io_base + ADL_OP_LINTCSR);
+        Dprintf(" intcsr=%04X \n", intcsr);
+      }
+    }
+   
+    ret = comedi_alloc_subdevices(dev, board_p->nsubdevs);
     if (ret)
         return ret;
 
     subdev = 0;
 
-    if (board->di_nchan) {
-        nchan = min(board->di_nchan, 32);
+    if (board_p->di_nchan) {
+        nchan = min(board_p->di_nchan, 32);
 
         s = &dev->subdevices[subdev];
         /* Isolated digital inputs 0 to 15/31 */
@@ -192,11 +503,19 @@
         s->insn_bits    = adl_pci7x3x_di_insn_bits;
         s->range_table    = &range_digital;
 
-        s->private    = (void *)PCI7X3X_DIO_REG;
+        sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
+        if(!sd_priv)
+        {
+          return -ENOMEM;
+        }
+        /*endif(!sd_priv)*/
+        sd_priv->port_offset = PCI7X3X_DIO_REG;
+        sd_priv->sd_idx = subdev;
+        sd_priv->cmd_running = 0;
 
         subdev++;
 
-        nchan = board->di_nchan - nchan;
+        nchan = board_p->di_nchan - nchan;
         if (nchan) {
             s = &dev->subdevices[subdev];
             /* Isolated digital inputs 32 to 63 */
@@ -207,14 +526,22 @@
             s->insn_bits    = adl_pci7x3x_di_insn_bits;
             s->range_table    = &range_digital;
 
-            s->private    = (void *)PCI743X_DIO_REG;
+            sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
+            if(!sd_priv)
+            {
+              return -ENOMEM;
+            }
+            /*endif(!sd_priv)*/
+            sd_priv->port_offset = PCI7X3X_DIO_REG;
+            sd_priv->sd_idx = subdev;
+            sd_priv->cmd_running = 0;
 
             subdev++;
         }
     }
 
-    if (board->do_nchan) {
-        nchan = min(board->do_nchan, 32);
+    if (board_p->do_nchan) {
+        nchan = min(board_p->do_nchan, 32);
 
         s = &dev->subdevices[subdev];
         /* Isolated digital outputs 0 to 15/31 */
@@ -225,11 +552,19 @@
         s->insn_bits    = adl_pci7x3x_do_insn_bits;
         s->range_table    = &range_digital;
 
-        s->private    = (void *)PCI7X3X_DIO_REG;
+        sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
+        if(!sd_priv)
+        {
+          return -ENOMEM;
+        }
+        /*endif(!sd_priv)*/
+        sd_priv->port_offset = PCI7X3X_DIO_REG;
+        sd_priv->sd_idx = subdev;
+        sd_priv->cmd_running = 0;
 
         subdev++;
 
-        nchan = board->do_nchan - nchan;
+        nchan = board_p->do_nchan - nchan;
         if (nchan) {
             s = &dev->subdevices[subdev];
             /* Isolated digital outputs 32 to 63 */
@@ -240,25 +575,90 @@
             s->insn_bits    = adl_pci7x3x_do_insn_bits;
             s->range_table    = &range_digital;
 
-            s->private    = (void *)PCI743X_DIO_REG;
+            sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
+            if(!sd_priv)
+            {
+              return -ENOMEM;
+            }
+            /*endif(!sd_priv)*/
+            sd_priv->port_offset = PCI7X3X_DIO_REG;
+            sd_priv->sd_idx = subdev;
+            sd_priv->cmd_running = 0;
 
             subdev++;
         }
     }
 
+    for (ic = 0; ic < board_p->irq_nchan; ++ic) {
+        nchan = 1;
+
+        s = &dev->subdevices[subdev];
+        /* Isolated digital inputs 0  or 1 */
+        s->type        = COMEDI_SUBD_DI;
+        s->subdev_flags    = SDF_READABLE;
+        s->n_chan    = nchan;
+        s->maxdata    = 1;
+        s->insn_bits    = adl_pci7x3x_di_insn_bits;
+        s->range_table    = &range_digital;
+
+        sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
+        if(!sd_priv)
+        {
+          return -ENOMEM;
+        }
+        /*endif(!sd_priv)*/
+        sd_priv->port_offset = PCI7X3X_DIO_REG;
+        sd_priv->sd_idx = subdev;
+        sd_priv->cmd_running = 0;
+
+        if (dev->irq) {
+            dev->read_subdev = s;
+            s->type        = COMEDI_SUBD_DI;
+            s->subdev_flags    = SDF_READABLE | SDF_CMD_READ;
+            s->len_chanlist    = 1;
+            s->do_cmdtest    = adl_pci7x3x_asy_cmdtest;
+            s->do_cmd    = adl_pci7x3x_asy_cmd;  /* causes ->async creation */
+            s->cancel    = adl_pci7x3x_asy_cancel;
+        }
+
+        subdev++;
+    }
+    /*next ic*/
+
     return 0;
 }
+/*endproc adl_pci7x3x_auto_attach() */
+
+
+static void adl_pci7x3x_detach(struct comedi_device *dev)
+{
+    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;
+    int boardtype = dev_private->boardtype;
+
+    if (dev->iobase)  adl_pci7x3x_reset(dev);
+    if (dev->irq)
+    {
+      Dprintf(" adl_7x3x ISR $%04X abmelden...\n", boardtype);
+      free_irq(dev->irq, dev);
+      dev->irq = 0;
+    }
+
+    //comedi_free_devpriv(dev);
+    comedi_pci_detach(dev);
+}
+
 
 static struct comedi_driver adl_pci7x3x_driver = {
     .driver_name    = "adl_pci7x3x",
     .module        = THIS_MODULE,
     .auto_attach    = adl_pci7x3x_auto_attach,
-    .detach        = comedi_pci_detach,
+    .detach        = adl_pci7x3x_detach,
 };
 
 static int adl_pci7x3x_pci_probe(struct pci_dev *dev,
                  const struct pci_device_id *id)
 {
+    FuncId("adl_pci7x3x_pci_probe() %s \n", DATETIME);
     return comedi_pci_auto_config(dev, &adl_pci7x3x_driver,
                       id->driver_data);
 }

[Der Alte]

Hi Folks,

here is the ext_trig.c source which is helpful to test the new Dig-IN features and learn how to take

advantage in fast reactions on external triggers. E.G. clocks with PPS (Pulse Per Second) output.

I first connected a simple button to the special input pins of the Dig-Input ports but that one was bouncing ugly.

Then I used a small Arduino Board (Digispark) to simulate a PPS  signal with just 20 usec duty cycle every second on P0 plus the inverted signal on a P2. And a 50 %  duty cycle rectangle  on the LED pin P1.

For the isolated inputs of the Advantech PCI 1730 the 20 us pulse with just 5 Volts seems

to be beyond the limits.

The Code of ext_trig.c was mainly taken from ledclock.c

Ciao, Bernd

/*

 * ext_trig demo
 * Part of Comedilib
 *
 * Copyright (c) 2001 David A. Schleef
 * Copyright (c) 2020 Bernd Harries
 *
 * This file may be freely modified, distributed, and combined with
 * other software, as long as proper attribution is given in the
 * source code.
 */

/*
 * Requirements:
 *    - A board with a subdevice that
 *      can trigger on an external digital line.
 *      A parallel port satisfies these requirements.
 *      Adlink PCI 7230 with adequate driver, too.
 *      Advantech PCI 1730  with adequate driver, too.
 *
 * The program sets up a comedi command to wait for an external trigger.
 * In a loop waits for the input subdevice in select() with
 * a long timeout. If select returns with the file descrptor
 * ready for read, the available input data is read from the
 * file descrptor and getimeofday() is called. The absolute
 * and the delta time since te last trigger event is printed as
 * well as the first two 16 bit samples possibly in the read-buffer.
 *
 * 2 instances per card can run at the same time if the you have
 * Adlink PCI 7230 card(s) installed.
 * 4 instances per card can run at the same time if the you have
 * Advantech PCI 1730 card(s) installed.
 * 1 instances per parport can run at the same time if the you have
 * parallel ports istalled.
 *
 * 5 parallel instances have successfully been used in a constellation
 * with 1 PCIe parport, 1 LPCIe 7230 and 1 PCI 1730 card.
 *
 */

#define _GNU_SOURCE

#include <stdio.h>
#include <comedilib.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <getopt.h>
#include <ctype.h>
#include <signal.h>
#include <string.h>
#include <sys/time.h>
#include "examples.h"


comedi_t *device;

int count;

int out_subd;

#define BUFSZ 1024
sampl_t buf[BUFSZ];

unsigned int chanlist[16];


void prepare_cmd(comedi_t *dev,comedi_cmd *cmd, int subdevice);
void do_cmd(comedi_t *dev,comedi_cmd *cmd);


int main(int argc, char *argv[])
{
    int ret;
    comedi_cmd cmd;
    struct parsed_options options;

    init_parsed_options(&options);
    parse_options(&options, argc, argv);

  fprintf(stderr, "\n now comedi_open(%s ) ... ", options.filename);
    device = comedi_open(options.filename);
    if(!device){
        perror(options.filename);
        exit(1);
    }

    ret = fcntl(comedi_fileno(device),F_SETFL,O_NONBLOCK|O_ASYNC);
    if(ret<0)perror("fcntl");

#if 0
    {
    struct sched_param p;

    memset(&p,0,sizeof(p));
    p.sched_priority = 1;
    ret = sched_setscheduler(0,SCHED_FIFO,&p);
    if(ret<0)perror("sched_setscheduler");
    }
#endif

  fprintf(stderr, "\n now prepare_cmd(sd = %d ) ... ", options.subdevice);
    prepare_cmd(device, &cmd, options.subdevice);

  fprintf(stderr, "\n now do_cmd(dev, &cmd ) ... ");
    do_cmd(device, &cmd);

    return 0;
}

void do_cmd(comedi_t *dev,comedi_cmd *cmd)
{
    int total = 0;
    int ret;
    int go;
    fd_set rdset;
    struct timeval timeout;
    struct timeval tvs_last = {0, 0};
    struct timeval tvs_now = {0, 0};;
    struct timeval tvs_delta = {0, 0};;

    ret = comedi_command_test(dev, cmd);

    fprintf(stderr, "\ncmd-test ret=%d ", ret);
    if(ret < 0){
        fprintf(stderr, "\nerrno=%d", errno);
        comedi_perror("comedi_command_test()");
        return;
    }

    dump_cmd(stderr, cmd);

    ret = comedi_command_test(dev, cmd);

    fprintf(stderr, "\ncmd-test ret=%d ", ret);
    if(ret < 0){
        fprintf(stderr, "\nerrno=%d", errno);
        comedi_perror("comedi_command_test()");
        return;
    }

    dump_cmd(stderr, cmd);

    comedi_set_read_subdevice(dev, cmd->subdev);
    ret = comedi_get_read_subdevice(dev);
    if (ret < 0 || ret != cmd->subdev) {
        fprintf(stderr,
            "failed to change 'read' subdevice from %d to %d",
            ret, cmd->subdev);
        return;
    }

  fprintf(stderr, "\n now comedi_command(dev, &cmd ) ... ");
    ret=comedi_command(dev, cmd);

    fprintf(stderr, "\nret=%d", ret);
    if(ret < 0)
    {
        fprintf(stderr, "\nerrno=%d", errno);
        comedi_perror("comedi_command");
        return;
    }

    ret = gettimeofday(&tvs_last, NULL);
    go = 1;
    while(go)
    {
        FD_ZERO(&rdset);
        FD_SET(comedi_fileno(dev), &rdset);
        timeout.tv_sec = 16;
        timeout.tv_usec = 50000;
  fprintf(stderr, "\n now select(comedi_fileno(dev) + 1, &rdset, NULL, NULL, &(%02ld.%06ld)) ... ",
    timeout.tv_sec, timeout.tv_usec);
        ret = select(comedi_fileno(dev) + 1, &rdset, NULL, NULL, &timeout);
        if(ret < 0)
        {
            perror("select");
        }else if(ret == 0)
        {
            /* timeout */
  fprintf(stderr, "TiO ");
        }
        else if(FD_ISSET(comedi_fileno(dev), &rdset))
        {
            ret=read(comedi_fileno(dev), buf, BUFSZ);
            if(ret < 0)
            {
                if(errno == EAGAIN){
                    go = 0;
                    perror("read");
                }
            }else if(ret == 0)  // no Data from read()
            {
                go = 0;
            }else{
                //int i;

  //fprintf(stderr, "\n now gettimeofday() ... ");
                ret = gettimeofday(&tvs_now, NULL);

                total += ret;
                //fprintf(stderr, "\nread %d %d", ret, total);
                //fprintf(stderr, "\ncount = %d", count);

                tvs_delta.tv_usec = tvs_now.tv_usec - tvs_last.tv_usec;
                tvs_delta.tv_sec = tvs_now.tv_sec - tvs_last.tv_sec;
                if(tvs_delta.tv_usec < 0)
                {
                  tvs_delta.tv_usec += 1000000;
                  tvs_delta.tv_sec -= 1;
                }
                //endif(tvs_delta.tv_usec < 0)
                tvs_last = tvs_now;
               
  fprintf(stderr, "\n t=%12ld.%06ld s dt=%4ld.%06ld s  buf=%04hX %04hX ",
    tvs_now.tv_sec, tvs_now.tv_usec,
    tvs_delta.tv_sec, tvs_delta.tv_usec,
    buf[0], buf[1]);

            }
            //endif(read_ret == 0)
        }
    }
}
//endproc do_cmd()

/*
 * prepare a comedi command to configure a subdevice for
 * external triggering.
 */
void prepare_cmd(comedi_t *dev, comedi_cmd *cmd, int subdevice)
{
    memset(cmd, 0, sizeof(*cmd));

    /* the subdevice that the command is sent to */
    cmd->subdev = subdevice;

    /* flags */
    cmd->flags =        TRIG_WAKE_EOS;

    cmd->start_src =    TRIG_NOW;
    cmd->start_arg =    0;

    cmd->scan_begin_src =    TRIG_EXT;
    cmd->scan_begin_arg =    0;

#if 0
    cmd->convert_src =    TRIG_TIMER;
    cmd->convert_arg =    1;
#else
    cmd->convert_src =    TRIG_ANY;
    cmd->convert_arg =    0;
#endif

    cmd->scan_end_src =    TRIG_COUNT;
    cmd->scan_end_arg =    1;

    cmd->stop_src =        TRIG_NONE;
    cmd->stop_arg =        0;

    cmd->chanlist =        chanlist;
    cmd->chanlist_len =    1;

    chanlist[0]=CR_PACK(0, 0, 0);
}
//endproc prepare_cmd()

[Ian Abbott]

Hello Bernd,

On 15/10/2020 16:03, Der Alte wrote:
> Hi all,
>
> ok. This is the verbose u-diff for the smaller one of the 2 drivers
> /usr/src/linux-4.12.14-lp151.28.67/drivers/staging/comedi/drivers/adl_pci_7x3x.c
>
> for commenting. It was taken from openSuse leap 15.1. It also fits for
> leap15.2 ( 5.3.18 kernel) with some lines offset.
>
> I already have silent versions and diffs but perhaps some folks here
> have the HW and can test it.
> The larger adv_pci_dio.c is modified in the same style and I can also
> post it here if desired.
>
> Hope it helps and have fun, Bernd Harries
>
> BtW: my name may already / still be in the Linux source tree through the
> atyfb driver for ATI-Mach64 etc.
> graphics cards in Atari and PPC boxes I worked on with Geert
> Uytterhoeven back in the "last millenium". :-)

You've been around, then. :)

Most email clients screw up patches posted inline (rather than as an
attachment), so I can't actually use the patch as posted, but at least I
can review its contents.

>
> --- /usr/src/linux/drivers/staging/comedi/drivers/adl_pci7x3x.c
> 2020-09-04 17:23:21.000000000 +0200
> +++ ./adl_pci7x3x.c    2020-10-14 12:30:06.537000000 +0200
> @@ -56,13 +56,54 @@
>
>  #include "../comedi_pci.h"
>
> +#include "plx9052.h"
> +
> +
>  /*
>   * Register I/O map (32-bit access only)
>   */
> -#define PCI7X3X_DIO_REG        0x00
> -#define PCI743X_DIO_REG        0x04
> +#define PCI7X3X_DIO_REG        0x0000    /* im DigIO Port Bereich */
> +#define PCI743X_DIO_REG        0x0004
> +
> +#define ADL_PT_CLRIRQ           0x0040    /* im DigIO Port Bereich */
> +
> +#define ADL_OP_LINTCSR          0x004C    /* im LCR Bereich */
> +#define ADL_OP_CNTRL            0x0050    /* im LCR Bereich */
> +#define ADL_OP_EEPROM           0x0053    /* im LCR Bereich */

You could use the existing PLX9052_INTCSR and PLX9052_CNTRL from
"plx9052.h" instead of ADL_OP_LINTCSR and ADL_OP_CNTRL.

Also, best to stick to English in the kernel sources. I'm not just
saying that because I happen to be English. :) You can think of it as
the official language of the Linux kernel development process.

> +
> +#define LINTi1_EN_HI (PLX9052_INTCSR_LI1ENAB | PLX9052_INTCSR_LI1POL)
> +#define LINTi2_EN_HI (PLX9052_INTCSR_LI2ENAB | PLX9052_INTCSR_LI2POL)
> +
> +#define LINTi1_EN_ACT_IDI0 (PLX9052_INTCSR_LI1ENAB |
> PLX9052_INTCSR_LI1STAT)
> +#define LINTi2_EN_ACT_IDI1 (PLX9052_INTCSR_LI2ENAB |
> PLX9052_INTCSR_LI2STAT)
> +
> +#define EN_PCI_LINT2EH_LINT1EH (PLX9052_INTCSR_PCIENAB | LINTi2_EN_HI |
> LINTi1_EN_HI)
> +#define EN_PCI_LINT2H_LINT1H (PLX9052_INTCSR_PCIENAB |
> PLX9052_INTCSR_LI2POL | PLX9052_INTCSR_LI1POL)
> +
>
> -enum apci1516_boardid {

Well done for spotting the cut-n-paste error in that enum type!

> +#define  FuncId         if( adl_pci7x3x_debug &  0x00000100) printk
> +#define  FuncDone       if( adl_pci7x3x_debug &  0x00000200) printk
> +#define  CFuncId        if( adl_pci7x3x_debug &  0x00000400) printk
> +#define  CFuncDone      if( adl_pci7x3x_debug &  0x00000800) printk
> +#define  Pprintf        if( adl_pci7x3x_debug &  0x00100000) printk
> +#define  BHAprintf      if( adl_pci7x3x_debug &  0x00000020) printk
> +#define  BHMprintf      if( adl_pci7x3x_debug &  0x00000010) printk
> +
> +#define  ISRprintf      if( adl_pci7x3x_debug &  0x80000000) printk
> +
> +#define  Dprintf        if( adl_pci7x3x_debug &  0x00800000) printk
> +
> +#define  EDprintf       if((adl_pci7x3x_debug &  0x00808000) ==
> 0x00808000) printk
> +
> +#define  Mprintf        if( adl_pci7x3x_debug &  0x00001000) printk
> +#define  Iprintf        if( adl_pci7x3x_debug &  0x00002000) printk
> +#define  Wprintf        if( adl_pci7x3x_debug &  0x00008000) printk
> +#define  Eprintf        if( adl_pci7x3x_debug &  0x00010000) printk
> +#define  Fprintf        if( adl_pci7x3x_debug &  0x00020000) printk
> +
> +#define DATETIME        "2020_10_14_12:25 "
> +

All that will be gone by the time it gets merged upstream.

> +enum adl_pci7x3x_boardid {
>      BOARD_PCI7230,
>      BOARD_PCI7233,
>      BOARD_PCI7234,
> @@ -76,14 +117,16 @@
>      int nsubdevs;
>      int di_nchan;
>      int do_nchan;
> +    int irq_nchan;

I was thinking the name 'irq_nchan' doesn't really match the usage of
the others since it spans more than one subdevice, but in fact it does
match because 'do_nchan' can also span more than one subdevice. No problem.

>  };

>
>  static const struct adl_pci7x3x_boardinfo adl_pci7x3x_boards[] = {
>      [BOARD_PCI7230] = {
>          .name        = "adl_pci7230",
> -        .nsubdevs    = 2,
> +        .nsubdevs    = 4,  /* IDI, IDO, IRQ_IDI0, IRQ_IDI1 */
>          .di_nchan    = 16,
>          .do_nchan    = 16,
> +        .irq_nchan    = 2,
>      },
>      [BOARD_PCI7233] = {
>          .name        = "adl_pci7233",
> @@ -113,13 +156,129 @@
>      }
>  };
>
> +
> +static u32                      adl_pci7x3x_debug = 0x80838300;
> +
> +struct adl_pci7x3x_dev_private_data
> +{
> +    unsigned long lcr_io_base;
> +    int boardtype;

The boardtype member seems redundant. More about that further down....

> +    int dio;

That one doesn't seem to be used anywhere.

> +    short int int_ctrl;

Better to use 'unsigned short' so it matches the intcsr variable used
elsewhere in the driver.

> +
> +};
> +
> +
> +struct adl_pci7x3x_sd_private_data

This private data is specific to the newly added 'interrupt' subdevices,
so it would be nice if the name reflected that, but it doesn't really
matter.

> +{
> +    unsigned long port_offset;
> +    short int sd_idx;

The 'sd_idx' member is redundant because 's->index' can be used instead.

The usual way to get at the static board data associated with the device
is to use a pointer variable initialized to 'dev->board_ptr', e.g.:

const struct adl_pci7x3x_boardinfo * const board = dev->board_ptr;

That will point to the specific element of adl_pci7x3x_boards[].

This 'if' statement is unnecessary, because there is no point setting up
an interrupt handler for boards that can't support it.

It is possible for both interrupt sources to be active here. Both
should be handled. The easiest way to do that would be to put the block
below into a function and call it for each of the active interrupt sources.

> +
> +
> +    /*block*/
> +    {
> +      struct comedi_subdevice * sd_p = &dev->subdevices[subdev];
> +      struct adl_pci7x3x_sd_private_data * sd_priv = sd_p->private;
> +      unsigned long reg = (unsigned long)sd_priv->port_offset;
> +
> +      struct comedi_async *   async_p = sd_p->async;
> +
> +      ISRprintf(" adl IFL=%04X %s sd%u r_of=%02lX cmd=%d ", intcsr,
> pin_name, subdev, reg, sd_priv->cmd_running);
> +      if(async_p)
> +      {
> +        sd_p->state = inw(dev->iobase + reg);
> +        if(sd_priv->cmd_running)
> +        {
> +          comedi_buf_write_samples(sd_p, &sd_p->state, 1);  /*
> 2020_09_21: Freezes the kernel without cmd running */
> +        }

It would be safer to use a spin-lock around that whole
'if(sd_priv->cmd_running) { ... }' statement (but release the spin-lock
before calling comedi_handle_events() below).

Does the 'adl_pci7x3x_do_insn_bits' function really need to change?
Different subdevices do not need to use 's->private' in the same way.

Those two spin-locked sections could be combined into a single
spin-locked section. The 'if' statements just need to store the
appropriate value (either PLX9052_INTCSR_LI1ENAB or
PLX9052_INTCSR_LI2ENAB) in a local variable so that
'dev_private->int_ctrl' can then be updated within the common
spin-locked section.

> +    //outb(EN_PCI_LINT2EH_LINT1EH, dev_private->lcr_io_base +
> ADL_OP_LINTCSR);  /* 0x5B enable PCI + IDI sdi[0] Ch 1 Ch 0 IRQ On
> ActHigh */
> +    outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);

It would be safer to put that 'outb()' call within the spin-locked
section too.

> +
> +    sd_priv->cmd_running = 1;

I would also suggest setting 'sd_priv->cmd_running' within a spin-locked
section. You could either use 'dev->spinlock' or create a spin-lock in
the subdevice private data and use that, whichever is easiest (but use
the same spin-lock in the interrupt handler!).

> +
> +    return 0;
> +}
> +
> +static int adl_pci7x3x_asy_cancel(struct comedi_device *dev,
> +                   struct comedi_subdevice *s)
> +{
> +    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;
> +    struct adl_pci7x3x_sd_private_data * sd_priv = s->private;
> +    unsigned long cpu_flags;
> +
> +    FuncId("adl_pci7x3x_asy_cancel() \n");
> +    sd_priv->cmd_running = 0;

As above, I suggest using a spin-lock for 'sd_priv->cmd_running'.

> +    /* disable Interrupts */
> +    if(sd_priv->sd_idx == 2)
> +    {
> +      Dprintf(" disable IDI CH 0 IRQ... \n");
> +      spin_lock_irqsave(&dev->spinlock, cpu_flags);
> +      dev_private->int_ctrl &= ~PLX9052_INTCSR_LI1ENAB;  /* Disable
> LINTi1 */
> +      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
> +    }
> +    if(sd_priv->sd_idx == 3)
> +    {
> +      Dprintf(" disable IDI CH 1 IRQ... \n");
> +      spin_lock_irqsave(&dev->spinlock, cpu_flags);
> +      dev_private->int_ctrl &= ~PLX9052_INTCSR_LI2ENAB;  /* Disable
> LINTi2 */
> +      spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
> +    }

As above, those can use a common spin-locked section.

> +    //outb(0x00, dev_private->lcr_io_base + ADL_OP_LINTCSR);  /*
> Disable PCI + LINTi2 + LINTi1 */
> +    outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);

And that 'outb()' call can go in the spin-locked section.

> +
> +    return 0;
> +}
> +
> +static int adl_pci7x3x_reset(struct comedi_device *dev)
> +{
> +    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;
> +
> +    /* disable Interrupts */
> +    dev_private->int_ctrl = 0x00;  /* Disable PCI + LINTi2 + LINTi1 */
> +    outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);
> +
> +    return 0;
> +}

The IRQ handler may still be set up when 'adl_pci7x3x_reset' is called,
so I suggest using 'dev->spinlock'.

> +
> +
>  static int adl_pci7x3x_auto_attach(struct comedi_device *dev,
>                     unsigned long context)
>  {
> -    struct pci_dev *pcidev = comedi_to_pci_dev(dev);
> -    const struct adl_pci7x3x_boardinfo *board = NULL;
> -    struct comedi_subdevice *s;
> +    struct pci_dev * pcidev = comedi_to_pci_dev(dev);
> +    struct adl_pci7x3x_dev_private_data * dev_private = dev->private;

'dev->private' will be NULL when this is called. Actually, it isn't
really necessary to initialize the 'dev_private' variable here because
it is assigned to below.

I suggest also checking 'board_p->irq_nchan' to avoid setting up the
interrupt handler if not needed.

> +      ret = request_irq(pcidev->irq, adl_pci7x3x_interrupt,
> IRQF_SHARED, dev->board_name, dev);
> +      if (ret == 0)
> +      {
> +        dev->irq = pcidev->irq;  /* muss gesetzt werden, bevor es
> abgefragt wird! */
> +        Dprintf(" prep IDI CH 0 HI + CH 1 HI IRQs... ");
> +        dev_private->int_ctrl = EN_PCI_LINT2H_LINT1H;  /* 0x52 enable
> PCI + IDI sdi[0] Ch 1 Ch 0 IRQ Off ActHigh */
> +        outb(dev_private->int_ctrl, dev_private->lcr_io_base +
> ADL_OP_LINTCSR);

Safer to lock 'dev->spinlock' when updating the 'int_ctrl' member and
writing to the INTCSR register.

The original version of this subdevice is fine, no need to allocate the
private data.

>
>          subdev++;
>
> -        nchan = board->di_nchan - nchan;
> +        nchan = board_p->di_nchan - nchan;
>          if (nchan) {
>              s = &dev->subdevices[subdev];
>              /* Isolated digital inputs 32 to 63 */
> @@ -207,14 +526,22 @@
>              s->insn_bits    = adl_pci7x3x_di_insn_bits;
>              s->range_table    = &range_digital;
>
> -            s->private    = (void *)PCI743X_DIO_REG;
> +            sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
> +            if(!sd_priv)
> +            {
> +              return -ENOMEM;
> +            }
> +            /*endif(!sd_priv)*/
> +            sd_priv->port_offset = PCI7X3X_DIO_REG;
> +            sd_priv->sd_idx = subdev;
> +            sd_priv->cmd_running = 0;

Ditto.

>
>              subdev++;
>          }
>      }
>
> -    if (board->do_nchan) {
> -        nchan = min(board->do_nchan, 32);
> +    if (board_p->do_nchan) {
> +        nchan = min(board_p->do_nchan, 32);
>
>          s = &dev->subdevices[subdev];
>          /* Isolated digital outputs 0 to 15/31 */
> @@ -225,11 +552,19 @@
>          s->insn_bits    = adl_pci7x3x_do_insn_bits;
>          s->range_table    = &range_digital;
>
> -        s->private    = (void *)PCI7X3X_DIO_REG;
> +        sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
> +        if(!sd_priv)
> +        {
> +          return -ENOMEM;
> +        }
> +        /*endif(!sd_priv)*/
> +        sd_priv->port_offset = PCI7X3X_DIO_REG;
> +        sd_priv->sd_idx = subdev;
> +        sd_priv->cmd_running = 0;

Ditto.

>
>          subdev++;
>
> -        nchan = board->do_nchan - nchan;
> +        nchan = board_p->do_nchan - nchan;
>          if (nchan) {
>              s = &dev->subdevices[subdev];
>              /* Isolated digital outputs 32 to 63 */
> @@ -240,25 +575,90 @@
>              s->insn_bits    = adl_pci7x3x_do_insn_bits;
>              s->range_table    = &range_digital;
>
> -            s->private    = (void *)PCI743X_DIO_REG;
> +            sd_priv = comedi_alloc_spriv(s, sizeof(*sd_priv));
> +            if(!sd_priv)
> +            {
> +              return -ENOMEM;
> +            }
> +            /*endif(!sd_priv)*/
> +            sd_priv->port_offset = PCI7X3X_DIO_REG;
> +            sd_priv->sd_idx = subdev;
> +            sd_priv->cmd_running = 0;

Ditto.

>
>              subdev++;
>          }
>      }
>
> +    for (ic = 0; ic < board_p->irq_nchan; ++ic) {
> +        nchan = 1;
> +
> +        s = &dev->subdevices[subdev];
> +        /* Isolated digital inputs 0  or 1 */
> +        s->type        = COMEDI_SUBD_DI;
> +        s->subdev_flags    = SDF_READABLE;
> +        s->n_chan    = nchan;
> +        s->maxdata    = 1;
> +        s->insn_bits    = adl_pci7x3x_di_insn_bits;

A different 'insn_bits' handler should be used for these 'interrupt'
subdevices. It doesn't really need to return anything meaningful since
it only has a single one-bit channel. Perhaps it could check the state
of the IDI0 or IDI1 line?

'dev->private' could be NULL here (although it is unlikely), leading to
a null pointer dereference bug. (The 'detach' handler gets called to
clean up even if the 'auto_attach' handler returned an error.)

Apart from printing the message (which will be removed anyway, probably
:) ), the freeing of the IRQ handler could be left to the
'comedi_pci_detach()' call below. By the way, the message could use the
'dev->board_name' instead of 'boardtype'. (In case the 'auto_attach'
handler returned an error before setting 'dev->board_name', it will
still be a non-NULL pointer because it is initialized to the driver name
before the 'auto_attach' handler is called. Also, the 'auto_attach'
handler is setting 'dev->board_name' before setting up the interrupt
handler anyway, so it should be the correct string.)

> +
> +    //comedi_free_devpriv(dev);
> +    comedi_pci_detach(dev);
> +}
> +
>
>  static struct comedi_driver adl_pci7x3x_driver = {
>      .driver_name    = "adl_pci7x3x",
>      .module        = THIS_MODULE,
>      .auto_attach    = adl_pci7x3x_auto_attach,
> -    .detach        = comedi_pci_detach,
> +    .detach        = adl_pci7x3x_detach,
>  };
>
>  static int adl_pci7x3x_pci_probe(struct pci_dev *dev,
>                   const struct pci_device_id *id)
>  {
> +    FuncId("adl_pci7x3x_pci_probe() %s \n", DATETIME);
>      return comedi_pci_auto_config(dev, &adl_pci7x3x_driver,
>                        id->driver_data);
>  }

That's all for now.

Best regards,
Ian

[Ian Abbott]

On 16/10/2020 12:41, Der Alte wrote:
> Hi Folks,
> here is the ext_trig.c source which is helpful to test the new Dig-IN
> features and learn how to take
> advantage in fast reactions on external triggers. E.G. clocks with PPS
> (Pulse Per Second) output.
>
> I first connected a simple button to the special input pins of the
> Dig-Input ports but that one was bouncing ugly.
> Then I used a small Arduino Board (Digispark) to simulate a PPS  signal
> with just 20 usec duty cycle every second on P0 plus the inverted signal
> on a P2. And a 50 %  duty cycle rectangle  on the LED pin P1.
> For the isolated inputs of the Advantech PCI 1730 the 20 us pulse with
> just 5 Volts seems
> to be beyond the limits.
>
> The Code of ext_trig.c was mainly taken from ledclock.c
>
> Ciao, Bernd

Hi Bernd,

Could you post it to me as an attachment? The whitespace tends to get
mangled when posted inline. Then I can incorporate it into the
Comedilib source repository.

Thanks,
Ian

[Der Alte]

Hi Ian,

thanks for commenting the diff.

> Also, best to stick to English in the kernel sources. I'm not just

Yep.

I also saw that I still have some C++ comments (//) in the source code.

How about that?

And I normally use 2 spaces for indent in my sources (and I don't really like Tab-characters in source code)

Is there a rule on that for kernel code?

Ciao, Bernd

[Ian Abbott]

Hi Bernd,

On 19/10/2020 10:14, Der Alte wrote:
> Hi Ian,
>
> thanks for commenting the diff.
>
> > Also, best to stick to English in the kernel sources. I'm not just
>
> Yep.
> I also saw that I still have some C++ comments (//) in the source code.
> How about that?

C++-style comments are allowed in the kernel now.

> And I normally use 2 spaces for indent in my sources (and I don't really
> like Tab-characters in source code)
> Is there a rule on that for kernel code?

The normal indentation in the kernel code is 8-column tab characters.
There is a coding style guide for the Linux kernel:

https://www.kernel.org/doc/html/latest/process/coding-style.html

There is also a Perl script in the kernel sources for checking the
format of patches or (with the -f option) source files. It is
"scripts/checkpatch.pl".

[Ian Abbott]

On 16/10/2020 12:41, Der Alte wrote:

> Hi Folks,
> here is the ext_trig.c source which is helpful to test the new Dig-IN
> features and learn how to take
> advantage in fast reactions on external triggers. E.G. clocks with PPS
> (Pulse Per Second) output.
>
> I first connected a simple button to the special input pins of the
> Dig-Input ports but that one was bouncing ugly.
> Then I used a small Arduino Board (Digispark) to simulate a PPS  signal
> with just 20 usec duty cycle every second on P0 plus the inverted signal
> on a P2. And a 50 %  duty cycle rectangle  on the LED pin P1.
> For the isolated inputs of the Advantech PCI 1730 the 20 us pulse with
> just 5 Volts seems
> to be beyond the limits.
>
> The Code of ext_trig.c was mainly taken from ledclock.c
>
> Ciao, Bernd

Hi folks,

Bernd's 'ext_trig' demo mentioned above has been added to the Comedilib
git repo <https://github.com/Linux-Comedi/comedilib> now, with minor
changes.

[Der Alte]

Hi Ian,

did you receive the 2 patches for adv_pci_dio.c and adl_pci7x3x.c?

I didn't intend to send it from  that address but I messed it up... :-)

Your comments on spin locks have not been "processed" so far because I would need some more discussion on spin locks.

> I would also suggest setting 'sd_priv->cmd_running' within a spin-locked
section.

My knowledge is that the locked sections shall be held as short as possible.

I only surrounded Read-Modify-Write access to static storage with spin locks so far.

Unconditionally setting a quasi-boolean variable which no other code manipulates conditionally should be uncritical as far as I understand it.

> outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);

also looks uncritical to me. The RMW accesses to dev_private->int_ctrl, are spin-locked.

Yesterday I found another astonishing point in CoMeDI:

The adv_pci_dio.c I downloaded from github-comedi is much longer (~ 40 KB) than even the new adv_pci_dio.c from the kernel tree _with_ the interrupt-stuff added. (~24 KB)  :-O

The diffs seem to be everywhere...

Is it mainly caused by the PNP "emulation"?

Stay healthy and fit, Bernd

Ian Abbott schrieb am Freitag, 16. Oktober 2020 um 16:57:32 UTC+2:

[Ian Abbott]

Hi Bernd,

On 23/10/2020 10:26, Der Alte wrote:
> Hi Ian,
>
> did you receive the 2 patches for adv_pci_dio.c and adl_pci7x3x.c?
> I didn't intend to send it from  that address but I messed it up... :-)

Yes, but I haven't got round to looking at them yet.

> Your comments on spin locks have not been "processed" so far because I
> would need some more discussion on spin locks.
>
> > I would also suggest setting 'sd_priv->cmd_running' within a spin-locked
> section.
>
> My knowledge is that the locked sections shall be held as short as possible.
> I only surrounded Read-Modify-Write access to static storage with spin
> locks so far.
> Unconditionally setting a quasi-boolean variable which no other code
> manipulates conditionally should be uncritical as far as I understand it.

It's probably safe to set 'sd_priv->cmd_running = 1;' in the cmd
handler, but the spin-lock provides a nice memory barrier.

> > outb(dev_private->int_ctrl, dev_private->lcr_io_base + ADL_OP_LINTCSR);
>
> also looks uncritical to me. The RMW accesses to dev_private->int_ctrl,
> are spin-locked.

But there is a race between the various bits of code that write this
register. An older value could get written after a newer value.

For example, adl_pci7x3x_asy_cmd() might set 'dev_private->int_ctrl |=
PLX9052_INTCSR_LI1ENAB;' and put the updated 'dev_private->int_ctrl'
value in the arguments for the 'outb()' call. Meanwhile, the interrupt
handler could run and call 'comedi_handle_events()' for the other
subdevice, which could call the other subdevice's 'cancel' routine,
which also modifies 'dev_private->int_ctrl' and calls 'outb()'. Then
'adl_pci7x3x_asy_cmd()' finally gets around to calling 'outb()' with the
original value of 'dev_private->int_ctrl' which is stale.

> Yesterday I found another astonishing point in CoMeDI:
> The adv_pci_dio.c I downloaded from github-comedi is much longer (~ 40
> KB) than even the new adv_pci_dio.c from the kernel tree _with_ the
> interrupt-stuff added. (~24 KB)  :-O
>
> The diffs seem to be everywhere...
> Is it mainly caused by the PNP "emulation"?

The same is pretty much the case for all the drivers! H Hartley Sweeten
went on a bit of a comedi-patching bender for a couple of years around
the 2014-2015 time period.

For adv_pci_dio.c, part of the difference is due to PCI-1760 support
being moved to its own driver "adv_pci1760" in the kernel sources.
Support for 8254 timer subdevice was also moved to a separate module
"comedi_8254".

> Stay healthy and fit, Bernd

Thanks! You too.

[Der Alte]

Hi Ian,

> It's probably safe to set 'sd_priv->cmd_running = 1;' in the cmd
> handler, but the spin-lock provides a nice memory barrier.

Ah, I wasn't aware about that. I'll have to think some time about that...

Would a "big" spin_locked section be better or multiple short phases after another just around the critical calls?

In the moment I would prefer the big section, because locking and unlocking probably has some overhead and the code probably stays better readable.

Ciao, Bernd

[Der Alte]

Hi Ian,

> > Where would I have to add spin_lock_irqsave() and -_restore() inside of do_cmd () and cancel () ?
> > Just inside the existing dev-locked sections?
>
> I would suggest the following for process_irq():
>
>         spin_lock(&sd_priv->cmd_lock);
>         if (sd_priv->cmd_running)
>         {
>             comedi_buf_write_samples(sd_p, &sd_p->state, 1);
>         }
>         spin_unlock(&sd_priv->cmd_lock);
>

Ok, aggreed. I have that already, but..

> (where 'sd_priv->cmd_lock' is the new spin-lock), plus the following in the 'cancel' handler:
>
>     spin_lock_irqsave(&sd_priv->cmd_lock, cpu_flags);
>     sd_priv->cmd_running = 0;
>     spin_unlock_irqrestore(&sd_priv->cmd_lock, cpu_flags);

Hmm, aha, around a single instruction... I had never assumed that myself.

That looked totally atomic to me. Except for the memory barrier.

But I added it anyway and am running / testing it on 2 machines now...

I was more thinking of something like this:

in the 'cancel' handler:

    if (s->index == 2)
    {
        Dprintf(" adl disable IDI CH 0 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);
    spin_lock_irqsave(&sd_priv->subd_slock, cpu_flags2);
    sd_priv->cmd_running = 0;

        dev_private->int_ctrl &= ~PLX9052_INTCSR_LI1ENAB;  /* Disable LINTi1 */

        outb(dev_private->int_ctrl, dev_private->lcr_io_base + PLX9052_INTCSR);
    spin_unlock_irqrestore(&sd_priv->subd_slock, cpu_flags2);
        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }

    if (s->index == 3)
    {
        Dprintf(" adl disable IDI CH 1 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);
    spin_lock_irqsave(&sd_priv->subd_slock, cpu_flags2);
    sd_priv->cmd_running = 0;

        dev_private->int_ctrl &= ~PLX9052_INTCSR_LI2ENAB;  /* Disable LINTi2 */

        outb(dev_private->int_ctrl, dev_private->lcr_io_base + PLX9052_INTCSR);
    spin_unlock_irqrestore(&sd_priv->subd_slock, cpu_flags2);

        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }

(Where 'sd_priv->subd_slock' is the new spin-lock)

Is that too much  locking?

> and the following in the 'do_cmd' handler:
>
>    spin_lock_irqsave(&sd_priv->cmd_lock, cpu_flags);
>     sd_priv->cmd_running = 1;
>     spin_unlock_irqrestore(&sd_priv->cmd_lock, cpu_flags);
>
> (That one might not be strictly necessary, but keeps the code consistent and has a nice memory barrier.)

> I must say so far I have much more 'respect' for dead locks than for some lost signal edge losses in program startup phases...
> >
> > And I have run 4 or 7 parallel instances of the ext_trig program statically for days. Including 1 comedi_parport.
> > The most important requirement is that the System must not freeze!

> To be honest, there are various comedi drivers that really ought to be using spin-locks more, including the comedi_parport driver.

Ciao, Bernd

[Ian Abbott]

It shouldn't do any harm, but the nesting of spin-lock locking here
seems like overkill since the ISR doesn't nest the locking.

(The main thing to be careful of with nested locking is to always lock
them in the same hierarchical order to avoid "A then B / B then A"
deadlocks, but that does not apply here.)

Using the irqsave/irqrestore style for both the outer and inner locks is
a bit overkill. If the outer spin_lock_irqsave has already disabled
interrupts on the local CPU and saved the flags, there is no need for
the inner spin_lock to do the same.

>
> > and the following in the 'do_cmd' handler:
> >
> >    spin_lock_irqsave(&sd_priv->cmd_lock, cpu_flags);
> >     sd_priv->cmd_running = 1;
> >     spin_unlock_irqrestore(&sd_priv->cmd_lock, cpu_flags);
> >
> > (That one might not be strictly necessary, but keeps the code
> consistent and has a nice memory barrier.)
>
> > I must say so far I have much more 'respect' for dead locks than for
> some lost signal edge losses in program startup phases...
> > >
> > > And I have run 4 or 7 parallel instances of the ext_trig program
> statically for days. Including 1 comedi_parport.
> > > The most important requirement is that the System must not freeze!
>
> > To be honest, there are various comedi drivers that really ought to
> be using spin-locks more, including the comedi_parport driver.
>
>
> Ciao, Bernd

Best regards,
Ian

[Der Alte]

Hi Ian and others,

yesterday i tested the 'light'  variant of spin-locked lines as you suggested and found no problems or differences to the previeous behaviour without subdevice specific locks.

Today I'm trying the announced variant of spinlock usage. It also seems to perform so far.

Code sample from comedi/drivers/adl_pci7x3x.c:

static int adl_pci7x3x_asy_cancel(struct comedi_device *dev,
    struct comedi_subdevice *s)
{

    struct adl_pci7x3x_dev_private_data *dev_private = dev->private;

    struct adl_pci7x3x_sd_private_data * sd_priv = s->private;

    unsigned long cpu_flags;
    unsigned long sd_cpu_flags;

    /* disable Interrupts */

    if (s->index == 2)
    {
        Dprintf(" adl disable IDI CH 0 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);

    spin_lock_irqsave(&sd_priv->subd_slock, sd_cpu_flags);

    sd_priv->cmd_running = 0;
        dev_private->int_ctrl &= ~PLX9052_INTCSR_LI1ENAB;  /* Disable LINTi1 */
        outb(dev_private->int_ctrl, dev_private->lcr_io_base + PLX9052_INTCSR);

    spin_unlock_irqrestore(&sd_priv->subd_slock, sd_cpu_flags);

        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }
    if (s->index == 3)
    {
        Dprintf(" adl disable IDI CH 1 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);

    spin_lock_irqsave(&sd_priv->subd_slock, sd_cpu_flags);

    sd_priv->cmd_running = 0;
        dev_private->int_ctrl &= ~PLX9052_INTCSR_LI2ENAB;  /* Disable LINTi2 */
        outb(dev_private->int_ctrl, dev_private->lcr_io_base + PLX9052_INTCSR);

    spin_unlock_irqrestore(&sd_priv->subd_slock, sd_cpu_flags);
        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }
    return 0;
}

Code sample from  drivers/adv_pci_dio.c:

 static int pci_dio_asy_cancel(struct comedi_device *dev,
     struct comedi_subdevice *s)
{
    struct pci_dio_dev_private_data *dev_private = dev->private;
    struct pci_dio_sd_private_data * sd_priv = s->private;
    unsigned long cpu_flags;
    unsigned long sd_cpu_flags;

    if (s->index == 5)
    {
        Dprintf(" adv disable DI CH 0 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);
    spin_lock_irqsave(&sd_priv->subd_slock, sd_cpu_flags);
    sd_priv->cmd_running = 0;
        dev_private->int_ctrl &= ~PCI173X_INT_DI0;  /* Disable DI0 IRQ */
        outb(dev_private->int_ctrl, dev->iobase + PCI173X_INT_EN_REG);
    spin_unlock_irqrestore(&sd_priv->subd_slock, sd_cpu_flags);
        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }
    if (s->index == 6)
    {
        Dprintf(" adv disable DI CH 1 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);
    spin_lock_irqsave(&sd_priv->subd_slock, sd_cpu_flags);
    sd_priv->cmd_running = 0;
        dev_private->int_ctrl &= ~PCI173X_INT_DI1;  /* Disable DI1 IRQ */
        outb(dev_private->int_ctrl, dev->iobase + PCI173X_INT_EN_REG);
    spin_unlock_irqrestore(&sd_priv->subd_slock, sd_cpu_flags);
        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }
    if (s->index == 7)
    {
        Dprintf(" adv disable IDI CH 0 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);
    spin_lock_irqsave(&sd_priv->subd_slock, sd_cpu_flags);
    sd_priv->cmd_running = 0;
        dev_private->int_ctrl &= ~PCI173X_INT_IDI0;  /* Disable IDI0 IRQ */
        outb(dev_private->int_ctrl, dev->iobase + PCI173X_INT_EN_REG);
    spin_unlock_irqrestore(&sd_priv->subd_slock, sd_cpu_flags);
        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }
    if (s->index == 8)
    {
        Dprintf(" adv disable IDI CH 1 IRQ... \n");
        spin_lock_irqsave(&dev->spinlock, cpu_flags);
    spin_lock_irqsave(&sd_priv->subd_slock, sd_cpu_flags);
    sd_priv->cmd_running = 0;
        dev_private->int_ctrl &= ~PCI173X_INT_IDI1;  /* Disable IDI1 IRQ */
        outb(dev_private->int_ctrl, dev->iobase + PCI173X_INT_EN_REG);
    spin_unlock_irqrestore(&sd_priv->subd_slock, sd_cpu_flags);
        spin_unlock_irqrestore(&dev->spinlock, cpu_flags);
    }
    return 0;
}

Additionally I read the hints at

https://0xax.gitbooks.io/linux-insides/content/SyncPrim/linux-sync-1.html

and was 'impressed'. :-)  I concluded that spin_locks in the Linux Kernel seem quite expansive to me. Although the most programming is realized with macros and thus should be resolved at compile time, the preemption is disabled. Am I right, that on a CPU / System with 20 cores all the other 19 cores are stopped from processing interrupts?

Maybe I have to learn much more about the mechanisms in CPUs other than the good old MC 68040 :-)

Ciao, Bernd

[Ian Abbott]

On 12/11/2020 12:12, Der Alte wrote:
> Additionally I read the hints at
> https://0xax.gitbooks.io/linux-insides/content/SyncPrim/linux-sync-1.html
> and was 'impressed'. :-)  I concluded that spin_locks in the Linux
> Kernel seem quite expansive to me. Although the most programming is
> realized with macros and thus should be resolved at compile time, the
> preemption is disabled. Am I right, that on a CPU / System with 20 cores
> all the other 19 cores are stopped from processing interrupts?

spin_lock_irq(&lock) and spin_lock_irqsave(&lock, flags) only disable
interrupts on the local CPU core. The reason for disabling the
interrupt while the spin-lock is held is to prevent the local CPU being
interrupted and running an ISR that acquires the same spin-lock, which
would cause the local CPU to become dead-locked. It does not need to
prevent other CPUs from running the ISR because those can just spin-wait
in the ISR until the lock is released by the CPU that acquired it.

[Der Alte]

Hi Ian,

funny, seems like we jut processed the group conversation parallelly. :-)

I started ~ an hour ago and collected the informations together peace by peace. Then you preempted me with your post .

I did not aquire an exclusive lock before I started. ;-)

But that was ok. The info I processed was still independent. An noone would be harmed anyway. ;-)

> Using the irqsave/irqrestore style for both the outer and inner locks is
> a bit overkill. If the outer spin_lock_irqsave has already disabled
> interrupts on the local CPU and saved the flags, there is no need for
> the inner spin_lock to do the same.

I was thinking about this, too, when I declared the extra sd_cpu_flags today. ;-)

But then thought that I probably I'll revert the changes after testing and hopefully challenging them as much as possible.

Ciao, Bernd