diff mbox series

[v4,2/2] tpm: ibmvtpm: Rename tpm_process_cmd to tpm_status and define flag

Message ID 20210809192159.2176580-3-stefanb@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show
Series ibmvtpm: Avoid error message when process gets signal while waiting | expand

Commit Message

Stefan Berger Aug. 9, 2021, 7:21 p.m. UTC 
From: Stefan Berger <stefanb@linux.ibm.com>

Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Nayna Jain <nayna@linux.ibm.com>
Cc: George Wilson <gcwilson@linux.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 15 ++++++++-------
 drivers/char/tpm/tpm_ibmvtpm.h |  3 ++-
 2 files changed, 10 insertions(+), 8 deletions(-)

Comments

Jarkko Sakkinen Aug. 10, 2021, 5:58 p.m. UTC | #1 
On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
> the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Nayna Jain <nayna@linux.ibm.com>
> Cc: George Wilson <gcwilson@linux.ibm.com>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 15 ++++++++-------
>  drivers/char/tpm/tpm_ibmvtpm.h |  3 ++-
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index 7a9eca5768f8..5d795866b483 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -215,11 +215,12 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
>  		return -EIO;
>  	}
>  
> -	if (ibmvtpm->tpm_processing_cmd) {
> +	if ((ibmvtpm->tpm_status & TPM_STATUS_BUSY)) {
>  		dev_info(ibmvtpm->dev,
>  		         "Need to wait for TPM to finish\n");
>  		/* wait for previous command to finish */
> -		sig = wait_event_interruptible(ibmvtpm->wq, !ibmvtpm->tpm_processing_cmd);
> +		sig = wait_event_interruptible(ibmvtpm->wq,
> +				(ibmvtpm->tpm_status & TPM_STATUS_BUSY) == 0);
>  		if (sig)
>  			return -EINTR;
>  	}
> @@ -232,7 +233,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
>  	 * set the processing flag before the Hcall, since we may get the
>  	 * result (interrupt) before even being able to check rc.
>  	 */
> -	ibmvtpm->tpm_processing_cmd = true;
> +	ibmvtpm->tpm_status |= TPM_STATUS_BUSY;
>  
>  again:
>  	rc = ibmvtpm_send_crq(ibmvtpm->vdev,
> @@ -250,7 +251,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
>  			goto again;
>  		}
>  		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_send failed rc=%d\n", rc);
> -		ibmvtpm->tpm_processing_cmd = false;
> +		ibmvtpm->tpm_status &= ~TPM_STATUS_BUSY;
>  	}
>  
>  	spin_unlock(&ibmvtpm->rtce_lock);
> @@ -266,7 +267,7 @@ static u8 tpm_ibmvtpm_status(struct tpm_chip *chip)
>  {
>  	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
>  
> -	return ibmvtpm->tpm_processing_cmd;
> +	return ibmvtpm->tpm_status;
>  }
>  
>  /**
> @@ -454,7 +455,7 @@ static const struct tpm_class_ops tpm_ibmvtpm = {
>  	.send = tpm_ibmvtpm_send,
>  	.cancel = tpm_ibmvtpm_cancel,
>  	.status = tpm_ibmvtpm_status,
> -	.req_complete_mask = true,
> +	.req_complete_mask = TPM_STATUS_BUSY,
>  	.req_complete_val = 0,
>  	.req_canceled = tpm_ibmvtpm_req_canceled,
>  };
> @@ -547,7 +548,7 @@ static void ibmvtpm_crq_process(struct ibmvtpm_crq *crq,
>  		case VTPM_TPM_COMMAND_RES:
>  			/* len of the data in rtce buffer */
>  			ibmvtpm->res_len = be16_to_cpu(crq->len);
> -			ibmvtpm->tpm_processing_cmd = false;
> +			ibmvtpm->tpm_status &= ~TPM_STATUS_BUSY;
>  			wake_up_interruptible(&ibmvtpm->wq);
>  			return;
>  		default:
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
> index 51198b137461..252f1cccdfc5 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.h
> +++ b/drivers/char/tpm/tpm_ibmvtpm.h
> @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
>  	wait_queue_head_t wq;
>  	u16 res_len;
>  	u32 vtpm_version;
> -	u8 tpm_processing_cmd;
> +	u8 tpm_status;
> +#define TPM_STATUS_BUSY		(1 << 0) /* vtpm is processing a command */

