[PATCH 1/2] tty/hvc: Use IRQF_SHARED for OPAL hvc consoles

[PATCH 1/2] tty/hvc: Use IRQF_SHARED for OPAL hvc consoles

[Samuel Mendoza-Jonas]

Commit 2def86a7200c
("hvc: Convert to using interrupts instead of opal events")
enabled the use of interrupts in the hvc_driver for OPAL platforms.
However on machines with more than one hvc console, any console after
the first will fail to register an interrupt handler in
notifier_add_irq() since all consoles share the same IRQ number but do
not set the IRQF_SHARED flag:

[   51.179907] genirq: Flags mismatch irq 31. 00000000 (hvc_console) vs.
00000000 (hvc_console)
[   51.180010] hvc_open: request_irq failed with rc -16.

This error propagates up to hvc_open() and the console is closed, but
OPAL will still generate interrupts that are not handled, leading to
rcu_sched stall warnings.

Set IRQF_SHARED when calling request_irq, allowing additional consoles
to start properly. This is only set for consoles handled by
hvc_opal_probe(), leaving other types unaffected.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Cc: <stable@vger.kernel.org> # 4.1.x-
---
 drivers/tty/hvc/hvc_console.h | 1 +
 drivers/tty/hvc/hvc_irq.c     | 7 +++++--
 drivers/tty/hvc/hvc_opal.c    | 3 +++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 9131019..798c48d 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -60,6 +60,7 @@ struct hvc_struct {
 	struct winsize ws;
 	struct work_struct tty_resize;
 	struct list_head next;
+	unsigned long flags;
 };
 
 /* implemented by a low level driver */
diff --git a/drivers/tty/hvc/hvc_irq.c b/drivers/tty/hvc/hvc_irq.c
index c9adb05..57d9df7 100644
--- a/drivers/tty/hvc/hvc_irq.c
+++ b/drivers/tty/hvc/hvc_irq.c
@@ -14,6 +14,9 @@ static irqreturn_t hvc_handle_interrupt(int irq, void *dev_instance)
 	/* if hvc_poll request a repoll, then kick the hvcd thread */
 	if (hvc_poll(dev_instance))
 		hvc_kick();
+	/* We're safe to always return IRQ_HANDLED as the hvcd thread will
+	 * iterate through each hvc_struct
+	 */
 	return IRQ_HANDLED;
 }
 
@@ -28,8 +31,8 @@ int notifier_add_irq(struct hvc_struct *hp, int irq)
 		hp->irq_requested = 0;
 		return 0;
 	}
-	rc = request_irq(irq, hvc_handle_interrupt, 0,
-			   "hvc_console", hp);
+	rc = request_irq(irq, hvc_handle_interrupt, hp->flags,
+			"hvc_console", hp);
 	if (!rc)
 		hp->irq_requested = 1;
 	return rc;
diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index 47b54c6..b7cd0ae 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -224,6 +224,9 @@ static int hvc_opal_probe(struct platform_device *dev)
 	hp = hvc_alloc(termno, irq, ops, MAX_VIO_PUT_CHARS);
 	if (IS_ERR(hp))
 		return PTR_ERR(hp);
+
+	/* hvc consoles on powernv may need to share a single irq */
+	hp->flags = IRQF_SHARED;
 	dev_set_drvdata(&dev->dev, hp);
 
 	return 0;
-- 
2.9.0

[PATCH 2/2] tty/hvc: Use opal irqchip interface if available

[Samuel Mendoza-Jonas]

Update the hvc driver to use the OPAL irqchip if made available by the
running firmware. If it is not present, the driver falls back to the
existing OPAL event number.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Cc: <stable@vger.kernel.org> # 4.1.x-
---
 drivers/tty/hvc/hvc_opal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
index b7cd0ae..8c53f5b 100644
--- a/drivers/tty/hvc/hvc_opal.c
+++ b/drivers/tty/hvc/hvc_opal.c
@@ -170,6 +170,8 @@ static int hvc_opal_probe(struct platform_device *dev)
 	hv_protocol_t proto;
 	unsigned int termno, irq, boot = 0;
 	const __be32 *reg;
+	u32 prop;
+	int rc;
 
 	if (of_device_is_compatible(dev->dev.of_node, "ibm,opal-console-raw")) {
 		proto = HV_PROTOCOL_RAW;
@@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev)
 		dev->dev.of_node->full_name,
 		boot ? " (boot console)" : "");
 
