[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Tomas Winkler]

TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation
of crypto keys which can be a computationally intensive task.
The timeout is set to 3min.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-interface.c |  4 ++++
 drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
 drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 85bdfa8c3348..c0aa9d11ec7a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
 		chip->duration[TPM_LONG] =
 		    msecs_to_jiffies(TPM2_DURATION_LONG);
+		chip->duration[TPM_LONG_LONG] =
+		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
+		chip->duration[TPM_UNDEFINED] =
+		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
 
 		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
 		return 0;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f895fba4e20d..192ba68b39c2 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -67,7 +67,9 @@ enum tpm_duration {
 	TPM_SHORT = 0,
 	TPM_MEDIUM = 1,
 	TPM_LONG = 2,
-	TPM_UNDEFINED,
+	TPM_LONG_LONG = 3,
+	TPM_UNDEFINED = 4,
+	TPM_DURATION_MAX,
 };
 
 #define TPM_WARN_RETRY          0x800
@@ -79,15 +81,17 @@ enum tpm_duration {
 #define TPM_HEADER_SIZE		10
 
 enum tpm2_const {
-	TPM2_PLATFORM_PCR	= 24,
-	TPM2_PCR_SELECT_MIN	= ((TPM2_PLATFORM_PCR + 7) / 8),
-	TPM2_TIMEOUT_A		= 750,
-	TPM2_TIMEOUT_B		= 2000,
-	TPM2_TIMEOUT_C		= 200,
-	TPM2_TIMEOUT_D		= 30,
-	TPM2_DURATION_SHORT	= 20,
-	TPM2_DURATION_MEDIUM	= 750,
-	TPM2_DURATION_LONG	= 2000,
+	TPM2_PLATFORM_PCR       =     24,
+	TPM2_PCR_SELECT_MIN     = ((TPM2_PLATFORM_PCR + 7) / 8),
+	TPM2_TIMEOUT_A          =    750,
+	TPM2_TIMEOUT_B          =   2000,
+	TPM2_TIMEOUT_C          =    200,
+	TPM2_TIMEOUT_D          =     30,
+	TPM2_DURATION_SHORT     =     20,
+	TPM2_DURATION_MEDIUM    =    750,
+	TPM2_DURATION_LONG      =   2000,
+	TPM2_DURATION_LONG_LONG = 300000,
+	TPM2_DURATION_DEFAULT   = 120000,
 };
 
 enum tpm2_structures {
@@ -123,6 +127,7 @@ enum tpm2_algorithms {
 
 enum tpm2_command_codes {
 	TPM2_CC_FIRST		= 0x011F,
+	TPM2_CC_CREATE_PRIMARY  = 0x0131,
 	TPM2_CC_SELF_TEST	= 0x0143,
 	TPM2_CC_STARTUP		= 0x0144,
 	TPM2_CC_SHUTDOWN	= 0x0145,
@@ -227,7 +232,7 @@ struct tpm_chip {
 	unsigned long timeout_c; /* jiffies */
 	unsigned long timeout_d; /* jiffies */
 	bool timeout_adjusted;
-	unsigned long duration[3]; /* jiffies */
+	unsigned long duration[TPM_DURATION_MAX]; /* jiffies */
 	bool duration_adjusted;
 
 	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a700f8f9ead7..c1ddbbba406e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -90,6 +90,8 @@ static struct tpm2_hash tpm2_hash_map[] = {
  * of time the chip could take to return the result. The values
  * of the SHORT, MEDIUM, and LONG durations are taken from the
  * PC Client Profile (PTP) specification.
+ * LONG_LONG is for commands that generates keys which empirically
+ * takes longer time on some systems.
  */
 static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 11F */
@@ -110,7 +112,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 12e */
 	TPM_UNDEFINED,		/* 12f */
 	TPM_UNDEFINED,		/* 130 */
-	TPM_UNDEFINED,		/* 131 */
+	TPM_LONG_LONG,		/* 131 */
 	TPM_UNDEFINED,		/* 132 */
 	TPM_UNDEFINED,		/* 133 */
 	TPM_UNDEFINED,		/* 134 */
@@ -144,7 +146,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 150 */
 	TPM_UNDEFINED,		/* 151 */
 	TPM_UNDEFINED,		/* 152 */
-	TPM_UNDEFINED,		/* 153 */
+	TPM_LONG_LONG,		/* 153 */
 	TPM_UNDEFINED,		/* 154 */
 	TPM_UNDEFINED,		/* 155 */
 	TPM_UNDEFINED,		/* 156 */
@@ -821,7 +823,7 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 		duration = chip->duration[index];
 
 	if (duration <= 0)
-		duration = 2 * 60 * HZ;
+		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
 
 	return duration;
 }
-- 
2.14.3

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Tomas Winkler]

TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation
of crypto keys which can be a computationally intensive task.
The timeout is set to 3min.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm-interface.c |  4 ++++
 drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
 drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 85bdfa8c3348..c0aa9d11ec7a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
 		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
 		chip->duration[TPM_LONG] =
 		    msecs_to_jiffies(TPM2_DURATION_LONG);
+		chip->duration[TPM_LONG_LONG] =
+		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
+		chip->duration[TPM_UNDEFINED] =
+		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
 
 		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
 		return 0;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f895fba4e20d..192ba68b39c2 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -67,7 +67,9 @@ enum tpm_duration {
 	TPM_SHORT = 0,
 	TPM_MEDIUM = 1,
 	TPM_LONG = 2,
-	TPM_UNDEFINED,
+	TPM_LONG_LONG = 3,
+	TPM_UNDEFINED = 4,
+	TPM_DURATION_MAX,
 };
 
 #define TPM_WARN_RETRY          0x800
@@ -79,15 +81,17 @@ enum tpm_duration {
 #define TPM_HEADER_SIZE		10
 
 enum tpm2_const {
-	TPM2_PLATFORM_PCR	= 24,
-	TPM2_PCR_SELECT_MIN	= ((TPM2_PLATFORM_PCR + 7) / 8),
-	TPM2_TIMEOUT_A		= 750,
-	TPM2_TIMEOUT_B		= 2000,
-	TPM2_TIMEOUT_C		= 200,
-	TPM2_TIMEOUT_D		= 30,
-	TPM2_DURATION_SHORT	= 20,
-	TPM2_DURATION_MEDIUM	= 750,
-	TPM2_DURATION_LONG	= 2000,
+	TPM2_PLATFORM_PCR       =     24,
+	TPM2_PCR_SELECT_MIN     = ((TPM2_PLATFORM_PCR + 7) / 8),
+	TPM2_TIMEOUT_A          =    750,
+	TPM2_TIMEOUT_B          =   2000,
+	TPM2_TIMEOUT_C          =    200,
+	TPM2_TIMEOUT_D          =     30,
+	TPM2_DURATION_SHORT     =     20,
+	TPM2_DURATION_MEDIUM    =    750,
+	TPM2_DURATION_LONG      =   2000,
+	TPM2_DURATION_LONG_LONG = 300000,
+	TPM2_DURATION_DEFAULT   = 120000,
 };
 
 enum tpm2_structures {
@@ -123,6 +127,7 @@ enum tpm2_algorithms {
 
 enum tpm2_command_codes {
 	TPM2_CC_FIRST		= 0x011F,
+	TPM2_CC_CREATE_PRIMARY  = 0x0131,
 	TPM2_CC_SELF_TEST	= 0x0143,
 	TPM2_CC_STARTUP		= 0x0144,
 	TPM2_CC_SHUTDOWN	= 0x0145,
@@ -227,7 +232,7 @@ struct tpm_chip {
 	unsigned long timeout_c; /* jiffies */
 	unsigned long timeout_d; /* jiffies */
 	bool timeout_adjusted;
-	unsigned long duration[3]; /* jiffies */
+	unsigned long duration[TPM_DURATION_MAX]; /* jiffies */
 	bool duration_adjusted;
 
 	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index a700f8f9ead7..c1ddbbba406e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -90,6 +90,8 @@ static struct tpm2_hash tpm2_hash_map[] = {
  * of time the chip could take to return the result. The values
  * of the SHORT, MEDIUM, and LONG durations are taken from the
  * PC Client Profile (PTP) specification.
+ * LONG_LONG is for commands that generates keys which empirically
+ * takes longer time on some systems.
  */
 static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 11F */
@@ -110,7 +112,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 12e */
 	TPM_UNDEFINED,		/* 12f */
 	TPM_UNDEFINED,		/* 130 */
-	TPM_UNDEFINED,		/* 131 */
+	TPM_LONG_LONG,		/* 131 */
 	TPM_UNDEFINED,		/* 132 */
 	TPM_UNDEFINED,		/* 133 */
 	TPM_UNDEFINED,		/* 134 */
@@ -144,7 +146,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED,		/* 150 */
 	TPM_UNDEFINED,		/* 151 */
 	TPM_UNDEFINED,		/* 152 */
-	TPM_UNDEFINED,		/* 153 */
+	TPM_LONG_LONG,		/* 153 */
 	TPM_UNDEFINED,		/* 154 */
 	TPM_UNDEFINED,		/* 155 */
 	TPM_UNDEFINED,		/* 156 */
@@ -821,7 +823,7 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 		duration = chip->duration[index];
 
 	if (duration <= 0)
-		duration = 2 * 60 * HZ;
+		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
 
 	return duration;
 }
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] tpm: add new tpm2 commands according to TCG 1.36

[Tomas Winkler]

1. TPM2_CC_LAST has moved from 182 to 193
2. Convert tpm2_ordinal_duration from an array into a switch statement,
   as there are not so many commands that require special duration
   relative to a number of commands.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm.h      |  41 ++++++----
 drivers/char/tpm/tpm2-cmd.c | 188 +++++++++++++++-----------------------------
 2 files changed, 90 insertions(+), 139 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 192ba68b39c2..b82b924d763c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -126,22 +126,31 @@ enum tpm2_algorithms {
 };
 
 enum tpm2_command_codes {
-	TPM2_CC_FIRST		= 0x011F,
-	TPM2_CC_CREATE_PRIMARY  = 0x0131,
-	TPM2_CC_SELF_TEST	= 0x0143,
-	TPM2_CC_STARTUP		= 0x0144,
-	TPM2_CC_SHUTDOWN	= 0x0145,
-	TPM2_CC_CREATE		= 0x0153,
-	TPM2_CC_LOAD		= 0x0157,
-	TPM2_CC_UNSEAL		= 0x015E,
-	TPM2_CC_CONTEXT_LOAD	= 0x0161,
-	TPM2_CC_CONTEXT_SAVE	= 0x0162,
-	TPM2_CC_FLUSH_CONTEXT	= 0x0165,
-	TPM2_CC_GET_CAPABILITY	= 0x017A,
-	TPM2_CC_GET_RANDOM	= 0x017B,
-	TPM2_CC_PCR_READ	= 0x017E,
-	TPM2_CC_PCR_EXTEND	= 0x0182,
-	TPM2_CC_LAST		= 0x018F,
+	TPM2_CC_FIRST		        = 0x011F,
+	TPM2_CC_HIERARCHY_CONTROL       = 0x0121,
+	TPM2_CC_HIERARCHY_CHANGE_AUTH   = 0x0129,
+	TPM2_CC_CREATE_PRIMARY          = 0x0131,
+	TPM2_CC_SEQUENCE_COMPLETE       = 0x013E,
+	TPM2_CC_SELF_TEST	        = 0x0143,
+	TPM2_CC_STARTUP		        = 0x0144,
+	TPM2_CC_SHUTDOWN	        = 0x0145,
+	TPM2_CC_NV_READ                 = 0x014E,
+	TPM2_CC_CREATE		        = 0x0153,
+	TPM2_CC_LOAD		        = 0x0157,
+	TPM2_CC_SEQUENCE_UPDATE         = 0x015C,
+	TPM2_CC_UNSEAL		        = 0x015E,
+	TPM2_CC_CONTEXT_LOAD	        = 0x0161,
+	TPM2_CC_CONTEXT_SAVE	        = 0x0162,
+	TPM2_CC_FLUSH_CONTEXT	        = 0x0165,
+	TPM2_CC_VERIFY_SIGNATURE        = 0x0177,
+	TPM2_CC_GET_CAPABILITY	        = 0x017A,
+	TPM2_CC_GET_RANDOM	        = 0x017B,
+	TPM2_CC_PCR_READ	        = 0x017E,
+	TPM2_CC_PCR_EXTEND	        = 0x0182,
+	TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185,
+	TPM2_CC_HASH_SEQUENCE_START     = 0x0186,
+	TPM2_CC_CREATE_LOADED           = 0x0191,
+	TPM2_CC_LAST		        = 0x0193, /* Spec 1.36 */
 };
 
 enum tpm2_permanent_handles {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c1ddbbba406e..9d2cc8b1daca 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -86,128 +86,73 @@ static struct tpm2_hash tpm2_hash_map[] = {
 };
 
 /*
- * Array with one entry per ordinal defining the maximum amount
+ * tpm2_ordinal_duration returns the maximum amount
  * of time the chip could take to return the result. The values
- * of the SHORT, MEDIUM, and LONG durations are taken from the
- * PC Client Profile (PTP) specification.
+ * of the MEDIUM, and LONG durations are taken from the
+ * PC Client Profile (PTP) specification (750, 2000 msec)
+ *
  * LONG_LONG is for commands that generates keys which empirically
  * takes longer time on some systems.
  */
-static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
-	TPM_UNDEFINED,		/* 11F */
-	TPM_UNDEFINED,		/* 120 */
-	TPM_LONG,		/* 121 */
-	TPM_UNDEFINED,		/* 122 */
-	TPM_UNDEFINED,		/* 123 */
-	TPM_UNDEFINED,		/* 124 */
-	TPM_UNDEFINED,		/* 125 */
-	TPM_UNDEFINED,		/* 126 */
-	TPM_UNDEFINED,		/* 127 */
-	TPM_UNDEFINED,		/* 128 */
-	TPM_LONG,		/* 129 */
-	TPM_UNDEFINED,		/* 12a */
-	TPM_UNDEFINED,		/* 12b */
-	TPM_UNDEFINED,		/* 12c */
-	TPM_UNDEFINED,		/* 12d */
-	TPM_UNDEFINED,		/* 12e */
-	TPM_UNDEFINED,		/* 12f */
-	TPM_UNDEFINED,		/* 130 */
-	TPM_LONG_LONG,		/* 131 */
-	TPM_UNDEFINED,		/* 132 */
-	TPM_UNDEFINED,		/* 133 */
-	TPM_UNDEFINED,		/* 134 */
-	TPM_UNDEFINED,		/* 135 */
-	TPM_UNDEFINED,		/* 136 */
-	TPM_UNDEFINED,		/* 137 */
-	TPM_UNDEFINED,		/* 138 */
-	TPM_UNDEFINED,		/* 139 */
-	TPM_UNDEFINED,		/* 13a */
-	TPM_UNDEFINED,		/* 13b */
-	TPM_UNDEFINED,		/* 13c */
-	TPM_UNDEFINED,		/* 13d */
-	TPM_MEDIUM,		/* 13e */
-	TPM_UNDEFINED,		/* 13f */
-	TPM_UNDEFINED,		/* 140 */
-	TPM_UNDEFINED,		/* 141 */
-	TPM_UNDEFINED,		/* 142 */
-	TPM_LONG,		/* 143 */
-	TPM_MEDIUM,		/* 144 */
-	TPM_UNDEFINED,		/* 145 */
-	TPM_UNDEFINED,		/* 146 */
-	TPM_UNDEFINED,		/* 147 */
-	TPM_UNDEFINED,		/* 148 */
-	TPM_UNDEFINED,		/* 149 */
-	TPM_UNDEFINED,		/* 14a */
-	TPM_UNDEFINED,		/* 14b */
-	TPM_UNDEFINED,		/* 14c */
-	TPM_UNDEFINED,		/* 14d */
-	TPM_LONG,		/* 14e */
-	TPM_UNDEFINED,		/* 14f */
-	TPM_UNDEFINED,		/* 150 */
-	TPM_UNDEFINED,		/* 151 */
-	TPM_UNDEFINED,		/* 152 */
-	TPM_LONG_LONG,		/* 153 */
-	TPM_UNDEFINED,		/* 154 */
-	TPM_UNDEFINED,		/* 155 */
-	TPM_UNDEFINED,		/* 156 */
-	TPM_UNDEFINED,		/* 157 */
-	TPM_UNDEFINED,		/* 158 */
-	TPM_UNDEFINED,		/* 159 */
-	TPM_UNDEFINED,		/* 15a */
-	TPM_UNDEFINED,		/* 15b */
-	TPM_MEDIUM,		/* 15c */
-	TPM_UNDEFINED,		/* 15d */
-	TPM_UNDEFINED,		/* 15e */
-	TPM_UNDEFINED,		/* 15f */
-	TPM_UNDEFINED,		/* 160 */
-	TPM_UNDEFINED,		/* 161 */
-	TPM_UNDEFINED,		/* 162 */
-	TPM_UNDEFINED,		/* 163 */
-	TPM_UNDEFINED,		/* 164 */
-	TPM_UNDEFINED,		/* 165 */
-	TPM_UNDEFINED,		/* 166 */
-	TPM_UNDEFINED,		/* 167 */
-	TPM_UNDEFINED,		/* 168 */
-	TPM_UNDEFINED,		/* 169 */
-	TPM_UNDEFINED,		/* 16a */
-	TPM_UNDEFINED,		/* 16b */
-	TPM_UNDEFINED,		/* 16c */
-	TPM_UNDEFINED,		/* 16d */
-	TPM_UNDEFINED,		/* 16e */
-	TPM_UNDEFINED,		/* 16f */
-	TPM_UNDEFINED,		/* 170 */
-	TPM_UNDEFINED,		/* 171 */
-	TPM_UNDEFINED,		/* 172 */
-	TPM_UNDEFINED,		/* 173 */
-	TPM_UNDEFINED,		/* 174 */
-	TPM_UNDEFINED,		/* 175 */
-	TPM_UNDEFINED,		/* 176 */
-	TPM_LONG,		/* 177 */
-	TPM_UNDEFINED,		/* 178 */
-	TPM_UNDEFINED,		/* 179 */
-	TPM_MEDIUM,		/* 17a */
-	TPM_LONG,		/* 17b */
-	TPM_UNDEFINED,		/* 17c */
-	TPM_UNDEFINED,		/* 17d */
-	TPM_UNDEFINED,		/* 17e */
-	TPM_UNDEFINED,		/* 17f */
-	TPM_UNDEFINED,		/* 180 */
-	TPM_UNDEFINED,		/* 181 */
-	TPM_MEDIUM,		/* 182 */
-	TPM_UNDEFINED,		/* 183 */
-	TPM_UNDEFINED,		/* 184 */
-	TPM_MEDIUM,		/* 185 */
-	TPM_MEDIUM,		/* 186 */
-	TPM_UNDEFINED,		/* 187 */
-	TPM_UNDEFINED,		/* 188 */
-	TPM_UNDEFINED,		/* 189 */
-	TPM_UNDEFINED,		/* 18a */
-	TPM_UNDEFINED,		/* 18b */
-	TPM_UNDEFINED,		/* 18c */
-	TPM_UNDEFINED,		/* 18d */
-	TPM_UNDEFINED,		/* 18e */
-	TPM_UNDEFINED		/* 18f */
-};
+static u8 tpm2_ordinal_duration(u32 ordinal)
+{
+	switch (ordinal) {
+	/* Startup */
+	case TPM2_CC_STARTUP:                 /* 144 */
+		return TPM_MEDIUM;
+
+	/* Selftest */
+	case TPM2_CC_SELF_TEST:               /* 143 */
+		return TPM_LONG;
+
+	/* Random Number Generator */
+	case TPM2_CC_GET_RANDOM:              /* 17B */
+		return TPM_LONG;
+
+	/* Hash/HMAC/Event Sequences */
+	case TPM2_CC_SEQUENCE_UPDATE:         /* 15C */
+		return TPM_MEDIUM;
+	case TPM2_CC_SEQUENCE_COMPLETE:       /* 13E */
+		return TPM_MEDIUM;
+	case TPM2_CC_EVENT_SEQUENCE_COMPLETE: /* 185 */
+		return TPM_MEDIUM;
+	case TPM2_CC_HASH_SEQUENCE_START:     /* 186 */
+		return TPM_MEDIUM;
+
+	/* Signature Verification */
+	case TPM2_CC_VERIFY_SIGNATURE:        /* 177 */
+		return TPM_LONG;
+
+	/* Integrity Collection (PCR) */
+	case TPM2_CC_PCR_EXTEND:              /* 182 */
+		return TPM_MEDIUM;
+
+	/* Hierarchy Commands */
+	case TPM2_CC_HIERARCHY_CONTROL:       /* 121 */
+		return TPM_LONG;
+	case TPM2_CC_HIERARCHY_CHANGE_AUTH:   /* 129 */
+		return TPM_LONG;
+
+	/* Capability Commands */
+	case TPM2_CC_GET_CAPABILITY:          /* 17A */
+		return TPM_MEDIUM;
+
+	/* Non-volatile Storage */
+	case TPM2_CC_NV_READ:                 /* 14E */
+		return TPM_LONG;
+
+	/* Key generation (not in PTP) */
+	case TPM2_CC_CREATE_PRIMARY:          /* 131 */
+		return TPM_LONG_LONG;
+	case TPM2_CC_CREATE:                  /* 153 */
+		return TPM_LONG_LONG;
+	case TPM2_CC_CREATE_LOADED:           /* 191 */
+		return TPM_LONG_LONG;
+
+	default:
+		return TPM_UNDEFINED;
+	}
+}
 
 struct tpm2_pcr_read_out {
 	__be32	update_cnt;
@@ -816,11 +761,8 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 	int index = TPM_UNDEFINED;
 	int duration = 0;
 
-	if (ordinal >= TPM2_CC_FIRST && ordinal <= TPM2_CC_LAST)
-		index = tpm2_ordinal_duration[ordinal - TPM2_CC_FIRST];
-
-	if (index != TPM_UNDEFINED)
-		duration = chip->duration[index];
+	index = tpm2_ordinal_duration(ordinal);
+	duration = chip->duration[index];
 
 	if (duration <= 0)
 		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
-- 
2.14.3

[PATCH 2/3] tpm: add new tpm2 commands according to TCG 1.36

[Tomas Winkler]

1. TPM2_CC_LAST has moved from 182 to 193
2. Convert tpm2_ordinal_duration from an array into a switch statement,
   as there are not so many commands that require special duration
   relative to a number of commands.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm.h      |  41 ++++++----
 drivers/char/tpm/tpm2-cmd.c | 188 +++++++++++++++-----------------------------
 2 files changed, 90 insertions(+), 139 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 192ba68b39c2..b82b924d763c 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -126,22 +126,31 @@ enum tpm2_algorithms {
 };
 
 enum tpm2_command_codes {
-	TPM2_CC_FIRST		= 0x011F,
-	TPM2_CC_CREATE_PRIMARY  = 0x0131,
-	TPM2_CC_SELF_TEST	= 0x0143,
-	TPM2_CC_STARTUP		= 0x0144,
-	TPM2_CC_SHUTDOWN	= 0x0145,
-	TPM2_CC_CREATE		= 0x0153,
-	TPM2_CC_LOAD		= 0x0157,
-	TPM2_CC_UNSEAL		= 0x015E,
-	TPM2_CC_CONTEXT_LOAD	= 0x0161,
-	TPM2_CC_CONTEXT_SAVE	= 0x0162,
-	TPM2_CC_FLUSH_CONTEXT	= 0x0165,
-	TPM2_CC_GET_CAPABILITY	= 0x017A,
-	TPM2_CC_GET_RANDOM	= 0x017B,
-	TPM2_CC_PCR_READ	= 0x017E,
-	TPM2_CC_PCR_EXTEND	= 0x0182,
-	TPM2_CC_LAST		= 0x018F,
+	TPM2_CC_FIRST		        = 0x011F,
+	TPM2_CC_HIERARCHY_CONTROL       = 0x0121,
+	TPM2_CC_HIERARCHY_CHANGE_AUTH   = 0x0129,
+	TPM2_CC_CREATE_PRIMARY          = 0x0131,
+	TPM2_CC_SEQUENCE_COMPLETE       = 0x013E,
+	TPM2_CC_SELF_TEST	        = 0x0143,
+	TPM2_CC_STARTUP		        = 0x0144,
+	TPM2_CC_SHUTDOWN	        = 0x0145,
+	TPM2_CC_NV_READ                 = 0x014E,
+	TPM2_CC_CREATE		        = 0x0153,
+	TPM2_CC_LOAD		        = 0x0157,
+	TPM2_CC_SEQUENCE_UPDATE         = 0x015C,
+	TPM2_CC_UNSEAL		        = 0x015E,
+	TPM2_CC_CONTEXT_LOAD	        = 0x0161,
+	TPM2_CC_CONTEXT_SAVE	        = 0x0162,
+	TPM2_CC_FLUSH_CONTEXT	        = 0x0165,
+	TPM2_CC_VERIFY_SIGNATURE        = 0x0177,
+	TPM2_CC_GET_CAPABILITY	        = 0x017A,
+	TPM2_CC_GET_RANDOM	        = 0x017B,
+	TPM2_CC_PCR_READ	        = 0x017E,
+	TPM2_CC_PCR_EXTEND	        = 0x0182,
+	TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185,
+	TPM2_CC_HASH_SEQUENCE_START     = 0x0186,
+	TPM2_CC_CREATE_LOADED           = 0x0191,
+	TPM2_CC_LAST		        = 0x0193, /* Spec 1.36 */
 };
 
 enum tpm2_permanent_handles {
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c1ddbbba406e..9d2cc8b1daca 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -86,128 +86,73 @@ static struct tpm2_hash tpm2_hash_map[] = {
 };
 
 /*
- * Array with one entry per ordinal defining the maximum amount
+ * tpm2_ordinal_duration returns the maximum amount
  * of time the chip could take to return the result. The values
- * of the SHORT, MEDIUM, and LONG durations are taken from the
- * PC Client Profile (PTP) specification.
+ * of the MEDIUM, and LONG durations are taken from the
+ * PC Client Profile (PTP) specification (750, 2000 msec)
+ *
  * LONG_LONG is for commands that generates keys which empirically
  * takes longer time on some systems.
  */
-static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
-	TPM_UNDEFINED,		/* 11F */
-	TPM_UNDEFINED,		/* 120 */
-	TPM_LONG,		/* 121 */
-	TPM_UNDEFINED,		/* 122 */
-	TPM_UNDEFINED,		/* 123 */
-	TPM_UNDEFINED,		/* 124 */
-	TPM_UNDEFINED,		/* 125 */
-	TPM_UNDEFINED,		/* 126 */
-	TPM_UNDEFINED,		/* 127 */
-	TPM_UNDEFINED,		/* 128 */
-	TPM_LONG,		/* 129 */
-	TPM_UNDEFINED,		/* 12a */
-	TPM_UNDEFINED,		/* 12b */
-	TPM_UNDEFINED,		/* 12c */
-	TPM_UNDEFINED,		/* 12d */
-	TPM_UNDEFINED,		/* 12e */
-	TPM_UNDEFINED,		/* 12f */
-	TPM_UNDEFINED,		/* 130 */
-	TPM_LONG_LONG,		/* 131 */
-	TPM_UNDEFINED,		/* 132 */
-	TPM_UNDEFINED,		/* 133 */
-	TPM_UNDEFINED,		/* 134 */
-	TPM_UNDEFINED,		/* 135 */
-	TPM_UNDEFINED,		/* 136 */
-	TPM_UNDEFINED,		/* 137 */
-	TPM_UNDEFINED,		/* 138 */
-	TPM_UNDEFINED,		/* 139 */
-	TPM_UNDEFINED,		/* 13a */
-	TPM_UNDEFINED,		/* 13b */
-	TPM_UNDEFINED,		/* 13c */
-	TPM_UNDEFINED,		/* 13d */
-	TPM_MEDIUM,		/* 13e */
-	TPM_UNDEFINED,		/* 13f */
-	TPM_UNDEFINED,		/* 140 */
-	TPM_UNDEFINED,		/* 141 */
-	TPM_UNDEFINED,		/* 142 */
-	TPM_LONG,		/* 143 */
-	TPM_MEDIUM,		/* 144 */
-	TPM_UNDEFINED,		/* 145 */
-	TPM_UNDEFINED,		/* 146 */
-	TPM_UNDEFINED,		/* 147 */
-	TPM_UNDEFINED,		/* 148 */
-	TPM_UNDEFINED,		/* 149 */
-	TPM_UNDEFINED,		/* 14a */
-	TPM_UNDEFINED,		/* 14b */
-	TPM_UNDEFINED,		/* 14c */
-	TPM_UNDEFINED,		/* 14d */
-	TPM_LONG,		/* 14e */
-	TPM_UNDEFINED,		/* 14f */
-	TPM_UNDEFINED,		/* 150 */
-	TPM_UNDEFINED,		/* 151 */
-	TPM_UNDEFINED,		/* 152 */
-	TPM_LONG_LONG,		/* 153 */
-	TPM_UNDEFINED,		/* 154 */
-	TPM_UNDEFINED,		/* 155 */
-	TPM_UNDEFINED,		/* 156 */
-	TPM_UNDEFINED,		/* 157 */
-	TPM_UNDEFINED,		/* 158 */
-	TPM_UNDEFINED,		/* 159 */
-	TPM_UNDEFINED,		/* 15a */
-	TPM_UNDEFINED,		/* 15b */
-	TPM_MEDIUM,		/* 15c */
-	TPM_UNDEFINED,		/* 15d */
-	TPM_UNDEFINED,		/* 15e */
-	TPM_UNDEFINED,		/* 15f */
-	TPM_UNDEFINED,		/* 160 */
-	TPM_UNDEFINED,		/* 161 */
-	TPM_UNDEFINED,		/* 162 */
-	TPM_UNDEFINED,		/* 163 */
-	TPM_UNDEFINED,		/* 164 */
-	TPM_UNDEFINED,		/* 165 */
-	TPM_UNDEFINED,		/* 166 */
-	TPM_UNDEFINED,		/* 167 */
-	TPM_UNDEFINED,		/* 168 */
-	TPM_UNDEFINED,		/* 169 */
-	TPM_UNDEFINED,		/* 16a */
-	TPM_UNDEFINED,		/* 16b */
-	TPM_UNDEFINED,		/* 16c */
-	TPM_UNDEFINED,		/* 16d */
-	TPM_UNDEFINED,		/* 16e */
-	TPM_UNDEFINED,		/* 16f */
-	TPM_UNDEFINED,		/* 170 */
-	TPM_UNDEFINED,		/* 171 */
-	TPM_UNDEFINED,		/* 172 */
-	TPM_UNDEFINED,		/* 173 */
-	TPM_UNDEFINED,		/* 174 */
-	TPM_UNDEFINED,		/* 175 */
-	TPM_UNDEFINED,		/* 176 */
-	TPM_LONG,		/* 177 */
-	TPM_UNDEFINED,		/* 178 */
-	TPM_UNDEFINED,		/* 179 */
-	TPM_MEDIUM,		/* 17a */
-	TPM_LONG,		/* 17b */
-	TPM_UNDEFINED,		/* 17c */
-	TPM_UNDEFINED,		/* 17d */
-	TPM_UNDEFINED,		/* 17e */
-	TPM_UNDEFINED,		/* 17f */
-	TPM_UNDEFINED,		/* 180 */
-	TPM_UNDEFINED,		/* 181 */
-	TPM_MEDIUM,		/* 182 */
-	TPM_UNDEFINED,		/* 183 */
-	TPM_UNDEFINED,		/* 184 */
-	TPM_MEDIUM,		/* 185 */
-	TPM_MEDIUM,		/* 186 */
-	TPM_UNDEFINED,		/* 187 */
-	TPM_UNDEFINED,		/* 188 */
-	TPM_UNDEFINED,		/* 189 */
-	TPM_UNDEFINED,		/* 18a */
-	TPM_UNDEFINED,		/* 18b */
-	TPM_UNDEFINED,		/* 18c */
-	TPM_UNDEFINED,		/* 18d */
-	TPM_UNDEFINED,		/* 18e */
-	TPM_UNDEFINED		/* 18f */
-};
+static u8 tpm2_ordinal_duration(u32 ordinal)
+{
+	switch (ordinal) {
+	/* Startup */
+	case TPM2_CC_STARTUP:                 /* 144 */
+		return TPM_MEDIUM;
+
+	/* Selftest */
+	case TPM2_CC_SELF_TEST:               /* 143 */
+		return TPM_LONG;
+
+	/* Random Number Generator */
+	case TPM2_CC_GET_RANDOM:              /* 17B */
+		return TPM_LONG;
+
+	/* Hash/HMAC/Event Sequences */
+	case TPM2_CC_SEQUENCE_UPDATE:         /* 15C */
+		return TPM_MEDIUM;
+	case TPM2_CC_SEQUENCE_COMPLETE:       /* 13E */
+		return TPM_MEDIUM;
+	case TPM2_CC_EVENT_SEQUENCE_COMPLETE: /* 185 */
+		return TPM_MEDIUM;
+	case TPM2_CC_HASH_SEQUENCE_START:     /* 186 */
+		return TPM_MEDIUM;
+
+	/* Signature Verification */
+	case TPM2_CC_VERIFY_SIGNATURE:        /* 177 */
+		return TPM_LONG;
+
+	/* Integrity Collection (PCR) */
+	case TPM2_CC_PCR_EXTEND:              /* 182 */
+		return TPM_MEDIUM;
+
+	/* Hierarchy Commands */
+	case TPM2_CC_HIERARCHY_CONTROL:       /* 121 */
+		return TPM_LONG;
+	case TPM2_CC_HIERARCHY_CHANGE_AUTH:   /* 129 */
+		return TPM_LONG;
+
+	/* Capability Commands */
+	case TPM2_CC_GET_CAPABILITY:          /* 17A */
+		return TPM_MEDIUM;
+
+	/* Non-volatile Storage */
+	case TPM2_CC_NV_READ:                 /* 14E */
+		return TPM_LONG;
+
+	/* Key generation (not in PTP) */
+	case TPM2_CC_CREATE_PRIMARY:          /* 131 */
+		return TPM_LONG_LONG;
+	case TPM2_CC_CREATE:                  /* 153 */
+		return TPM_LONG_LONG;
+	case TPM2_CC_CREATE_LOADED:           /* 191 */
+		return TPM_LONG_LONG;
+
+	default:
+		return TPM_UNDEFINED;
+	}
+}
 
 struct tpm2_pcr_read_out {
 	__be32	update_cnt;
@@ -816,11 +761,8 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 	int index = TPM_UNDEFINED;
 	int duration = 0;
 
-	if (ordinal >= TPM2_CC_FIRST && ordinal <= TPM2_CC_LAST)
-		index = tpm2_ordinal_duration[ordinal - TPM2_CC_FIRST];
-
-	if (index != TPM_UNDEFINED)
-		duration = chip->duration[index];
+	index = tpm2_ordinal_duration(ordinal);
+	duration = chip->duration[index];
 
 	if (duration <= 0)
 		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] tpm: add new tpm2 commands according to TCG 1.36

[Jarkko Sakkinen]

On Sun, Mar 04, 2018 at 02:12:04PM +0200, Tomas Winkler wrote:
> 1. TPM2_CC_LAST has moved from 182 to 193
> 2. Convert tpm2_ordinal_duration from an array into a switch statement,
>    as there are not so many commands that require special duration
>    relative to a number of commands.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

My previous suggestion applies here.

/Jarkko

[PATCH 2/3] tpm: add new tpm2 commands according to TCG 1.36

[Jarkko Sakkinen]

On Sun, Mar 04, 2018 at 02:12:04PM +0200, Tomas Winkler wrote:
> 1. TPM2_CC_LAST has moved from 182 to 193
> 2. Convert tpm2_ordinal_duration from an array into a switch statement,
>    as there are not so many commands that require special duration
>    relative to a number of commands.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

My previous suggestion applies here.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Tomas Winkler]

This suppresses sparse warning
drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm_crb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index acfcdc6f31af..29fecdea0b2d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -489,6 +489,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
+	__le64 __rsp_pa;
 	u64 rsp_pa;
 	u32 rsp_size;
 	int ret;
@@ -554,8 +555,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		goto out;
 	}
 
-	memcpy_fromio(&rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
-	rsp_pa = le64_to_cpu(rsp_pa);
+	memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
+	rsp_pa = le64_to_cpu(__rsp_pa);
 	rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
 				      ioread32(&priv->regs_t->ctrl_rsp_size));
 
-- 
2.14.3

[PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Tomas Winkler]

This suppresses sparse warning
drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/char/tpm/tpm_crb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index acfcdc6f31af..29fecdea0b2d 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -489,6 +489,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
+	__le64 __rsp_pa;
 	u64 rsp_pa;
 	u32 rsp_size;
 	int ret;
@@ -554,8 +555,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 		goto out;
 	}
 
-	memcpy_fromio(&rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
-	rsp_pa = le64_to_cpu(rsp_pa);
+	memcpy_fromio(&__rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
+	rsp_pa = le64_to_cpu(__rsp_pa);
 	rsp_size = crb_fixup_cmd_size(dev, &io_res, rsp_pa,
 				      ioread32(&priv->regs_t->ctrl_rsp_size));
 
-- 
2.14.3

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Jarkko Sakkinen]

On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> This suppresses sparse warning
> drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

The guideline is that you should describe what is wrong rather than
copy-paste the sparse message.

/Jarkko

[PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Jarkko Sakkinen]

On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> This suppresses sparse warning
> drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

The guideline is that you should describe what is wrong rather than
copy-paste the sparse message.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > This suppresses sparse warning
> > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> The guideline is that you should describe what is wrong rather than
> copy-paste the sparse message.

Jason, didn't yo give the feedback to some patch 1-2 years ago that
instead of copy-pasting parse error one should write a clear commit
msg or is this OK?

/Jarkko

[PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > This suppresses sparse warning
> > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  drivers/char/tpm/tpm_crb.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> The guideline is that you should describe what is wrong rather than
> copy-paste the sparse message.

Jason, didn't yo give the feedback to some patch 1-2 years ago that
instead of copy-pasting parse error one should write a clear commit
msg or is this OK?

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Winkler, Tomas]

> On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > > This suppresses sparse warning
> > > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted
> > > __le64
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > >  drivers/char/tpm/tpm_crb.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > The guideline is that you should describe what is wrong rather than
> > copy-paste the sparse message.
> 
> Jason, didn't yo give the feedback to some patch 1-2 years ago that instead
> of copy-pasting parse error one should write a clear commit msg or is this
> OK?

I think you are reading wrongly the rule,  the title explains the issue and in addition 
I'm adding exact sparse warning. this is usually required. 
What is wrong is putting something like 'Fix sparse error' or "Fix warning' into patch subject. 
So the imperative here is 'adding annotation' and not a 'fixing a sparse message'.

Thanks
Tomas

[PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Winkler, Tomas]

> On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > > This suppresses sparse warning
> > > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted
> > > __le64
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > >  drivers/char/tpm/tpm_crb.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > The guideline is that you should describe what is wrong rather than
> > copy-paste the sparse message.
> 
> Jason, didn't yo give the feedback to some patch 1-2 years ago that instead
> of copy-pasting parse error one should write a clear commit msg or is this
> OK?

I think you are reading wrongly the rule,  the title explains the issue and in addition 
I'm adding exact sparse warning. this is usually required. 
What is wrong is putting something like 'Fix sparse error' or "Fix warning' into patch subject. 
So the imperative here is 'adding annotation' and not a 'fixing a sparse message'.

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Jason Gunthorpe]

On Tue, Mar 06, 2018 at 10:28:21AM +0200, Jarkko Sakkinen wrote:
> On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > > This suppresses sparse warning
> > > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> > > 
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > >  drivers/char/tpm/tpm_crb.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > The guideline is that you should describe what is wrong rather than
> > copy-paste the sparse message.
> 
> Jason, didn't yo give the feedback to some patch 1-2 years ago that
> instead of copy-pasting parse error one should write a clear commit
> msg or is this OK?

The standard is to give some explaination why the tool complaint is
valid and then if suitable include the tool complaint itself.

Eg bad:

Fix sparse warning on resp

Good:

use __le64 annotated variable for response buffer address

IMHO, the subject line sufficiently describes the patch, and it is
generally OK to clip the tool warning into the body..

Jason

[PATCH 3/3] tpm_crb: use __le64 annotated variable for response buffer address

[Jason Gunthorpe]

On Tue, Mar 06, 2018 at 10:28:21AM +0200, Jarkko Sakkinen wrote:
> On Mon, Mar 05, 2018 at 03:03:20PM +0200, Jarkko Sakkinen wrote:
> > On Sun, Mar 04, 2018 at 02:12:05PM +0200, Tomas Winkler wrote:
> > > This suppresses sparse warning
> > > drivers/char/tpm/tpm_crb.c:558:18: warning: cast to restricted __le64
> > > 
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > >  drivers/char/tpm/tpm_crb.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > The guideline is that you should describe what is wrong rather than
> > copy-paste the sparse message.
> 
> Jason, didn't yo give the feedback to some patch 1-2 years ago that
> instead of copy-pasting parse error one should write a clear commit
> msg or is this OK?

The standard is to give some explaination why the tool complaint is
valid and then if suitable include the tool complaint itself.

Eg bad:

Fix sparse warning on resp

Good:

use __le64 annotated variable for response buffer address

IMHO, the subject line sufficiently describes the patch, and it is
generally OK to clip the tool warning into the body..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Sun, Mar 04, 2018 at 02:12:03PM +0200, Tomas Winkler wrote:
> TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation
> of crypto keys which can be a computationally intensive task.
> The timeout is set to 3min.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Where is the cover letter? Please send separate patches if they are
unrelated *or* add a cover letter that describes what they do as a
whole.

I will not review the next version if it does not have cover letter
describing the high level change and containing the change log.

> ---
>  drivers/char/tpm/tpm-interface.c |  4 ++++
>  drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
>  drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
>  3 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 85bdfa8c3348..c0aa9d11ec7a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
>  		chip->duration[TPM_LONG] =
>  		    msecs_to_jiffies(TPM2_DURATION_LONG);
> +		chip->duration[TPM_LONG_LONG] =
> +		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
> +		chip->duration[TPM_UNDEFINED] =
> +		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
>  
>  		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
>  		return 0;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f895fba4e20d..192ba68b39c2 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -67,7 +67,9 @@ enum tpm_duration {
>  	TPM_SHORT = 0,
>  	TPM_MEDIUM = 1,
>  	TPM_LONG = 2,
> -	TPM_UNDEFINED,
> +	TPM_LONG_LONG = 3,
> +	TPM_UNDEFINED = 4,
> +	TPM_DURATION_MAX,

This is starting to rotten to become unmaintainable.

Here is what I suggest to move forward:

* Have essentially two duration types:
  1. Default
  2. Long
  'default' is the old long duration i.e. two seconds. 'long' is a

We should probably have two durations:

enum tpm_duration {
	TPM_DURATION_DEFAULT = 2000,
	TPM_DURATION_LONG = 300000,
};

These would be both for TPM 1.2 and TPM 2.0. Instead of having
table for every ordinal there should be a small tables describing
commands that require long timeout.

> -		duration = 2 * 60 * HZ;
> +		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);

NAK for this change.

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Sun, Mar 04, 2018 at 02:12:03PM +0200, Tomas Winkler wrote:
> TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation
> of crypto keys which can be a computationally intensive task.
> The timeout is set to 3min.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Where is the cover letter? Please send separate patches if they are
unrelated *or* add a cover letter that describes what they do as a
whole.

I will not review the next version if it does not have cover letter
describing the high level change and containing the change log.

> ---
>  drivers/char/tpm/tpm-interface.c |  4 ++++
>  drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
>  drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
>  3 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 85bdfa8c3348..c0aa9d11ec7a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
>  		chip->duration[TPM_LONG] =
>  		    msecs_to_jiffies(TPM2_DURATION_LONG);
> +		chip->duration[TPM_LONG_LONG] =
> +		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
> +		chip->duration[TPM_UNDEFINED] =
> +		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
>  
>  		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
>  		return 0;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f895fba4e20d..192ba68b39c2 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -67,7 +67,9 @@ enum tpm_duration {
>  	TPM_SHORT = 0,
>  	TPM_MEDIUM = 1,
>  	TPM_LONG = 2,
> -	TPM_UNDEFINED,
> +	TPM_LONG_LONG = 3,
> +	TPM_UNDEFINED = 4,
> +	TPM_DURATION_MAX,

This is starting to rotten to become unmaintainable.

Here is what I suggest to move forward:

* Have essentially two duration types:
  1. Default
  2. Long
  'default' is the old long duration i.e. two seconds. 'long' is a

We should probably have two durations:

enum tpm_duration {
	TPM_DURATION_DEFAULT = 2000,
	TPM_DURATION_LONG = 300000,
};

These would be both for TPM 1.2 and TPM 2.0. Instead of having
table for every ordinal there should be a small tables describing
commands that require long timeout.

> -		duration = 2 * 60 * HZ;
> +		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);

NAK for this change.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com]
> Sent: Monday, March 05, 2018 14:57
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; linux-integrity@vger.kernel.org; linux-
> security-module@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation
> commands.
> 
> On Sun, Mar 04, 2018 at 02:12:03PM +0200, Tomas Winkler wrote:
> > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve
> > generation of crypto keys which can be a computationally intensive task.
> > The timeout is set to 3min.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> Where is the cover letter? Please send separate patches if they are unrelated
> *or* add a cover letter that describes what they do as a whole.
>
Why you need cover letter?  What are u missing in the patch description
 
> I will not review the next version if it does not have cover letter describing
> the high level change and containing the change log.

Don't follow.

> 
> > ---
> >  drivers/char/tpm/tpm-interface.c |  4 ++++
> >  drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
> >  drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
> >  3 files changed, 25 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 85bdfa8c3348..c0aa9d11ec7a 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
> >  		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> >  		chip->duration[TPM_LONG] =
> >  		    msecs_to_jiffies(TPM2_DURATION_LONG);
> > +		chip->duration[TPM_LONG_LONG] =
> > +		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
> > +		chip->duration[TPM_UNDEFINED] =
> > +		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
> >
> >  		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
> >  		return 0;
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index
> > f895fba4e20d..192ba68b39c2 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -67,7 +67,9 @@ enum tpm_duration {
> >  	TPM_SHORT = 0,
> >  	TPM_MEDIUM = 1,
> >  	TPM_LONG = 2,
> > -	TPM_UNDEFINED,
> > +	TPM_LONG_LONG = 3,
> > +	TPM_UNDEFINED = 4,
> > +	TPM_DURATION_MAX,
> 
> This is starting to rotten to become unmaintainable.

> Here is what I suggest to move forward:

I fixed that in next patch, but this also moves the code to new spec,  so I didn't want to make too much noise in this one. 
 
> * Have essentially two duration types:
>   1. Default
>   2. Long
>   'default' is the old long duration i.e. two seconds. 'long' is a

> 
> We should probably have two durations:
> 
> enum tpm_duration {
> 	TPM_DURATION_DEFAULT = 2000,
> 	TPM_DURATION_LONG = 300000,
> };
> 
How is this aligned with the spec PTP spec?

> These would be both for TPM 1.2 and TPM 2.0. Instead of having table for
> every ordinal there should be a small tables describing commands that
> require long timeout.

Yeah I didn't cover the 1.2. 
> 
> > -		duration = 2 * 60 * HZ;
> > +		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
> 
> NAK for this change.


You should explain your NAKs, .... in general, doesn't look good.


Thanks
Tomas

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen at linux.intel.com]
> Sent: Monday, March 05, 2018 14:57
> To: Winkler, Tomas <tomas.winkler@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; linux-integrity at vger.kernel.org; linux-
> security-module at vger.kernel.org; linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation
> commands.
> 
> On Sun, Mar 04, 2018 at 02:12:03PM +0200, Tomas Winkler wrote:
> > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve
> > generation of crypto keys which can be a computationally intensive task.
> > The timeout is set to 3min.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> 
> Where is the cover letter? Please send separate patches if they are unrelated
> *or* add a cover letter that describes what they do as a whole.
>
Why you need cover letter?  What are u missing in the patch description
 
> I will not review the next version if it does not have cover letter describing
> the high level change and containing the change log.

Don't follow.

> 
> > ---
> >  drivers/char/tpm/tpm-interface.c |  4 ++++
> >  drivers/char/tpm/tpm.h           | 27 ++++++++++++++++-----------
> >  drivers/char/tpm/tpm2-cmd.c      |  8 +++++---
> >  3 files changed, 25 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 85bdfa8c3348..c0aa9d11ec7a 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip)
> >  		    msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> >  		chip->duration[TPM_LONG] =
> >  		    msecs_to_jiffies(TPM2_DURATION_LONG);
> > +		chip->duration[TPM_LONG_LONG] =
> > +		    msecs_to_jiffies(TPM2_DURATION_LONG_LONG);
> > +		chip->duration[TPM_UNDEFINED] =
> > +		    msecs_to_jiffies(TPM2_DURATION_DEFAULT);
> >
> >  		chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS;
> >  		return 0;
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index
> > f895fba4e20d..192ba68b39c2 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -67,7 +67,9 @@ enum tpm_duration {
> >  	TPM_SHORT = 0,
> >  	TPM_MEDIUM = 1,
> >  	TPM_LONG = 2,
> > -	TPM_UNDEFINED,
> > +	TPM_LONG_LONG = 3,
> > +	TPM_UNDEFINED = 4,
> > +	TPM_DURATION_MAX,
> 
> This is starting to rotten to become unmaintainable.

> Here is what I suggest to move forward:

I fixed that in next patch, but this also moves the code to new spec,  so I didn't want to make too much noise in this one. 
 
> * Have essentially two duration types:
>   1. Default
>   2. Long
>   'default' is the old long duration i.e. two seconds. 'long' is a

> 
> We should probably have two durations:
> 
> enum tpm_duration {
> 	TPM_DURATION_DEFAULT = 2000,
> 	TPM_DURATION_LONG = 300000,
> };
> 
How is this aligned with the spec PTP spec?

> These would be both for TPM 1.2 and TPM 2.0. Instead of having table for
> every ordinal there should be a small tables describing commands that
> require long timeout.

Yeah I didn't cover the 1.2. 
> 
> > -		duration = 2 * 60 * HZ;
> > +		duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT);
> 
> NAK for this change.


You should explain your NAKs, .... in general, doesn't look good.


Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > enum tpm_duration {
> > 	TPM_DURATION_DEFAULT = 2000,
> > 	TPM_DURATION_LONG = 300000,
> > };
> > 
> How is this aligned with the spec PTP spec?

For TPM 2.0 that spec only partially defines durations for CCs and thus
our look up table is already kind "flakky". In a sense that the default
duration is upper limit for spec defined durations.

> > These would be both for TPM 1.2 and TPM 2.0. Instead of having table for
> > every ordinal there should be a small tables describing commands that
> > require long timeout.
> 
> Yeah I didn't cover the 1.2. 

I could probably help with TPM 1.2 changes if required.

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > enum tpm_duration {
> > 	TPM_DURATION_DEFAULT = 2000,
> > 	TPM_DURATION_LONG = 300000,
> > };
> > 
> How is this aligned with the spec PTP spec?

For TPM 2.0 that spec only partially defines durations for CCs and thus
our look up table is already kind "flakky". In a sense that the default
duration is upper limit for spec defined durations.

> > These would be both for TPM 1.2 and TPM 2.0. Instead of having table for
> > every ordinal there should be a small tables describing commands that
> > require long timeout.
> 
> Yeah I didn't cover the 1.2. 

I could probably help with TPM 1.2 changes if required.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > enum tpm_duration {
> > > 	TPM_DURATION_DEFAULT = 2000,
> > > 	TPM_DURATION_LONG = 300000,
> > > };
> > >
> > How is this aligned with the spec PTP spec?
> 
> For TPM 2.0 that spec only partially defines durations for CCs and thus our
> look up table is already kind "flakky". In a sense that the default duration is
> upper limit for spec defined durations.

The timeouts for LONG and MEDIUM is defined by the  PTP spec,  we need to maintain those as those effect the system.
The UNDEFINED and LONG LONG is the implementation choice we driver from empirical data we have so far.

> 
> > > These would be both for TPM 1.2 and TPM 2.0. Instead of having table
> > > for every ordinal there should be a small tables describing commands
> > > that require long timeout.
> >
> > Yeah I didn't cover the 1.2.
> 
> I could probably help with TPM 1.2 changes if required.

In middle of it, will send for review in few.

Thanks
Tomas

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > enum tpm_duration {
> > > 	TPM_DURATION_DEFAULT = 2000,
> > > 	TPM_DURATION_LONG = 300000,
> > > };
> > >
> > How is this aligned with the spec PTP spec?
> 
> For TPM 2.0 that spec only partially defines durations for CCs and thus our
> look up table is already kind "flakky". In a sense that the default duration is
> upper limit for spec defined durations.

The timeouts for LONG and MEDIUM is defined by the  PTP spec,  we need to maintain those as those effect the system.
The UNDEFINED and LONG LONG is the implementation choice we driver from empirical data we have so far.

> 
> > > These would be both for TPM 1.2 and TPM 2.0. Instead of having table
> > > for every ordinal there should be a small tables describing commands
> > > that require long timeout.
> >
> > Yeah I didn't cover the 1.2.
> 
> I could probably help with TPM 1.2 changes if required.

In middle of it, will send for review in few.

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 06:04:56PM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > enum tpm_duration {
> > > > 	TPM_DURATION_DEFAULT = 2000,
> > > > 	TPM_DURATION_LONG = 300000,
> > > > };
> > > >
> > > How is this aligned with the spec PTP spec?
> > 
> > For TPM 2.0 that spec only partially defines durations for CCs and thus our
> > look up table is already kind "flakky". In a sense that the default duration is
> > upper limit for spec defined durations.
> 
> The timeouts for LONG and MEDIUM is defined by the  PTP spec,  we need to maintain those as those effect the system.
> The UNDEFINED and LONG LONG is the implementation choice we driver from empirical data we have so far.

Where can be get this empirical data?

You are not only adding 30s delay but also turning the 2s delay to 12s
delay.

IMHO we could very well use PTP LONG for all commands as the timeout.
Why that would not work?

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 06:04:56PM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > enum tpm_duration {
> > > > 	TPM_DURATION_DEFAULT = 2000,
> > > > 	TPM_DURATION_LONG = 300000,
> > > > };
> > > >
> > > How is this aligned with the spec PTP spec?
> > 
> > For TPM 2.0 that spec only partially defines durations for CCs and thus our
> > look up table is already kind "flakky". In a sense that the default duration is
> > upper limit for spec defined durations.
> 
> The timeouts for LONG and MEDIUM is defined by the  PTP spec,  we need to maintain those as those effect the system.
> The UNDEFINED and LONG LONG is the implementation choice we driver from empirical data we have so far.

Where can be get this empirical data?

You are not only adding 30s delay but also turning the 2s delay to 12s
delay.

IMHO we could very well use PTP LONG for all commands as the timeout.
Why that would not work?

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Mon, Mar 05, 2018 at 06:04:56PM +0000, Winkler, Tomas wrote:
> > >
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > enum tpm_duration {
> > > > > 	TPM_DURATION_DEFAULT = 2000,
> > > > > 	TPM_DURATION_LONG = 300000,
> > > > > };
> > > > >
> > > > How is this aligned with the spec PTP spec?
> > >
> > > For TPM 2.0 that spec only partially defines durations for CCs and
> > > thus our look up table is already kind "flakky". In a sense that the
> > > default duration is upper limit for spec defined durations.
> >
> > The timeouts for LONG and MEDIUM is defined by the  PTP spec,  we need
> to maintain those as those effect the system.
> > The UNDEFINED and LONG LONG is the implementation choice we driver
> from empirical data we have so far.
> 
> Where can be get this empirical data?
>From testing the HW.
> 
> You are not only adding 30s delay but also turning the 2s delay to 12s delay.


I'm adding 3 min, no other changes.  Where is 12s?

> IMHO we could very well use PTP LONG for all commands as the timeout.
> Why that would not work?


Empirically it doesn't go test it you have the HW.

Thanks
Tomas

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Mon, Mar 05, 2018 at 06:04:56PM +0000, Winkler, Tomas wrote:
> > >
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > enum tpm_duration {
> > > > > 	TPM_DURATION_DEFAULT = 2000,
> > > > > 	TPM_DURATION_LONG = 300000,
> > > > > };
> > > > >
> > > > How is this aligned with the spec PTP spec?
> > >
> > > For TPM 2.0 that spec only partially defines durations for CCs and
> > > thus our look up table is already kind "flakky". In a sense that the
> > > default duration is upper limit for spec defined durations.
> >
> > The timeouts for LONG and MEDIUM is defined by the  PTP spec,  we need
> to maintain those as those effect the system.
> > The UNDEFINED and LONG LONG is the implementation choice we driver
> from empirical data we have so far.
> 
> Where can be get this empirical data?
>From testing the HW.
> 
> You are not only adding 30s delay but also turning the 2s delay to 12s delay.


I'm adding 3 min, no other changes.  Where is 12s?

> IMHO we could very well use PTP LONG for all commands as the timeout.
> Why that would not work?


Empirically it doesn't go test it you have the HW.

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> Why you need cover letter?  What are u missing in the patch description

If you submit a *patch set* I *require* a cover letter, yes.

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> Why you need cover letter?  What are u missing in the patch description

If you submit a *patch set* I *require* a cover letter, yes.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > Why you need cover letter?  What are u missing in the patch description
> 
> If you submit a *patch set* I *require* a cover letter, yes.

It's good but it is not must, you are inventing your own rules.

Thanks
Tomas

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > Why you need cover letter?  What are u missing in the patch description
> 
> If you submit a *patch set* I *require* a cover letter, yes.

It's good but it is not must, you are inventing your own rules.

Thanks
Tomas

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[James Bottomley]

On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > 
> > 
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > 
> > > Why you need cover letter?  What are u missing in the patch
> > > description
> > 
> > If you submit a *patch set* I *require* a cover letter, yes.
> 
> It's good but it is not must, you are inventing your own rules.

As long as the Maintainer is the gatekeeper, you're not going to get
very far with this argument.  The fact is that a lot of subsystems have
varying rules; often undocumented, some of which are even in conflict,
like alphabetic vs reverse christmas tree format for includes.

A cover letter is actually one of the more uniform rules.  It's
referred to in submitting patches, but not actually documented there.

James

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[James Bottomley]

On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > 
> > 
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > 
> > > Why you need cover letter?  What are u missing in the patch
> > > description
> > 
> > If you submit a *patch set* I *require* a cover letter, yes.
> 
> It's good but it is not must, you are inventing your own rules.

As long as the Maintainer is the gatekeeper, you're not going to get
very far with this argument.  The fact is that a lot of subsystems have
varying rules; often undocumented, some of which are even in conflict,
like alphabetic vs reverse christmas tree format for includes.

A cover letter is actually one of the more uniform rules.  It's
referred to in submitting patches, but not actually documented there.

James

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[James Bottomley]

On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > 
> > 
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > 
> > > Why you need cover letter???What are u missing in the patch
> > > description
> > 
> > If you submit a *patch set* I *require* a cover letter, yes.
> 
> It's good but it is not must, you are inventing your own rules.

As long as the Maintainer is the gatekeeper, you're not going to get
very far with this argument. ?The fact is that a lot of subsystems have
varying rules; often undocumented, some of which are even in conflict,
like alphabetic vs reverse christmas tree format for includes.

A cover letter is actually one of the more uniform rules. ?It's
referred to in submitting patches, but not actually documented there.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > >
> > >
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > >
> > > > Why you need cover letter?  What are u missing in the patch
> > > > description
> > >
> > > If you submit a *patch set* I *require* a cover letter, yes.
> >
> > It's good but it is not must, you are inventing your own rules.
> 
> As long as the Maintainer is the gatekeeper, you're not going to get very far
> with this argument.  The fact is that a lot of subsystems have varying rules;
> often undocumented, some of which are even in conflict, like alphabetic vs
> reverse christmas tree format for includes.

Usually I'm trying to stay in convention of the surouding code even if it's agains my personal taste.
 I think this particular case was a bit different. 
 
> A cover letter is actually one of the more uniform rules.  It's referred to in
> submitting patches, but not actually documented there.

I'm all for cover letters,  but for few code line fixes it's more comments then code.

What is most important I think I and Jarkko had found finally a common ground.

Thanks
Tomas

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > >
> > >
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > >
> > > > Why you need cover letter?  What are u missing in the patch
> > > > description
> > >
> > > If you submit a *patch set* I *require* a cover letter, yes.
> >
> > It's good but it is not must, you are inventing your own rules.
> 
> As long as the Maintainer is the gatekeeper, you're not going to get very far
> with this argument.  The fact is that a lot of subsystems have varying rules;
> often undocumented, some of which are even in conflict, like alphabetic vs
> reverse christmas tree format for includes.

Usually I'm trying to stay in convention of the surouding code even if it's agains my personal taste.
 I think this particular case was a bit different. 
 
> A cover letter is actually one of the more uniform rules.  It's referred to in
> submitting patches, but not actually documented there.

I'm all for cover letters,  but for few code line fixes it's more comments then code.

What is most important I think I and Jarkko had found finally a common ground.

Thanks
Tomas

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > >
> > >
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > >
> > > > Why you need cover letter?????What are u missing in the patch
> > > > description
> > >
> > > If you submit a *patch set* I *require* a cover letter, yes.
> >
> > It's good but it is not must, you are inventing your own rules.
> 
> As long as the Maintainer is the gatekeeper, you're not going to get very far
> with this argument. ??The fact is that a lot of subsystems have varying rules;
> often undocumented, some of which are even in conflict, like alphabetic vs
> reverse christmas tree format for includes.

Usually I'm trying to stay in convention of the surouding code even if it's agains my personal taste.
 I think this particular case was a bit different. 
 
> A cover letter is actually one of the more uniform rules. ??It's referred to in
> submitting patches, but not actually documented there.

I'm all for cover letters,  but for few code line fixes it's more comments then code.

What is most important I think I and Jarkko had found finally a common ground.

Thanks
Tomas



????{.n?+???????+%???????\x17??w??{.n?+????{??????????v?^?)????w*\x1fjg???\x1e???????j??\a??G??????\f???j:+v???w?j?m?????\x1e??\x1e?w?????f???h?????????

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > 
> > > 
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > 
> > > > Why you need cover letter?  What are u missing in the patch
> > > > description
> > > 
> > > If you submit a *patch set* I *require* a cover letter, yes.
> > 
> > It's good but it is not must, you are inventing your own rules.
> 
> As long as the Maintainer is the gatekeeper, you're not going to get
> very far with this argument.  The fact is that a lot of subsystems have
> varying rules; often undocumented, some of which are even in conflict,
> like alphabetic vs reverse christmas tree format for includes.
> 
> A cover letter is actually one of the more uniform rules.  It's
> referred to in submitting patches, but not actually documented there.

I've heard that some maintainers are moving away from cover letters,
since they are not include in the git repo and are lost.  I've seen
Andrew Morton cut and paste the cover letter in the first patch
description of the patch set.

Mimi

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > 
> > > 
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > 
> > > > Why you need cover letter?  What are u missing in the patch
> > > > description
> > > 
> > > If you submit a *patch set* I *require* a cover letter, yes.
> > 
> > It's good but it is not must, you are inventing your own rules.
> 
> As long as the Maintainer is the gatekeeper, you're not going to get
> very far with this argument.  The fact is that a lot of subsystems have
> varying rules; often undocumented, some of which are even in conflict,
> like alphabetic vs reverse christmas tree format for includes.
> 
> A cover letter is actually one of the more uniform rules.  It's
> referred to in submitting patches, but not actually documented there.

I've heard that some maintainers are moving away from cover letters,
since they are not include in the git repo and are lost.  I've seen
Andrew Morton cut and paste the cover letter in the first patch
description of the patch set.

Mimi

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > 
> > > 
> > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > 
> > > > Why you need cover letter???What are u missing in the patch
> > > > description
> > > 
> > > If you submit a *patch set* I *require* a cover letter, yes.
> > 
> > It's good but it is not must, you are inventing your own rules.
> 
> As long as the Maintainer is the gatekeeper, you're not going to get
> very far with this argument. ?The fact is that a lot of subsystems have
> varying rules; often undocumented, some of which are even in conflict,
> like alphabetic vs reverse christmas tree format for includes.
> 
> A cover letter is actually one of the more uniform rules. ?It's
> referred to in submitting patches, but not actually documented there.

I've heard that some maintainers are moving away from cover letters,
since they are not include in the git repo and are lost. ?I've seen
Andrew Morton cut and paste the cover letter in the first patch
description of the patch set.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jason Gunthorpe]

On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > 
> > > > 
> > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > 
> > > > > Why you need cover letter?  What are u missing in the patch
> > > > > description
> > > > 
> > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > 
> > > It's good but it is not must, you are inventing your own rules.
> > 
> > As long as the Maintainer is the gatekeeper, you're not going to get
> > very far with this argument.  The fact is that a lot of subsystems have
> > varying rules; often undocumented, some of which are even in conflict,
> > like alphabetic vs reverse christmas tree format for includes.
> > 
> > A cover letter is actually one of the more uniform rules.  It's
> > referred to in submitting patches, but not actually documented there.
> 
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.  I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

Andrew has a workflow unlike any other I've seen..

In my view the cover letter should explain why the maintainer should
apply the series, and give any guidance to the review process.

Not duplicate information that belongs in the patch comments. It
shouldn't explain how/why the patch(es) works, etc.

Jason

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jason Gunthorpe]

On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > 
> > > > 
> > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > 
> > > > > Why you need cover letter?  What are u missing in the patch
> > > > > description
> > > > 
> > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > 
> > > It's good but it is not must, you are inventing your own rules.
> > 
> > As long as the Maintainer is the gatekeeper, you're not going to get
> > very far with this argument.  The fact is that a lot of subsystems have
> > varying rules; often undocumented, some of which are even in conflict,
> > like alphabetic vs reverse christmas tree format for includes.
> > 
> > A cover letter is actually one of the more uniform rules.  It's
> > referred to in submitting patches, but not actually documented there.
> 
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.  I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

Andrew has a workflow unlike any other I've seen..

In my view the cover letter should explain why the maintainer should
apply the series, and give any guidance to the review process.

Not duplicate information that belongs in the patch comments. It
shouldn't explain how/why the patch(es) works, etc.

Jason

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jason Gunthorpe]

On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > 
> > > > 
> > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > 
> > > > > Why you need cover letter???What are u missing in the patch
> > > > > description
> > > > 
> > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > 
> > > It's good but it is not must, you are inventing your own rules.
> > 
> > As long as the Maintainer is the gatekeeper, you're not going to get
> > very far with this argument. ?The fact is that a lot of subsystems have
> > varying rules; often undocumented, some of which are even in conflict,
> > like alphabetic vs reverse christmas tree format for includes.
> > 
> > A cover letter is actually one of the more uniform rules. ?It's
> > referred to in submitting patches, but not actually documented there.
> 
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost. ?I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

Andrew has a workflow unlike any other I've seen..

In my view the cover letter should explain why the maintainer should
apply the series, and give any guidance to the review process.

Not duplicate information that belongs in the patch comments. It
shouldn't explain how/why the patch(es) works, etc.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote:
> On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > > 
> > > > > 
> > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > > 
> > > > > > Why you need cover letter?  What are u missing in the patch
> > > > > > description
> > > > > 
> > > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > > 
> > > > It's good but it is not must, you are inventing your own rules.
> > > 
> > > As long as the Maintainer is the gatekeeper, you're not going to get
> > > very far with this argument.  The fact is that a lot of subsystems have
> > > varying rules; often undocumented, some of which are even in conflict,
> > > like alphabetic vs reverse christmas tree format for includes.
> > > 
> > > A cover letter is actually one of the more uniform rules.  It's
> > > referred to in submitting patches, but not actually documented there.
> > 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.  I've seen
> > Andrew Morton cut and paste the cover letter in the first patch
> > description of the patch set.
> 
> Andrew has a workflow unlike any other I've seen..

Andrew is the only one that actually cut and pasted the cover letter
in the first patch description, but I've heard this from others.

> In my view the cover letter should explain why the maintainer should
> apply the series, and give any guidance to the review process.

> Not duplicate information that belongs in the patch comments. It
> shouldn't explain how/why the patch(es) works, etc.

Patch descriptions should never explain how/why a particular patch
works.  If it is that difficult to understand, then something is wrong
with the patch.  The individual patch descriptions should provide the
current status, the motivation for the change, and short summary of
the change (eg. new features, configs, etc).

The cover letter is needed for (larger) patch sets in order to explain
the overall motivation, instead of having to guess where the patch set
is going.  I wouldn't expect to see a cover letter for a single bug
fix or two.

Mimi

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote:
> On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > > 
> > > > > 
> > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > > 
> > > > > > Why you need cover letter?  What are u missing in the patch
> > > > > > description
> > > > > 
> > > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > > 
> > > > It's good but it is not must, you are inventing your own rules.
> > > 
> > > As long as the Maintainer is the gatekeeper, you're not going to get
> > > very far with this argument.  The fact is that a lot of subsystems have
> > > varying rules; often undocumented, some of which are even in conflict,
> > > like alphabetic vs reverse christmas tree format for includes.
> > > 
> > > A cover letter is actually one of the more uniform rules.  It's
> > > referred to in submitting patches, but not actually documented there.
> > 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.  I've seen
> > Andrew Morton cut and paste the cover letter in the first patch
> > description of the patch set.
> 
> Andrew has a workflow unlike any other I've seen..

Andrew is the only one that actually cut and pasted the cover letter
in the first patch description, but I've heard this from others.

> In my view the cover letter should explain why the maintainer should
> apply the series, and give any guidance to the review process.

> Not duplicate information that belongs in the patch comments. It
> shouldn't explain how/why the patch(es) works, etc.

Patch descriptions should never explain how/why a particular patch
works.  If it is that difficult to understand, then something is wrong
with the patch.  The individual patch descriptions should provide the
current status, the motivation for the change, and short summary of
the change (eg. new features, configs, etc).

The cover letter is needed for (larger) patch sets in order to explain
the overall motivation, instead of having to guess where the patch set
is going.  I wouldn't expect to see a cover letter for a single bug
fix or two.

Mimi

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote:
> On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > > 
> > > > > 
> > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > > 
> > > > > > Why you need cover letter???What are u missing in the patch
> > > > > > description
> > > > > 
> > > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > > 
> > > > It's good but it is not must, you are inventing your own rules.
> > > 
> > > As long as the Maintainer is the gatekeeper, you're not going to get
> > > very far with this argument. ?The fact is that a lot of subsystems have
> > > varying rules; often undocumented, some of which are even in conflict,
> > > like alphabetic vs reverse christmas tree format for includes.
> > > 
> > > A cover letter is actually one of the more uniform rules. ?It's
> > > referred to in submitting patches, but not actually documented there.
> > 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost. ?I've seen
> > Andrew Morton cut and paste the cover letter in the first patch
> > description of the patch set.
> 
> Andrew has a workflow unlike any other I've seen..

Andrew is the only one that actually cut and pasted the cover letter
in the first patch description, but I've heard this from others.

> In my view the cover letter should explain why the maintainer should
> apply the series, and give any guidance to the review process.

> Not duplicate information that belongs in the patch comments. It
> shouldn't explain how/why the patch(es) works, etc.

Patch descriptions should never explain how/why a particular patch
works. ?If it is that difficult to understand, then something is wrong
with the patch. ?The individual patch descriptions should provide the
current status, the motivation for the change, and short summary of
the change (eg. new features, configs, etc).

The cover letter is needed for (larger) patch sets in order to explain
the overall motivation, instead of having to guess where the patch set
is going. ?I wouldn't expect to see a cover letter for a single bug
fix or two.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote:
> > On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> > > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > >
> > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > Why you need cover letter?  What are u missing in the patch
> > > > > > > description
> > > > > >
> > > > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > > >
> > > > > It's good but it is not must, you are inventing your own rules.
> > > >
> > > > As long as the Maintainer is the gatekeeper, you're not going to
> > > > get very far with this argument.  The fact is that a lot of
> > > > subsystems have varying rules; often undocumented, some of which
> > > > are even in conflict, like alphabetic vs reverse christmas tree format for
> includes.
> > > >
> > > > A cover letter is actually one of the more uniform rules.  It's
> > > > referred to in submitting patches, but not actually documented there.
> > >
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost.  I've seen
> > > Andrew Morton cut and paste the cover letter in the first patch
> > > description of the patch set.
> >
> > Andrew has a workflow unlike any other I've seen..
> 
> Andrew is the only one that actually cut and pasted the cover letter in the
> first patch description, but I've heard this from others.
> 
> > In my view the cover letter should explain why the maintainer should
> > apply the series, and give any guidance to the review process.
> 
> > Not duplicate information that belongs in the patch comments. It
> > shouldn't explain how/why the patch(es) works, etc.
> 
> Patch descriptions should never explain how/why a particular patch
> works.  If it is that difficult to understand, then something is wrong with the
> patch.  The individual patch descriptions should provide the current status,
> the motivation for the change, and short summary of the change (eg. new
> features, configs, etc).
> 
> The cover letter is needed for (larger) patch sets in order to explain the
> overall motivation, instead of having to guess where the patch set is going.  I
> wouldn't expect to see a cover letter for a single bug fix or two.
> 
> Mimi

I second that.

Tomas.

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote:
> > On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> > > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > >
> > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > Why you need cover letter?  What are u missing in the patch
> > > > > > > description
> > > > > >
> > > > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > > >
> > > > > It's good but it is not must, you are inventing your own rules.
> > > >
> > > > As long as the Maintainer is the gatekeeper, you're not going to
> > > > get very far with this argument.  The fact is that a lot of
> > > > subsystems have varying rules; often undocumented, some of which
> > > > are even in conflict, like alphabetic vs reverse christmas tree format for
> includes.
> > > >
> > > > A cover letter is actually one of the more uniform rules.  It's
> > > > referred to in submitting patches, but not actually documented there.
> > >
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost.  I've seen
> > > Andrew Morton cut and paste the cover letter in the first patch
> > > description of the patch set.
> >
> > Andrew has a workflow unlike any other I've seen..
> 
> Andrew is the only one that actually cut and pasted the cover letter in the
> first patch description, but I've heard this from others.
> 
> > In my view the cover letter should explain why the maintainer should
> > apply the series, and give any guidance to the review process.
> 
> > Not duplicate information that belongs in the patch comments. It
> > shouldn't explain how/why the patch(es) works, etc.
> 
> Patch descriptions should never explain how/why a particular patch
> works.  If it is that difficult to understand, then something is wrong with the
> patch.  The individual patch descriptions should provide the current status,
> the motivation for the change, and short summary of the change (eg. new
> features, configs, etc).
> 
> The cover letter is needed for (larger) patch sets in order to explain the
> overall motivation, instead of having to guess where the patch set is going.  I
> wouldn't expect to see a cover letter for a single bug fix or two.
> 
> Mimi

I second that.

Tomas.

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote:
> > On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote:
> > > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote:
> > > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote:
> > > > > >
> > > > > >
> > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> > > > > > >
> > > > > > > Why you need cover letter?????What are u missing in the patch
> > > > > > > description
> > > > > >
> > > > > > If you submit a *patch set* I *require* a cover letter, yes.
> > > > >
> > > > > It's good but it is not must, you are inventing your own rules.
> > > >
> > > > As long as the Maintainer is the gatekeeper, you're not going to
> > > > get very far with this argument. ??The fact is that a lot of
> > > > subsystems have varying rules; often undocumented, some of which
> > > > are even in conflict, like alphabetic vs reverse christmas tree format for
> includes.
> > > >
> > > > A cover letter is actually one of the more uniform rules. ??It's
> > > > referred to in submitting patches, but not actually documented there.
> > >
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost. ??I've seen
> > > Andrew Morton cut and paste the cover letter in the first patch
> > > description of the patch set.
> >
> > Andrew has a workflow unlike any other I've seen..
> 
> Andrew is the only one that actually cut and pasted the cover letter in the
> first patch description, but I've heard this from others.
> 
> > In my view the cover letter should explain why the maintainer should
> > apply the series, and give any guidance to the review process.
> 
> > Not duplicate information that belongs in the patch comments. It
> > shouldn't explain how/why the patch(es) works, etc.
> 
> Patch descriptions should never explain how/why a particular patch
> works. ??If it is that difficult to understand, then something is wrong with the
> patch. ??The individual patch descriptions should provide the current status,
> the motivation for the change, and short summary of the change (eg. new
> features, configs, etc).
> 
> The cover letter is needed for (larger) patch sets in order to explain the
> overall motivation, instead of having to guess where the patch set is going. ??I
> wouldn't expect to see a cover letter for a single bug fix or two.
> 
> Mimi

I second that.

Tomas.

????{.n?+???????+%???????\x17??w??{.n?+????{??????????v?^?)????w*\x1fjg???\x1e???????j??\a??G??????\f???j:+v???w?j?m?????\x1e??\x1e?w?????f???h?????????

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jonathan Corbet]

On Tue, 06 Mar 2018 13:36:36 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.

If I get a patch series with a cover letter that should be preserved, I
apply the series in a branch then do a no-ff merge; the cover letter can
then go into the merge commit.  There's no reason why cover letters need to
be lost.

jon

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jonathan Corbet]

On Tue, 06 Mar 2018 13:36:36 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.

If I get a patch series with a cover letter that should be preserved, I
apply the series in a branch then do a no-ff merge; the cover letter can
then go into the merge commit.  There's no reason why cover letters need to
be lost.

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Winkler, Tomas]

> 
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I apply the
> series in a branch then do a no-ff merge; the cover letter can then go into
> the merge commit.  There's no reason why cover letters need to be lost.

That's clever, but still it feels like a hack. 
I've try to keep cover letters as an empty commits for that but it has to be always forced when rebasing.
Not sure git has a real user friendly support for that.

Tomas

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I
> apply the series in a branch then do a no-ff merge; the cover letter can
> then go into the merge commit.  There's no reason why cover letters need to
> be lost.

Thanks, Jon.  That sounds like a really, good idea.

Some maintainers are saying to put the Changelog after the "---" so
that it isn't included in the patch description.

One of the reasons for including the Changelog in the patch
description, is to credit people with bug fixes, important rework of
the patch, etc.

Do you have any thoughts on this?

Mimi

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Mimi Zohar]

On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I
> apply the series in a branch then do a no-ff merge; the cover letter can
> then go into the merge commit.  There's no reason why cover letters need to
> be lost.

Thanks, Jon.  That sounds like a really, good idea.

Some maintainers are saying to put the Changelog after the "---" so
that it isn't included in the patch description.

One of the reasons for including the Changelog in the patch
description, is to credit people with bug fixes, important rework of
the patch, etc.

Do you have any thoughts on this?

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jonathan Corbet]

