Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Maarten Brock]

I also wonder why this driver doesn't use threaded interrupts, just like the
MAX310x driver it is based upon.

Maarten


> This driver should use a threaded interrupt instead of trying to emulate it
> via dis/enable_irq and a worker thread.
> 
> Then you simply call c16is7xx_port_irq() right from the interrupt routine
> and the core code deals with the interrupt mask/unmask automatically.
> 
> Thanks,
> 
> 	tglx

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sean Nyekjær]

On 2016-03-13 20:05, Maarten Brock wrote:
> I also wonder why this driver doesn't use threaded interrupts, just like the
> MAX310x driver it is based upon.
>
> Maarten
Me too.. :-) But both I and Sebastian Andrzej Siewior have proposed a 
PATCH for that but it was turned down.
>
>
>> This driver should use a threaded interrupt instead of trying to emulate it
>> via dis/enable_irq and a worker thread.
>>
>> Then you simply call c16is7xx_port_irq() right from the interrupt routine
>> and the core code deals with the interrupt mask/unmask automatically.
>>
>> Thanks,
>>
>> 	tglx
Remember not to top post :-) and use a proper mail client that uses 
In-Reply-To...

/Sean

[PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sean Nyekjaer]

Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
---
This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT" 

 drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 73d46e2..96d1f3e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
 	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
 	int i;
 
-	for (i = 0; i < s->devtype->nr_uart; ++i)
+	for (i = 0; i < s->devtype->nr_uart; ++i) {
 		sc16is7xx_port_irq(s, i);
+		enable_irq(s->p[i].port.irq);
+	}
+
 }
 
 static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 {
 	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
+	int i;
+
+	for (i = 0; i < s->devtype->nr_uart; ++i)
+		disable_irq_nosync(s->p[i].port.irq);
 
 	queue_kthread_work(&s->kworker, &s->irq_work);
 
-- 
2.7.2

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sean Nyekjær]

On 2016-03-11 11:34, Sean Nyekjaer wrote:
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
> ---
> This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT"
>
>   drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 73d46e2..96d1f3e 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
>   	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
>   	int i;
>   
> -	for (i = 0; i < s->devtype->nr_uart; ++i)
> +	for (i = 0; i < s->devtype->nr_uart; ++i) {
>   		sc16is7xx_port_irq(s, i);
> +		enable_irq(s->p[i].port.irq);
> +	}
> +
Ops one-line break to much :-(
Waiting for other comments before fixing :-)

/Sean
>   }
>   
>   static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>   {
>   	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
> +	int i;
> +
> +	for (i = 0; i < s->devtype->nr_uart; ++i)
> +		disable_irq_nosync(s->p[i].port.irq);
>   
>   	queue_kthread_work(&s->kworker, &s->irq_work);
>   

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sebastian Andrzej Siewior]

* Sean Nyekjær | 2016-03-11 11:36:42 [+0100]:

>Waiting for other comments before fixing :-)

You could start by writting a changelog stating why you think this change
is a good idea instead leaving it empty.

>/Sean

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sean Nyekjær]

On 2016-03-11 12:21, Sebastian Andrzej Siewior wrote:
> * Sean Nyekjær | 2016-03-11 11:36:42 [+0100]:
>
>> Waiting for other comments before fixing :-)
> You could start by writting a changelog stating why you think this change
> is a good idea instead leaving it empty.
Cool down a bit ;-)

It's left empty because I have been encouraged by Josh and Jakub to make 
this PATCH.
I have not seen any problems after simply dropping the IRQ_ONESHOT flag, 
but they proposed this fix.

>> /Sean
> Sebastian
/Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sean Nyekjær]

On 2016-03-11 11:36, Sean Nyekjær wrote:
>
>
> On 2016-03-11 11:34, Sean Nyekjaer wrote:
Could this work as a description?

The problem here is still the use of IRQF_ONESHOT.  The semantics
of IRQF_ONESHOT are only defined w.r.t. the irq core's threaded
interrupt handling.

This implements it's own oneshot-like handling
at the device-level: in the registered irq handler, capture triggered
interrupt state, squelch/mask, and enqueue the kthread_work.  In the
tail-end of the kthread_work, re-enable interrupts at the device level.

>> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
>> ---
>> This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT"
>>
>>   drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c 
>> b/drivers/tty/serial/sc16is7xx.c
>> index 73d46e2..96d1f3e 100644
>> --- a/drivers/tty/serial/sc16is7xx.c
>> +++ b/drivers/tty/serial/sc16is7xx.c
>> @@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
>>       struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
>>       int i;
>>   -    for (i = 0; i < s->devtype->nr_uart; ++i)
>> +    for (i = 0; i < s->devtype->nr_uart; ++i) {
>>           sc16is7xx_port_irq(s, i);
>> +        enable_irq(s->p[i].port.irq);
>> +    }
>> +
> Ops one-line break to much :-(
> Waiting for other comments before fixing :-)
>
> /Sean
>>   }
>>     static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>>   {
>>       struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
>> +    int i;
>> +
>> +    for (i = 0; i < s->devtype->nr_uart; ++i)
>> +        disable_irq_nosync(s->p[i].port.irq);
>>         queue_kthread_work(&s->kworker, &s->irq_work);
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe 
> linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Greg Kroah-Hartman]

