Re: [PATCH] vTPM: fix missing error handling for suspend operation
> > > + } else if (crq.msg == (u8) PARTNER_PARTITION_FAILED) {
> > > + dev_err(ibmvtpm->dev,
> > > + "vtpm has terminated fatally; reboot to
> > > reinstate a trusted state.\n");
> > > + } else if (crq.msg == (u8) PARTNER_PARTITION_DEREG_CRQ) {
> > > + /* The vtpm is in the process of being reloaded by
> > > + * firmware and has de-registered CRQ. The client
> > > + * must wait for the CRQ INITIALIZATION message and
> > > + * respond and must resubmit suspend message.
> > > + */
> > > + sig =
> > > + wait_event_interruptible(ibmvtpm->wq,
> > > + crq_initialized == 1);
> > > + if (sig)
> > > + return -EINTR;
> > > +
> > > + if (suspend_again_count < 1) {
> > > + suspend_again_count++;
> > > + goto suspendagain;
> > > + }
> > > + } else
> > > + ;
> > > + }
> > > +
> > > if (rc != H_SUCCESS)
> > > - dev_err(ibmvtpm->dev,
> > > - "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > > + dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > >
> > > return rc;
> > > +
> > > +suspendagain:
> > > + rc = tpm_ibmvtpm_suspend(ibmvtpm->dev);
> > > + suspend_again_count = 0;
> > > +
> > > + if (rc != H_SUCCESS)
> > > + dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > > +
> > > + return rc;
> > > +
> >
> > Get rid of this horrible looking tail recursion thing.
> >
> > What the heck is suspend_again_count and why it can be module scope
> > variable? You could use a local variable instead if you would iterate
> > with a loop.
> >
> > /Jarkko
> >
>
> The reason for the 'goto' statement and the suspend_again_count was to
> prevent the suspend function recurse again. In the case if vtpm is in
> the process of being reloaded by firmware, we want to wait for the CRQ
> INITIALIZATION and resubmit suspend message i.e. recurse only once.
>
Never mind.. I don't really save any repetitive code by using recursion
now. I'll rework and resubmit the patch.
Thanks,
Vicky
Re: [PATCH] vTPM: fix missing error handling for suspend operation
On Fri, 2016-03-04 at 18:55 +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 02, 2016 at 01:23:47AM -0500, Hon Ching(Vicky) Lo wrote:
> > ibmvtpm_send_crq in tpm_ibmvtpm_suspend returns errors in a more
> > granular level than what the existing code does. This patch adds
> > the missing CRQ transport event code checks to ensure appropriate
> > action taken, in the case that ibmvtpm_send_crq returns H_CLOSED.
> >
> > Signed-off-by: Hon Ching(Vicky) Lo
> > ---
> > drivers/char/tpm/tpm_ibmvtpm.c | 58
> > +---
> > drivers/char/tpm/tpm_ibmvtpm.h |9 ++
> > 2 files changed, 63 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> > index 3e6a226..5d984af 100644
> > --- a/drivers/char/tpm/tpm_ibmvtpm.c
> > +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> > @@ -335,17 +335,61 @@ static int tpm_ibmvtpm_suspend(struct device *dev)
> > struct ibmvtpm_crq crq;
> > u64 *buf = (u64 *) &crq;
> > int rc = 0;
> > + int sig;
> >
> > - crq.valid = (u8)IBMVTPM_VALID_CMD;
> > - crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
> > + crq_initialized = 0;
> > + crq.valid = (u8) IBMVTPM_VALID_CMD;
> > + crq.msg = (u8) VTPM_PREPARE_TO_SUSPEND;
> >
> > rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
> > cpu_to_be64(buf[1]));
> > +
> > + if ((rc == H_CLOSED) && (crq.valid == (u8) VALID_TRANSPORT_EVENT)) {
>
> What if rc == H_CLOSED and crq.valid != VALID_TRANSPORT_EVENT?
If that's the case, the function will return rc as the execution will
skip this if block.
>
> > + if (crq.msg == (u8) PARTNER_PARTITION_SUSPENDED) {
> > + /* The "partner partition suspended" transport
> > +* event disables the associated CRQ such that
> > +* any H_SEND_CRQ hcall() to the associated CRQ
> > +* returns H_Closed until CRQ has been explicitly
> > +* enabled using the H_ENABLED_CRQ hcall.
> > +*/
> > + return H_SUCCESS;
>
> I'm having trouble to understand when the suspend happens through this
> route and when you just get H_SUCCESS from ibmvtpm_send_crq(). It
> seems that there are two ways how suspend can happen.
>
> I don't understand the big picture.
You're right. This is not a valid case. As I revisited it, I realized
that "partner partition suspended" transport event was handled in the
rtas calls; vtpm doesn't have to take that into account. I'll get rid
of this if-block.
So, upon receiving H_CLOSED only the following two events are expected
to be handled: 1) vtpm has terminated fatally. 2) partner partition
preregistered CRQ.
> > + } else if (crq.msg == (u8) PARTNER_PARTITION_FAILED) {
> > + dev_err(ibmvtpm->dev,
> > + "vtpm has terminated fatally; reboot to
> > reinstate a trusted state.\n");
> > + } else if (crq.msg == (u8) PARTNER_PARTITION_DEREG_CRQ) {
> > + /* The vtpm is in the process of being reloaded by
> > +* firmware and has de-registered CRQ. The client
> > +* must wait for the CRQ INITIALIZATION message and
> > +* respond and must resubmit suspend message.
> > +*/
> > + sig =
> > + wait_event_interruptible(ibmvtpm->wq,
> > +crq_initialized == 1);
> > + if (sig)
> > + return -EINTR;
> > +
> > + if (suspend_again_count < 1) {
> > + suspend_again_count++;
> > + goto suspendagain;
> > + }
> > + } else
> > + ;
> > + }
> > +
> > if (rc != H_SUCCESS)
> > - dev_err(ibmvtpm->dev,
> > - "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > + dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> >
> > return rc;
> > +
> > +suspendagain:
> > + rc = tpm_ibmvtpm_suspend(ibmvtpm->dev);
> > + suspend_again_count = 0;
> > +
> > + if (rc != H_SUCCESS)
> > + dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> > +
> > + return rc;
> > +
>
> Get rid of this horrible looking tail recursion thing.
>
> What the heck is suspend_again_count and why it can be module scope
> variable? You could use a local variable instead if you would iterate
> with a loop.
>
The reason for the 'goto' statement and the suspend_again_count was to
prevent the suspend function recurse again. In the case if vtpm is in
the process of being reloaded by firmware, we want to wait for the CRQ
INITIALIZATION and resubmit suspend message i.e. recurse only once.
> /Jarkko
>
Re: [PATCH] vTPM: fix missing error handling for suspend operation
On Wed, Mar 02, 2016 at 01:23:47AM -0500, Hon Ching(Vicky) Lo wrote:
> ibmvtpm_send_crq in tpm_ibmvtpm_suspend returns errors in a more
> granular level than what the existing code does. This patch adds
> the missing CRQ transport event code checks to ensure appropriate
> action taken, in the case that ibmvtpm_send_crq returns H_CLOSED.
>
> Signed-off-by: Hon Ching(Vicky) Lo
> ---
> drivers/char/tpm/tpm_ibmvtpm.c | 58
> +---
> drivers/char/tpm/tpm_ibmvtpm.h |9 ++
> 2 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 3e6a226..5d984af 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -335,17 +335,61 @@ static int tpm_ibmvtpm_suspend(struct device *dev)
> struct ibmvtpm_crq crq;
> u64 *buf = (u64 *) &crq;
> int rc = 0;
> + int sig;
>
> - crq.valid = (u8)IBMVTPM_VALID_CMD;
> - crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
> + crq_initialized = 0;
> + crq.valid = (u8) IBMVTPM_VALID_CMD;
> + crq.msg = (u8) VTPM_PREPARE_TO_SUSPEND;
>
> rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
> cpu_to_be64(buf[1]));
> +
> + if ((rc == H_CLOSED) && (crq.valid == (u8) VALID_TRANSPORT_EVENT)) {
What if rc == H_CLOSED and crq.valid != VALID_TRANSPORT_EVENT?
> + if (crq.msg == (u8) PARTNER_PARTITION_SUSPENDED) {
> + /* The "partner partition suspended" transport
> + * event disables the associated CRQ such that
> + * any H_SEND_CRQ hcall() to the associated CRQ
> + * returns H_Closed until CRQ has been explicitly
> + * enabled using the H_ENABLED_CRQ hcall.
> + */
> + return H_SUCCESS;
I'm having trouble to understand when the suspend happens through this
route and when you just get H_SUCCESS from ibmvtpm_send_crq(). It
seems that there are two ways how suspend can happen.
I don't understand the big picture.
> + } else if (crq.msg == (u8) PARTNER_PARTITION_FAILED) {
> + dev_err(ibmvtpm->dev,
> + "vtpm has terminated fatally; reboot to
> reinstate a trusted state.\n");
> + } else if (crq.msg == (u8) PARTNER_PARTITION_DEREG_CRQ) {
> + /* The vtpm is in the process of being reloaded by
> + * firmware and has de-registered CRQ. The client
> + * must wait for the CRQ INITIALIZATION message and
> + * respond and must resubmit suspend message.
> + */
> + sig =
> + wait_event_interruptible(ibmvtpm->wq,
> + crq_initialized == 1);
> + if (sig)
> + return -EINTR;
> +
> + if (suspend_again_count < 1) {
> + suspend_again_count++;
> + goto suspendagain;
> + }
> + } else
> + ;
> + }
> +
> if (rc != H_SUCCESS)
> - dev_err(ibmvtpm->dev,
> - "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> + dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
>
> return rc;
> +
> +suspendagain:
> + rc = tpm_ibmvtpm_suspend(ibmvtpm->dev);
> + suspend_again_count = 0;
> +
> + if (rc != H_SUCCESS)
> + dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
> +
> + return rc;
> +
Get rid of this horrible looking tail recursion thing.
What the heck is suspend_again_count and why it can be module scope
variable? You could use a local variable instead if you would iterate
with a loop.
/Jarkko
[PATCH] vTPM: fix missing error handling for suspend operation
ibmvtpm_send_crq in tpm_ibmvtpm_suspend returns errors in a more
granular level than what the existing code does. This patch adds
the missing CRQ transport event code checks to ensure appropriate
action taken, in the case that ibmvtpm_send_crq returns H_CLOSED.
Signed-off-by: Hon Ching(Vicky) Lo
---
drivers/char/tpm/tpm_ibmvtpm.c | 58 +---
drivers/char/tpm/tpm_ibmvtpm.h |9 ++
2 files changed, 63 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 3e6a226..5d984af 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -335,17 +335,61 @@ static int tpm_ibmvtpm_suspend(struct device *dev)
struct ibmvtpm_crq crq;
u64 *buf = (u64 *) &crq;
int rc = 0;
+ int sig;
- crq.valid = (u8)IBMVTPM_VALID_CMD;
- crq.msg = (u8)VTPM_PREPARE_TO_SUSPEND;
+ crq_initialized = 0;
+ crq.valid = (u8) IBMVTPM_VALID_CMD;
+ crq.msg = (u8) VTPM_PREPARE_TO_SUSPEND;
rc = ibmvtpm_send_crq(ibmvtpm->vdev, cpu_to_be64(buf[0]),
cpu_to_be64(buf[1]));
+
+ if ((rc == H_CLOSED) && (crq.valid == (u8) VALID_TRANSPORT_EVENT)) {
+ if (crq.msg == (u8) PARTNER_PARTITION_SUSPENDED) {
+ /* The "partner partition suspended" transport
+* event disables the associated CRQ such that
+* any H_SEND_CRQ hcall() to the associated CRQ
+* returns H_Closed until CRQ has been explicitly
+* enabled using the H_ENABLED_CRQ hcall.
+*/
+ return H_SUCCESS;
+ } else if (crq.msg == (u8) PARTNER_PARTITION_FAILED) {
+ dev_err(ibmvtpm->dev,
+ "vtpm has terminated fatally; reboot to
reinstate a trusted state.\n");
+ } else if (crq.msg == (u8) PARTNER_PARTITION_DEREG_CRQ) {
+ /* The vtpm is in the process of being reloaded by
+* firmware and has de-registered CRQ. The client
+* must wait for the CRQ INITIALIZATION message and
+* respond and must resubmit suspend message.
+*/
+ sig =
+ wait_event_interruptible(ibmvtpm->wq,
+crq_initialized == 1);
+ if (sig)
+ return -EINTR;
+
+ if (suspend_again_count < 1) {
+ suspend_again_count++;
+ goto suspendagain;
+ }
+ } else
+ ;
+ }
+
if (rc != H_SUCCESS)
- dev_err(ibmvtpm->dev,
- "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
+ dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
return rc;
+
+suspendagain:
+ rc = tpm_ibmvtpm_suspend(ibmvtpm->dev);
+ suspend_again_count = 0;
+
+ if (rc != H_SUCCESS)
+ dev_err(ibmvtpm->dev, "tpm_ibmvtpm_suspend failed rc=%d\n", rc);
+
+ return rc;
+
}
/**
@@ -477,6 +521,9 @@ static void ibmvtpm_crq_process(struct ibmvtpm_crq *crq,
case INIT_CRQ_COMP_RES:
dev_info(ibmvtpm->dev,
"CRQ initialization completed\n");
+ /* in case vtpm is being reloaded */
+ crq_initialized = 1;
+ wake_up_interruptible(&ibmvtpm->wq);
return;
default:
dev_err(ibmvtpm->dev, "Unknown crq message type: %d\n",
crq->msg);
@@ -517,6 +564,9 @@ static void ibmvtpm_crq_process(struct ibmvtpm_crq *crq,
ibmvtpm->res_len = be16_to_cpu(crq->len);
wake_up_interruptible(&ibmvtpm->wq);
return;
+ case VTPM_PREPARE_TO_SUSPEND_RES:
+ dev_info(ibmvtpm->dev, "Prepare to Suspend Response\n");
+ return;
default:
return;
}
diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
index 6af9289..1990d3c 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.h
+++ b/drivers/char/tpm/tpm_ibmvtpm.h
@@ -73,4 +73,13 @@ struct ibmvtpm_dev {
#define VTPM_PREPARE_TO_SUSPEND0x04
#define VTPM_PREPARE_TO_SUSPEND_RES(0x04 | VTPM_MSG_RES)
+/* vTPM CRQ Transport Event codes */
+#define VALID_TRANSPORT_EVENT 0xFF
+#define PARTNER_PARTITION_FAILED 0x01
+#define PARTNER_PARTITION_DEREG_CRQ0x02
+#define PARTNER_PARTITION_SUSPENDED0x06
+
+int crq_initialized;
+int suspend_again_count;
+
#endif
--