On Wed, 07 Mar 2018 11:35:03 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> Some maintainers are saying to put the Changelog after the "---" so
> that it isn't included in the patch description.
> 
> One of the reasons for including the Changelog in the patch
> description, is to credit people with bug fixes, important rework of
> the patch, etc.
> 
> Do you have any thoughts on this?

Not sure I have a strong opinion there.  We do have various tags meant to
ensure proper credit; I would lean toward using those when possible rather
than including long change histories that, in the long term, don't help
readers to understand why the patch was applied.

Thanks,

jon

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jonathan Corbet]

On Wed, 07 Mar 2018 11:35:03 -0500
Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> Some maintainers are saying to put the Changelog after the "---" so
> that it isn't included in the patch description.
> 
> One of the reasons for including the Changelog in the patch
> description, is to credit people with bug fixes, important rework of
> the patch, etc.
> 
> Do you have any thoughts on this?

Not sure I have a strong opinion there.  We do have various tags meant to
ensure proper credit; I would lean toward using those when possible rather
than including long change histories that, in the long term, don't help
readers to understand why the patch was applied.

Thanks,

jon
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 11:35 -0500, Mimi Zohar wrote:
> On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> > On Tue, 06 Mar 2018 13:36:36 -0500
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost.
> > 
> > If I get a patch series with a cover letter that should be preserved, I
> > apply the series in a branch then do a no-ff merge; the cover letter can
> > then go into the merge commit.  There's no reason why cover letters need to
> > be lost.
> 
> Thanks, Jon.  That sounds like a really, good idea.
> 
> Some maintainers are saying to put the Changelog after the "---" so
> that it isn't included in the patch description.
> 
> One of the reasons for including the Changelog in the patch
> description, is to credit people with bug fixes, important rework of
> the patch, etc.