On Fri, Mar 11, 2016 at 11:34:36AM +0100, Sean Nyekjaer wrote:
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>

I can not accept a patch without a changelog entry, especially for
something like this that is not "trivial".

greg k-h

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Thomas Gleixner]

On Fri, 11 Mar 2016, Sean Nyekjaer wrote:

>  static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>  {
>  	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
> +	int i;
> +
> +	for (i = 0; i < s->devtype->nr_uart; ++i)
> +		disable_irq_nosync(s->p[i].port.irq);

Aside of the lack of a changelog. This is completely bogus. You disable the
same interrupt a gazillion of times. 

>  	queue_kthread_work(&s->kworker, &s->irq_work);

This driver should use a threaded interrupt instead of trying to emulate it
via dis/enable_irq and a worker thread.

Then you simply call c16is7xx_port_irq() right from the interrupt routine and
the core code deals with the interrupt mask/unmask automatically.

Thanks,

	tglx

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Sean Nyekjær]

On 2016-03-13 11:25, Thomas Gleixner wrote:
> On Fri, 11 Mar 2016, Sean Nyekjaer wrote:
>
>>   static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
>>   {
>>   	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
>> +	int i;
>> +
>> +	for (i = 0; i < s->devtype->nr_uart; ++i)
>> +		disable_irq_nosync(s->p[i].port.irq);
> Aside of the lack of a changelog. This is completely bogus. You disable the
> same interrupt a gazillion of times.
I can't see why the interrupt is disabled a gazillion times.
When the irq is disabled the function will not be called before it's 
enabled again... Or have i missed something?
>>   	queue_kthread_work(&s->kworker, &s->irq_work);
> This driver should use a threaded interrupt instead of trying to emulate it
> via dis/enable_irq and a worker thread.
I agree, I and "Sebastian Andrzej Siewior" have proposed this fix, but 
it was turned down. :-)
>
> Then you simply call c16is7xx_port_irq() right from the interrupt routine and
> the core code deals with the interrupt mask/unmask automatically.
>
> Thanks,
>
> 	tglx
>
/Sean

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 945 bytes --]

On Mon, 14 Mar 2016, Sean Nyekjær wrote:
> On 2016-03-13 11:25, Thomas Gleixner wrote:
> > On Fri, 11 Mar 2016, Sean Nyekjaer wrote:
> > 
> > >   static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
> > >   {
> > >   	struct sc16is7xx_port *s = (struct sc16is7xx_port *)dev_id;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < s->devtype->nr_uart; ++i)
> > > +		disable_irq_nosync(s->p[i].port.irq);
> > Aside of the lack of a changelog. This is completely bogus. You disable the
> > same interrupt a gazillion of times.
> I can't see why the interrupt is disabled a gazillion times.
> When the irq is disabled the function will not be called before it's enabled
> again... Or have i missed something?

> > > +	for (i = 0; i < s->devtype->nr_uart; ++i)
> > > +		disable_irq_nosync(s->p[i].port.irq);

port.irq is the same for all ports at least according to the init function. It
is actually @irq, the handler function argument.

Thanks,

	tglx

Re: [PATCH] tty: serial: sc16is7xx: implemented our own oneshot-like handling

[Jakub Kiciński]

On Fri, 11 Mar 2016 11:34:36 +0100, Sean Nyekjaer wrote:
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
> ---
> This patch depends on patch "sc16is7xx: drop bogus use of IRQF_ONESHOT" 
> 
>  drivers/tty/serial/sc16is7xx.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 73d46e2..96d1f3e 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -706,13 +706,20 @@ static void sc16is7xx_ist(struct kthread_work *ws)
>  	struct sc16is7xx_port *s = to_sc16is7xx_port(ws, irq_work);
>  	int i;
>  
> -	for (i = 0; i < s->devtype->nr_uart; ++i)
> +	for (i = 0; i < s->devtype->nr_uart; ++i) {
>  		sc16is7xx_port_irq(s, i);
> +		enable_irq(s->p[i].port.irq);
> +	}
> +
>  }

Apart from needless multiple disable/enable (as tglx commented)
I think you also have to re-read the IRQ status (i.e. call
sc16is7xx_port_irq()) because IRQs coming between status read and
enable_irq may get lost for some IRQ controllers.