Declare this already in the fix, and just leave the rename here.

>  };
>  
>  #define CRQ_RES_BUF_SIZE	PAGE_SIZE
> -- 
> 2.31.1
> 
> 

Otherwise, these look fine.

/Jarkko
Stefan Berger Aug. 11, 2021, 1:50 a.m. UTC | #2 
On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
> On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
>> the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
>>
>>
>>   		default:
>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
>> index 51198b137461..252f1cccdfc5 100644
>> --- a/drivers/char/tpm/tpm_ibmvtpm.h
>> +++ b/drivers/char/tpm/tpm_ibmvtpm.h
>> @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
>>   	wait_queue_head_t wq;
>>   	u16 res_len;
>>   	u32 vtpm_version;
>> -	u8 tpm_processing_cmd;
>> +	u8 tpm_status;
>> +#define TPM_STATUS_BUSY		(1 << 0) /* vtpm is processing a command */
> Declare this already in the fix, and just leave the rename here.

You mean the fix patch does not use 'true' anymore but uses the 
TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? 
And literally only the renaming of this field is done in the 2nd patch?


    Stefan


>
>>   };
>>   
>>   #define CRQ_RES_BUF_SIZE	PAGE_SIZE
>> -- 
>> 2.31.1
>>
>>
> Otherwise, these look fine.
>
> /Jarkko
Jarkko Sakkinen Aug. 11, 2021, 2:10 a.m. UTC | #3 
On Tue, Aug 10, 2021 at 09:50:55PM -0400, Stefan Berger wrote:
> 
> On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
> > On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
> > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > 
> > > Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
> > > the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
> > > 
> > > 
> > >   		default:
> > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
> > > index 51198b137461..252f1cccdfc5 100644
> > > --- a/drivers/char/tpm/tpm_ibmvtpm.h
> > > +++ b/drivers/char/tpm/tpm_ibmvtpm.h
> > > @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
> > >   	wait_queue_head_t wq;
> > >   	u16 res_len;
> > >   	u32 vtpm_version;
> > > -	u8 tpm_processing_cmd;
> > > +	u8 tpm_status;
> > > +#define TPM_STATUS_BUSY		(1 << 0) /* vtpm is processing a command */
> > Declare this already in the fix, and just leave the rename here.
> 
> You mean the fix patch does not use 'true' anymore but uses the
> TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? And
> literally only the renaming of this field is done in the 2nd patch?

I can fixup these patches, and use '1', instead of true. No need to send
new ones.

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko
Stefan Berger Aug. 11, 2021, 12:15 p.m. UTC | #4 
On 8/10/21 10:10 PM, Jarkko Sakkinen wrote:
> On Tue, Aug 10, 2021 at 09:50:55PM -0400, Stefan Berger wrote:
>> On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
>>> On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
>>>> the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
>>>>
>>>>
>>>>    		default:
>>>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
>>>> index 51198b137461..252f1cccdfc5 100644
>>>> --- a/drivers/char/tpm/tpm_ibmvtpm.h
>>>> +++ b/drivers/char/tpm/tpm_ibmvtpm.h
>>>> @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
>>>>    	wait_queue_head_t wq;
>>>>    	u16 res_len;
>>>>    	u32 vtpm_version;
>>>> -	u8 tpm_processing_cmd;
>>>> +	u8 tpm_status;
>>>> +#define TPM_STATUS_BUSY		(1 << 0) /* vtpm is processing a command */
>>> Declare this already in the fix, and just leave the rename here.
>> You mean the fix patch does not use 'true' anymore but uses the
>> TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? And
>> literally only the renaming of this field is done in the 2nd patch?
> I can fixup these patches, and use '1', instead of true. No need to send
> new ones.
>
> Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

THANKS!

   Stefan