This is a really good point. By adding the cover letter to the
GIT history helps to better track people who to thank or blame :-)

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 11:35 -0500, Mimi Zohar wrote:
> On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> > On Tue, 06 Mar 2018 13:36:36 -0500
> > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > 
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost.
> > 
> > If I get a patch series with a cover letter that should be preserved, I
> > apply the series in a branch then do a no-ff merge; the cover letter can
> > then go into the merge commit.  There's no reason why cover letters need to
> > be lost.
> 
> Thanks, Jon.  That sounds like a really, good idea.
> 
> Some maintainers are saying to put the Changelog after the "---" so
> that it isn't included in the patch description.
> 
> One of the reasons for including the Changelog in the patch
> description, is to credit people with bug fixes, important rework of
> the patch, etc.

This is a really good point. By adding the cover letter to the
GIT history helps to better track people who to thank or blame :-)

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I
> apply the series in a branch then do a no-ff merge; the cover letter can
> then go into the merge commit.  There's no reason why cover letters need to
> be lost.

That is an awesome idea. I'll start using this approach. Thank you.

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I
> apply the series in a branch then do a no-ff merge; the cover letter can
> then go into the merge commit.  There's no reason why cover letters need to
> be lost.

That is an awesome idea. I'll start using this approach. Thank you.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Tue, 2018-03-06 at 13:36 -0500, Mimi Zohar wrote:
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.  I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

When I contribute code, the cover letter helps me to do the Right
Thing.. Taking the time to write a proper cover letter helps to do a
"mental exercise" that

1. The changes make sense in the first place.
2. Only the necessary is done, not more or less.

Even for a small patch set the time used to write the cover letter
will pay off because it helps the maitainer to make a fair and
educated decision.

/Jarkko

[PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Tue, 2018-03-06 at 13:36 -0500, Mimi Zohar wrote:
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.  I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

When I contribute code, the cover letter helps me to do the Right
Thing.. Taking the time to write a proper cover letter helps to do a
"mental exercise" that

1. The changes make sense in the first place.
2. Only the necessary is done, not more or less.

Even for a small patch set the time used to write the cover letter
will pay off because it helps the maitainer to make a fair and
educated decision.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html