Is this an issue? In SAMA5 sam_tsd_read a call to enter_critical_section() can possibly block.

[TimJTi]

I noticed that a call to sam_tsd_read in SAMA5D2, to read touchscreen data, calls sam_tsd_waitsample if the character driver has been opened as blocking. That routine calls an enter_critical_section() to wait for a sample - and the low level driver defaults to pen detection as the interrupt mechanism to wait for a sample, so the wait could be for a very long time!

Assuming I have understand critical sections properly (i.e. all interrupts suppressed) of course.

While we probably shouldn't be opening an input device as blocking (but the touchscreen example app does), is this something that shouldn't be like this?

I will be submitting a PR for other changes to this driver so can make changes if my hunch is right?

[acassis]

@TimJTi I think you are right. A proper way to fix it should be using sched_lock() if the MCU doesn't support SMP or using spinlock otherwise.

[TimJTi]

@acassis Not an SMP system.

Won't sched_lock cause a similar problem though? Why shouldn't other tasks be added/run just because the TSD driver is waiting on something?

I am not 100% sure I am fully understanding the nuances of this :-)

[acassis]

No, think with shed_lock() the interrupts still enabled. So its ISR could run and the TSD could get the data. Unless it is using work queue to get the data from the HW, in this case the context switch will not happen.

[TimJTi]

@acassis

No, think with shed_lock() the interrupts still enabled. So its ISR could run and the TSD could get the data. Unless it is using work queue to get the data from the HW, in this case the context switch will not happen.

I checked the description of sched_lock():

"Disables context switching by disabling addition of new tasks to the ready-to-run task list. The task that calls this function will be the only task that is allowed to run until it either calls sched_unlock (the appropriate number of times) or until it blocks itself."

Does that mean no other tasks will run except the TSD task, until someone touches the screen (because by default interrupts are just pen detection, waiting for something to happen)? It could be many many minutes or hours if the screen is not being used! Or am I still not "getting" this?

[acassis]

@TimJTi I think you are right, the issue is not the interruption "per se", but the way the logic to read the TS interrupts is done.

[TimJTi]

@acassis - as it happens I don't use the pendet interrupt and last year submitted changes to make sure the SAMA5 TSD works properly if the ADC is set to periodic triggers.

But I still don't know what the right fix is, when the TSD and ADC are a shared peripheral. What if the periodic TSD trigger is slow - the same would be true and it could block for ages.

Maybe just a DEBUGASSERT if the TSD is opened as blocking?

[TimJTi]

@acassis I think that the SAMA5 family are perhaps unique in allowing the pen detection interrupt. This is great if that is all you are using the adc for, but is otherwise no use. And even worse if you open the character driver in blocking mode and want to do other things!

What I propose is the following.

  ret = sam_tsd_sample(priv, &sample);
  if (ret < 0)
    {
      /* Sample data is not available now.  We would have to wait to get
       * receive sample data.  If the user has specified the O_NONBLOCK
       * option, then just return an error.
       */

      iinfo("Sample data is not available\n");
      if (filep->f_oflags & O_NONBLOCK)
        {
          ret = -EAGAIN;
          goto errout;
        }
      else
        {
          /* If we have opened in blocking mode, there is a big risk that
           * we cause the system to hang because tsd_wait_sample will enter
           * a critical section until an adc sample is available - which may
           * be a very long time if the pen detect ADC trigger is in use.
           */

          regval  = sam_adc_getreg(priv->adc, SAM_ADC_TRGR);
          regval &= ADC_TRGR_TRGMOD_MASK;
          DEBUGASSERT(regval != ADC_TRGR_TRGMOD_PEN);
        }

Is that acceptable?

[TimJTi]

Implemented, as above, in PR #12131