| Message ID | 1448485536-7282-6-git-send-email-jgunthorpe@obsidianresearch.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 25, 2015 at 02:05:34PM -0700, Jason Gunthorpe wrote: > The new code that works directly in tpm_tis_send is able to handle > IRQ probing duties as well, so just use it for everything. Doesn't this patch implement also new IRQ probing? Couldn't you replace irq_probing and irq_tested with a variable 'flags' and have an enum? I'm not a huge fan of boolean variables. They make the code look a mess and you cannot print them all with a single printk nor you can change the state with a single assign statement. > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> /Jarkko > --- > drivers/char/tpm/tpm.h | 1 - > drivers/char/tpm/tpm_tis.c | 67 ++++++++++++++-------------------------------- > 2 files changed, 20 insertions(+), 48 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index cdd49cd51dc1..542a80cbfd9c 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -142,7 +142,6 @@ struct tpm_vendor_specific { > unsigned long base; /* TPM base address */ > > int irq; > - int probed_irq; > > int region_size; > int have_region; > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 65a252c7ec5f..2580893de023 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -99,6 +99,7 @@ static struct tpm_info tis_default_info = { > #define TPM_RID(l) (0x0F04 | ((l) << 12)) > > struct priv_data { > + bool irq_probing; > bool irq_tested; > }; > > @@ -463,8 +464,9 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > msleep(1); > if (!priv->irq_tested) { > disable_interrupts(chip); > - dev_err(chip->pdev, > - FW_BUG "TPM interrupt not working, polling instead\n"); > + if (!priv->irq_probing) > + dev_err(chip->pdev, FW_BUG > + "TPM interrupt not working, polling instead\n"); > } > priv->irq_tested = true; > return rc; > @@ -570,26 +572,6 @@ static const struct tpm_class_ops tpm_tis = { > .req_canceled = tpm_tis_req_canceled, > }; > > -static irqreturn_t tis_int_probe(int irq, void *dev_id) > -{ > - struct tpm_chip *chip = dev_id; > - u32 interrupt; > - > - interrupt = ioread32(chip->vendor.iobase + > - TPM_INT_STATUS(chip->vendor.locality)); > - > - if (interrupt == 0) > - return IRQ_NONE; > - > - chip->vendor.probed_irq = irq; > - > - /* Clear interrupts handled with TPM_EOI */ > - iowrite32(interrupt, > - chip->vendor.iobase + > - TPM_INT_STATUS(chip->vendor.locality)); > - return IRQ_HANDLED; > -} > - > static irqreturn_t tis_int_handler(int dummy, void *dev_id) > { > struct tpm_chip *chip = dev_id; > @@ -772,13 +754,14 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > iowrite8(i, chip->vendor.iobase + > TPM_INT_VECTOR(chip->vendor.locality)); > if (devm_request_irq > - (dev, i, tis_int_probe, IRQF_SHARED, > + (dev, i, tis_int_handler, IRQF_SHARED, > chip->devname, chip) != 0) { > dev_info(chip->pdev, > "Unable to request irq: %d for probe\n", > i); > continue; > } > + chip->vendor.irq = i; > > /* Clear all existing */ > iowrite32(ioread32 > @@ -792,7 +775,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > chip->vendor.iobase + > TPM_INT_ENABLE(chip->vendor.locality)); > > - chip->vendor.probed_irq = 0; > + priv->irq_tested = false; > + priv->irq_probing = true; > > /* Generate Interrupts */ > if (chip->flags & TPM_CHIP_FLAG_TPM2) > @@ -800,29 +784,20 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > else > tpm_gen_interrupt(chip); > > - if (!chip->vendor.probed_irq) > - msleep(1); > - > - chip->vendor.irq = chip->vendor.probed_irq; > - > - /* free_irq will call into tis_int_probe; > - clear all irqs we haven't seen while doing > - tpm_gen_interrupt */ > - iowrite32(ioread32 > - (chip->vendor.iobase + > - TPM_INT_STATUS(chip->vendor.locality)), > - chip->vendor.iobase + > - TPM_INT_STATUS(chip->vendor.locality)); > - > - /* Turn off */ > - iowrite32(intmask, > - chip->vendor.iobase + > - TPM_INT_ENABLE(chip->vendor.locality)); > + priv->irq_probing = false; > > - devm_free_irq(dev, i, chip); > + /* tpm_tis_send will either confirm the interrupt is > + * working or it will call disable_irq which undoes > + * all of the above. > + */ > + if (chip->vendor.irq) > + break; > } > + if (!chip->vendor.irq) > + iowrite8(irq_r, chip->vendor.iobase + > + TPM_INT_VECTOR(chip->vendor.locality)); > } > - if (chip->vendor.irq) { > + if (chip->vendor.irq && !priv->irq_tested) { > iowrite8(chip->vendor.irq, > chip->vendor.iobase + > TPM_INT_VECTOR(chip->vendor.locality)); > @@ -846,9 +821,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > chip->vendor.iobase + > TPM_INT_ENABLE(chip->vendor.locality)); > } > - } else if (irq_r != -1) > - iowrite8(irq_r, chip->vendor.iobase + > - TPM_INT_VECTOR(chip->vendor.locality)); > + } > > if (tpm_get_timeouts(chip)) { > dev_err(dev, "Could not get TPM timeouts and durations\n"); > -- > 2.1.4 > ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
On Mon, Nov 30, 2015 at 05:50:00PM +0200, Jarkko Sakkinen wrote: > On Wed, Nov 25, 2015 at 02:05:34PM -0700, Jason Gunthorpe wrote: > > The new code that works directly in tpm_tis_send is able to handle > > IRQ probing duties as well, so just use it for everything. > > Doesn't this patch implement also new IRQ probing? Couldn't you replace > irq_probing and irq_tested with a variable 'flags' and have an enum? irq_probing is just a temporary thing, it is removed in a later patch once enough steps have been done to rework the entire flow > I'm not a huge fan of boolean variables. They make the code look a mess > and you cannot print them all with a single printk nor you can change > the state with a single assign statement. There is no sense to change to a flags for the one boolean. Jason ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
On Mon, Nov 30, 2015 at 10:24:23AM -0700, Jason Gunthorpe wrote: > On Mon, Nov 30, 2015 at 05:50:00PM +0200, Jarkko Sakkinen wrote: > > On Wed, Nov 25, 2015 at 02:05:34PM -0700, Jason Gunthorpe wrote: > > > The new code that works directly in tpm_tis_send is able to handle > > > IRQ probing duties as well, so just use it for everything. > > > > Doesn't this patch implement also new IRQ probing? Couldn't you replace > > irq_probing and irq_tested with a variable 'flags' and have an enum? > > irq_probing is just a temporary thing, it is removed in a later patch > once enough steps have been done to rework the entire flow > > > I'm not a huge fan of boolean variables. They make the code look a mess > > and you cannot print them all with a single printk nor you can change > > the state with a single assign statement. > > There is no sense to change to a flags for the one boolean. It makes the code easier to follow when you don't have to backtrace what some true/false parameter meant (i.e. self-documenting). I'll create a patch to top of these series. It's a separate clean up. /Jarkko ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
On Mon, Nov 30, 2015 at 11:12:00PM +0200, Jarkko Sakkinen wrote: > On Mon, Nov 30, 2015 at 10:24:23AM -0700, Jason Gunthorpe wrote: > > On Mon, Nov 30, 2015 at 05:50:00PM +0200, Jarkko Sakkinen wrote: > > > On Wed, Nov 25, 2015 at 02:05:34PM -0700, Jason Gunthorpe wrote: > > > > The new code that works directly in tpm_tis_send is able to handle > > > > IRQ probing duties as well, so just use it for everything. > > > > > > Doesn't this patch implement also new IRQ probing? Couldn't you replace > > > irq_probing and irq_tested with a variable 'flags' and have an enum? > > > > irq_probing is just a temporary thing, it is removed in a later patch > > once enough steps have been done to rework the entire flow > > > > > I'm not a huge fan of boolean variables. They make the code look a mess > > > and you cannot print them all with a single printk nor you can change > > > the state with a single assign statement. > > > > There is no sense to change to a flags for the one boolean. > > It makes the code easier to follow when you don't have to backtrace what > some true/false parameter meant (i.e. self-documenting). I'll create a > patch to top of these series. It's a separate clean up. Actually no. I'll create it post-v4.5. It's something I saw that makes sense but there are already so many high priority patches that it's better to postpone it. /Jarkko ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index cdd49cd51dc1..542a80cbfd9c 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -142,7 +142,6 @@ struct tpm_vendor_specific { unsigned long base; /* TPM base address */ int irq; - int probed_irq; int region_size; int have_region; diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 65a252c7ec5f..2580893de023 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -99,6 +99,7 @@ static struct tpm_info tis_default_info = { #define TPM_RID(l) (0x0F04 | ((l) << 12)) struct priv_data { + bool irq_probing; bool irq_tested; }; @@ -463,8 +464,9 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) msleep(1); if (!priv->irq_tested) { disable_interrupts(chip); - dev_err(chip->pdev, - FW_BUG "TPM interrupt not working, polling instead\n"); + if (!priv->irq_probing) + dev_err(chip->pdev, FW_BUG + "TPM interrupt not working, polling instead\n"); } priv->irq_tested = true; return rc; @@ -570,26 +572,6 @@ static const struct tpm_class_ops tpm_tis = { .req_canceled = tpm_tis_req_canceled, }; -static irqreturn_t tis_int_probe(int irq, void *dev_id) -{ - struct tpm_chip *chip = dev_id; - u32 interrupt; - - interrupt = ioread32(chip->vendor.iobase + - TPM_INT_STATUS(chip->vendor.locality)); - - if (interrupt == 0) - return IRQ_NONE; - - chip->vendor.probed_irq = irq; - - /* Clear interrupts handled with TPM_EOI */ - iowrite32(interrupt, - chip->vendor.iobase + - TPM_INT_STATUS(chip->vendor.locality)); - return IRQ_HANDLED; -} - static irqreturn_t tis_int_handler(int dummy, void *dev_id) { struct tpm_chip *chip = dev_id; @@ -772,13 +754,14 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, iowrite8(i, chip->vendor.iobase + TPM_INT_VECTOR(chip->vendor.locality)); if (devm_request_irq - (dev, i, tis_int_probe, IRQF_SHARED, + (dev, i, tis_int_handler, IRQF_SHARED, chip->devname, chip) != 0) { dev_info(chip->pdev, "Unable to request irq: %d for probe\n", i); continue; } + chip->vendor.irq = i; /* Clear all existing */ iowrite32(ioread32 @@ -792,7 +775,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, chip->vendor.iobase + TPM_INT_ENABLE(chip->vendor.locality)); - chip->vendor.probed_irq = 0; + priv->irq_tested = false; + priv->irq_probing = true; /* Generate Interrupts */ if (chip->flags & TPM_CHIP_FLAG_TPM2) @@ -800,29 +784,20 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, else tpm_gen_interrupt(chip); - if (!chip->vendor.probed_irq) - msleep(1); - - chip->vendor.irq = chip->vendor.probed_irq; - - /* free_irq will call into tis_int_probe; - clear all irqs we haven't seen while doing - tpm_gen_interrupt */ - iowrite32(ioread32 - (chip->vendor.iobase + - TPM_INT_STATUS(chip->vendor.locality)), - chip->vendor.iobase + - TPM_INT_STATUS(chip->vendor.locality)); - - /* Turn off */ - iowrite32(intmask, - chip->vendor.iobase + - TPM_INT_ENABLE(chip->vendor.locality)); + priv->irq_probing = false; - devm_free_irq(dev, i, chip); + /* tpm_tis_send will either confirm the interrupt is + * working or it will call disable_irq which undoes + * all of the above. + */ + if (chip->vendor.irq) + break; } + if (!chip->vendor.irq) + iowrite8(irq_r, chip->vendor.iobase + + TPM_INT_VECTOR(chip->vendor.locality)); } - if (chip->vendor.irq) { + if (chip->vendor.irq && !priv->irq_tested) { iowrite8(chip->vendor.irq, chip->vendor.iobase + TPM_INT_VECTOR(chip->vendor.locality)); @@ -846,9 +821,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, chip->vendor.iobase + TPM_INT_ENABLE(chip->vendor.locality)); } - } else if (irq_r != -1) - iowrite8(irq_r, chip->vendor.iobase + - TPM_INT_VECTOR(chip->vendor.locality)); + } if (tpm_get_timeouts(chip)) { dev_err(dev, "Could not get TPM timeouts and durations\n");
The new code that works directly in tpm_tis_send is able to handle IRQ probing duties as well, so just use it for everything. Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> --- drivers/char/tpm/tpm.h | 1 - drivers/char/tpm/tpm_tis.c | 67 ++++++++++++++-------------------------------- 2 files changed, 20 insertions(+), 48 deletions(-)