-	irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
+	rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop);
+	if (rc) {
+		pr_info("hvc%d: No interrupts property, using OPAL event\n",
+				termno);
+		irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
+	} else {
+		irq = irq_of_parse_and_map(dev->dev.of_node, 0);
+	}
+
 	if (!irq) {
 		pr_err("hvc_opal: Unable to map interrupt for device %s\n",
 			dev->dev.of_node->full_name);
-- 
2.9.0

Re: [PATCH 2/2] tty/hvc: Use opal irqchip interface if available

[Benjamin Herrenschmidt]

On Tue, 2016-06-28 at 13:11 +1000, Samuel Mendoza-Jonas wrote:
> Update the hvc driver to use the OPAL irqchip if made available by the
> running firmware. If it is not present, the driver falls back to the
> existing OPAL event number.

One thing that worries me a bit with the original transition to using
an interrupt from the old OPAL callback is that when passed an interrupt,
the HVC thread assumes interrupts work reliably and thus stops polling.

However, not all platforms have a functional serial interrupt. For
example rhesus doesn't. In fact we don't always know when we build
the device-tree whether the serial interrupt will work or not.

Now we might be saved by the OPAL heartbeat ... we do call
opal_poll_events regularily there. But I'd like you to verify it
by disabling the LPC interrupt for example on an openpower machine
and see how the console beahves.

Cheers,
Ben.
 

> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Cc: <stable@vger.kernel.org> # 4.1.x-
> ---
>  drivers/tty/hvc/hvc_opal.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index b7cd0ae..8c53f5b 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -170,6 +170,8 @@ static int hvc_opal_probe(struct platform_device *dev)
>  	hv_protocol_t proto;
>  	unsigned int termno, irq, boot = 0;
>  	const __be32 *reg;
> +	u32 prop;
> +	int rc;
>  
>  	if (of_device_is_compatible(dev->dev.of_node, "ibm,opal-console-raw")) {
>  		proto = HV_PROTOCOL_RAW;
> @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev)
>  		dev->dev.of_node->full_name,
>  		boot ? " (boot console)" : "");
>  
> -	irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> +	rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop);
> +	if (rc) {
> +		pr_info("hvc%d: No interrupts property, using OPAL event\n",
> +				termno);
> +		irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> +	} else {
> +		irq = irq_of_parse_and_map(dev->dev.of_node, 0);
> +	}
> +
>  	if (!irq) {
>  		pr_err("hvc_opal: Unable to map interrupt for device %s\n",
>  			dev->dev.of_node->full_name);

Re: [PATCH 2/2] tty/hvc: Use opal irqchip interface if available

[Benjamin Herrenschmidt]

On Tue, 2016-06-28 at 13:34 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2016-06-28 at 13:11 +1000, Samuel Mendoza-Jonas wrote:
> > Update the hvc driver to use the OPAL irqchip if made available by
> > the
> > running firmware. If it is not present, the driver falls back to
> > the
> > existing OPAL event number.
> 
> One thing that worries me a bit with the original transition to using
> an interrupt from the old OPAL callback is that when passed an
> interrupt,
> the HVC thread assumes interrupts work reliably and thus stops
> polling.

Note to Greg: This patch is fine, this is a reflexion about a change
that was already done.

> However, not all platforms have a functional serial interrupt. For
> example rhesus doesn't. In fact we don't always know when we build
> the device-tree whether the serial interrupt will work or not.
> 
> Now we might be saved by the OPAL heartbeat ... we do call
> opal_poll_events regularily there. But I'd like you to verify it
> by disabling the LPC interrupt for example on an openpower machine
> and see how the console beahves.
> 
> Cheers,
> Ben.
>  
> 
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> > Cc: <stable@vger.kernel.org> # 4.1.x-
> > ---
> >  drivers/tty/hvc/hvc_opal.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/hvc/hvc_opal.c
> > b/drivers/tty/hvc/hvc_opal.c
> > index b7cd0ae..8c53f5b 100644
> > --- a/drivers/tty/hvc/hvc_opal.c
> > +++ b/drivers/tty/hvc/hvc_opal.c
> > @@ -170,6 +170,8 @@ static int hvc_opal_probe(struct
> > platform_device *dev)
> >  	hv_protocol_t proto;
> >  	unsigned int termno, irq, boot = 0;
> >  	const __be32 *reg;
> > +	u32 prop;
> > +	int rc;
> >  
> >  	if (of_device_is_compatible(dev->dev.of_node, "ibm,opal-
> > console-raw")) {
> >  		proto = HV_PROTOCOL_RAW;
> > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct
> > platform_device *dev)
> >  		dev->dev.of_node->full_name,
> >  		boot ? " (boot console)" : "");
> >  
> > -	irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> > +	rc = of_property_read_u32(dev->dev.of_node, "interrupts",
> > &prop);
> > +	if (rc) {
> > +		pr_info("hvc%d: No interrupts property, using OPAL
> > event\n",
> > +				termno);
> > +		irq =
> > opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> > +	} else {
> > +		irq = irq_of_parse_and_map(dev->dev.of_node, 0);
> > +	}
> > +
> >  	if (!irq) {
> >  		pr_err("hvc_opal: Unable to map interrupt for
> > device %s\n",
> >  			dev->dev.of_node->full_name);

Re: [2/2] tty/hvc: Use opal irqchip interface if available

[Michael Ellerman]

On Tue, 2016-28-06 at 03:11:39 UTC, Sam Mendoza-Jonas wrote:
> diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> index b7cd0ae..8c53f5b 100644
> --- a/drivers/tty/hvc/hvc_opal.c
> +++ b/drivers/tty/hvc/hvc_opal.c
> @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev)
>  		dev->dev.of_node->full_name,
>  		boot ? " (boot console)" : "");
>  
> -	irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> +	rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop);
> +	if (rc) {
> +		pr_info("hvc%d: No interrupts property, using OPAL event\n",
> +				termno);
> +		irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> +	} else {
> +		irq = irq_of_parse_and_map(dev->dev.of_node, 0);
> +	}