>
> /Jarkko
Jarkko Sakkinen Aug. 12, 2021, 7:48 p.m. UTC | #5 
On Wed, Aug 11, 2021 at 08:15:14AM -0400, Stefan Berger wrote:
> 
> On 8/10/21 10:10 PM, Jarkko Sakkinen wrote:
> > On Tue, Aug 10, 2021 at 09:50:55PM -0400, Stefan Berger wrote:
> > > On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
> > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > > 
> > > > > Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
> > > > > the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
> > > > > 
> > > > > 
> > > > >    		default:
> > > > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
> > > > > index 51198b137461..252f1cccdfc5 100644
> > > > > --- a/drivers/char/tpm/tpm_ibmvtpm.h
> > > > > +++ b/drivers/char/tpm/tpm_ibmvtpm.h
> > > > > @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
> > > > >    	wait_queue_head_t wq;
> > > > >    	u16 res_len;
> > > > >    	u32 vtpm_version;
> > > > > -	u8 tpm_processing_cmd;
> > > > > +	u8 tpm_status;
> > > > > +#define TPM_STATUS_BUSY		(1 << 0) /* vtpm is processing a command */
> > > > Declare this already in the fix, and just leave the rename here.
> > > You mean the fix patch does not use 'true' anymore but uses the
> > > TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? And
> > > literally only the renaming of this field is done in the 2nd patch?
> > I can fixup these patches, and use '1', instead of true. No need to send
> > new ones.
> > 
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

I applied the first. If you have only one flag that you even
document as "processing the command" in the inline comment,
it makes absolutely no sense to rename it, as the current
name perfectly documents what it exactly is.

/Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 7a9eca5768f8..5d795866b483 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -215,11 +215,12 @@  static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 		return -EIO;
 	}
 
-	if (ibmvtpm->tpm_processing_cmd) {
+	if ((ibmvtpm->tpm_status & TPM_STATUS_BUSY)) {
 		dev_info(ibmvtpm->dev,
 		         "Need to wait for TPM to finish\n");
 		/* wait for previous command to finish */
-		sig = wait_event_interruptible(ibmvtpm->wq, !ibmvtpm->tpm_processing_cmd);
+		sig = wait_event_interruptible(ibmvtpm->wq,
+				(ibmvtpm->tpm_status & TPM_STATUS_BUSY) == 0);
 		if (sig)
 			return -EINTR;
 	}
@@ -232,7 +233,7 @@  static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 	 * set the processing flag before the Hcall, since we may get the
 	 * result (interrupt) before even being able to check rc.
 	 */
-	ibmvtpm->tpm_processing_cmd = true;
+	ibmvtpm->tpm_status |= TPM_STATUS_BUSY;
 
 again:
 	rc = ibmvtpm_send_crq(ibmvtpm->vdev,
@@ -250,7 +251,7 @@  static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 			goto again;
 		}
 		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_send failed rc=%d\n", rc);
-		ibmvtpm->tpm_processing_cmd = false;
+		ibmvtpm->tpm_status &= ~TPM_STATUS_BUSY;
 	}
 
 	spin_unlock(&ibmvtpm->rtce_lock);
@@ -266,7 +267,7 @@  static u8 tpm_ibmvtpm_status(struct tpm_chip *chip)
 {
 	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
 
-	return ibmvtpm->tpm_processing_cmd;
+	return ibmvtpm->tpm_status;
 }
 
 /**
@@ -454,7 +455,7 @@  static const struct tpm_class_ops tpm_ibmvtpm = {
 	.send = tpm_ibmvtpm_send,
 	.cancel = tpm_ibmvtpm_cancel,
 	.status = tpm_ibmvtpm_status,
-	.req_complete_mask = true,
+	.req_complete_mask = TPM_STATUS_BUSY,
 	.req_complete_val = 0,
 	.req_canceled = tpm_ibmvtpm_req_canceled,
 };
@@ -547,7 +548,7 @@  static void ibmvtpm_crq_process(struct ibmvtpm_crq *crq,
 		case VTPM_TPM_COMMAND_RES:
 			/* len of the data in rtce buffer */
 			ibmvtpm->res_len = be16_to_cpu(crq->len);
-			ibmvtpm->tpm_processing_cmd = false;
+			ibmvtpm->tpm_status &= ~TPM_STATUS_BUSY;
 			wake_up_interruptible(&ibmvtpm->wq);
 			return;
 		default:
diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
index 51198b137461..252f1cccdfc5 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.h
+++ b/drivers/char/tpm/tpm_ibmvtpm.h
@@ -41,7 +41,8 @@  struct ibmvtpm_dev {
 	wait_queue_head_t wq;
 	u16 res_len;
 	u32 vtpm_version;
-	u8 tpm_processing_cmd;
+	u8 tpm_status;
+#define TPM_STATUS_BUSY		(1 << 0) /* vtpm is processing a command */
 };
 
 #define CRQ_RES_BUF_SIZE	PAGE_SIZE