[tpmdd-devel,4/6] tpm_tis: print log message before probing IRQs

From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

IRQ probing can take a long time and irrtitate users.
Inform users what's going on.

Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
---
 drivers/char/tpm/tpm_tis.c | 4 ++++
 1 file changed, 4 insertions(+)

On Thu, Nov 05, 2015 at 05:19:11PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> IRQ probing can take a long time and irrtitate users.
> Inform users what's going on.
> 
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 7619035..f5d7d52 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -779,6 +779,10 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  			irq_e = 15;
>  		}
>  
> +		dev_info(dev, "Probing IRQ - this may take some time.\n");

This is the only line that makes sense to me. Other lines add more
clutter than bring benefit.

> +		dev_info(dev, "\"genirq: Flags mismatch\" warnings may be logged while probing;\n");
> +		dev_info(dev, "they can be safely ignored.\n");
> +		dev_info(dev, "You may skip IRQ probing with parameter \"tpm_tis.interrupts=0\"\n");
>  		for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
>  			dev_dbg(dev, "Probing irq %d\n", i);
>  			iowrite8(i, chip->vendor.iobase +
> -- 
> 1.8.3.1

/Jarkko

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

Hi Jarkko,


On Do, 2015-11-05 at 23:44 +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 05, 2015 at 05:19:11PM +0100, martin.wilck@ts.fujitsu.com wrote:
> > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> > 
> > IRQ probing can take a long time and irrtitate users.
> > Inform users what's going on.
> > 
> > Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> > ---
> >  drivers/char/tpm/tpm_tis.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index 7619035..f5d7d52 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -779,6 +779,10 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  			irq_e = 15;
> >  		}
> >  
> > +		dev_info(dev, "Probing IRQ - this may take some time.\n");
> 
> This is the only line that makes sense to me. Other lines add more
> clutter than bring benefit.

If any of the probed IRQs has already been taken by a handler that
hasn't set IRQF_SHARED, probing will cause seriously ugly kernel stack
traces starting with a "genirq: Flags mismatch" message. Have you seen
those? They look highly irritating even to experienced users. I see no
way to avoid this, therefore I thought a "don't worry" message would do
no harm.

I know there's sort of a contradiction between this statement and my
other patch that actually introduces IRQF_SHARED. The intention of the
other patch is to avoid false probing. The intention of this one is just
to make the (almost) necessarily generated kernel traces look less
scary.

Regards
Martin


> 
> > +		dev_info(dev, "\"genirq: Flags mismatch\" warnings may be logged while probing;\n");
> > +		dev_info(dev, "they can be safely ignored.\n");
> > +		dev_info(dev, "You may skip IRQ probing with parameter \"tpm_tis.interrupts=0\"\n");
> >  		for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
> >  			dev_dbg(dev, "Probing irq %d\n", i);
> >  			iowrite8(i, chip->vendor.iobase +



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

On Tue, Nov 17, 2015 at 08:51:30PM +0100, Wilck, Martin wrote:
> If any of the probed IRQs has already been taken by a handler that
> hasn't set IRQF_SHARED, probing will cause seriously ugly kernel stack
> traces starting with a "genirq: Flags mismatch" message. Have you seen
> those? They look highly irritating even to experienced users. I see no
> way to avoid this, therefore I thought a "don't worry" message would do
> no harm.
> 
> I know there's sort of a contradiction between this statement and my
> other patch that actually introduces IRQF_SHARED. The intention of the
> other patch is to avoid false probing. The intention of this one is just
> to make the (almost) necessarily generated kernel traces look less
> scary.

And adds more traffic to klog without any major benefit. I don't even
know if we can talk about user convenience here because no standard user
reads klog. And those who read it would probably appreciate that each
subsystem keeps its traffic minimal and to the point.

> Regards
> Martin

/Jarkko

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