That seems a bit backward.

Shouldn't we try irq_of_parse_and_map() and if that fails, then we go back to
opal_event_request() ?

cheers

Re: [2/2] tty/hvc: Use opal irqchip interface if available

[Samuel Mendoza-Jonas]

On Tue, 2016-07-05 at 15:31 +1000, Michael Ellerman wrote:
> On Tue, 2016-28-06 at 03:11:39 UTC, Sam Mendoza-Jonas wrote:
> > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
> > index b7cd0ae..8c53f5b 100644
> > --- a/drivers/tty/hvc/hvc_opal.c
> > +++ b/drivers/tty/hvc/hvc_opal.c
> > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev)
> >  		dev->dev.of_node->full_name,
> >  		boot ? " (boot console)" : "");
> >  
> > -	irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> > +	rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop);
> > +	if (rc) {
> > +		pr_info("hvc%d: No interrupts property, using OPAL event\n",
> > +				termno);
> > +		irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
> > +	} else {
> > +		irq = irq_of_parse_and_map(dev->dev.of_node, 0);
> > +	}
> 
> That seems a bit backward.
> 
> Shouldn't we try irq_of_parse_and_map() and if that fails, then we go back to
> opal_event_request() ?
> 
> cheers

I was unsure for a minute so I double checked this;
of_property_read_u32() returns of_property_read_u32_array(), which
returns 0 on success, unlike a bunch of other of_property helpers.

So if the 'interrupts' property does exist we get 0, and try
irq_of_parse_and_map(). But are you suggesting we try
irq_of_parse_and_map() regardless and then fall back to
opal_event_request()?

Cheers,
Sam

Re: [2/2] tty/hvc: Use opal irqchip interface if available

[Michael Ellerman]

Samuel Mendoza-Jonas <sam@mendozajonas.com> writes:

> On Tue, 2016-07-05 at 15:31 +1000, Michael Ellerman wrote:
>> On Tue, 2016-28-06 at 03:11:39 UTC, Sam Mendoza-Jonas wrote:
>> > diff --git a/drivers/tty/hvc/hvc_opal.c b/drivers/tty/hvc/hvc_opal.c
>> > index b7cd0ae..8c53f5b 100644
>> > --- a/drivers/tty/hvc/hvc_opal.c
>> > +++ b/drivers/tty/hvc/hvc_opal.c
>> > @@ -214,7 +216,15 @@ static int hvc_opal_probe(struct platform_device *dev)
>> >  		dev->dev.of_node->full_name,
>> >  		boot ? " (boot console)" : "");
>> >  
>> > -	irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
>> > +	rc = of_property_read_u32(dev->dev.of_node, "interrupts", &prop);
>> > +	if (rc) {
>> > +		pr_info("hvc%d: No interrupts property, using OPAL event\n",
>> > +				termno);
>> > +		irq = opal_event_request(ilog2(OPAL_EVENT_CONSOLE_INPUT));
>> > +	} else {
>> > +		irq = irq_of_parse_and_map(dev->dev.of_node, 0);
>> > +	}
>> 
>> That seems a bit backward.
>> 
>> Shouldn't we try irq_of_parse_and_map() and if that fails, then we go back to
>> opal_event_request() ?

> But are you suggesting we try irq_of_parse_and_map() regardless and
> then fall back to opal_event_request()?

Yes.

cheers

Re: [1/2] tty/hvc: Use IRQF_SHARED for OPAL hvc consoles

[Michael Ellerman]

On Tue, 2016-28-06 at 03:11:38 UTC, Sam Mendoza-Jonas wrote:
> Commit 2def86a7200c
> ("hvc: Convert to using interrupts instead of opal events")
> enabled the use of interrupts in the hvc_driver for OPAL platforms.
> However on machines with more than one hvc console, any console after
> the first will fail to register an interrupt handler in
> notifier_add_irq() since all consoles share the same IRQ number but do
> not set the IRQF_SHARED flag:
> 
> [   51.179907] genirq: Flags mismatch irq 31. 00000000 (hvc_console) vs.
> 00000000 (hvc_console)
> [   51.180010] hvc_open: request_irq failed with rc -16.
> 
> This error propagates up to hvc_open() and the console is closed, but
> OPAL will still generate interrupts that are not handled, leading to
> rcu_sched stall warnings.
> 
> Set IRQF_SHARED when calling request_irq, allowing additional consoles
> to start properly. This is only set for consoles handled by
> hvc_opal_probe(), leaving other types unaffected.
> 
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>

Acked-by: Michael Ellerman <mpe@ellerman.id.au>


Greg I'm assuming you'll take this unless you tell me otherwise.

cheers