diff mbox

[OPW,kernel,v2,2/4] firmware: efi: Add timespec to timespec64 conversion

Message ID 1414758146-11906-3-git-send-email-somyaanand214@gmail.com
State New, archived
Headers show

Commit Message

Somya Anand Oct. 31, 2014, 12:22 p.m. UTC
32-bit systems using 'struct timespec' will break in the year 2038,
so we have to replace that code with more appropriate types.

Introducing a local variable of type 'struct timespec64' to update
interface of pstore_info.read and pstore_info.erase in order to
support new pstore_info API.

Signed-off-by: Somya Anand <somyaanand214@gmail.com>
---
Changes since version 1:
  * Use timespec_to_timespec64 conversion.
  * Reword commit message.
  
 drivers/firmware/efi/efi-pstore.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

Comments

Arnd Bergmann Oct. 31, 2014, 4:06 p.m. UTC | #1
On Friday 31 October 2014 17:52:24 Somya Anand wrote:
> 32-bit systems using 'struct timespec' will break in the year 2038,
> so we have to replace that code with more appropriate types.
> 
> Introducing a local variable of type 'struct timespec64' to update
> interface of pstore_info.read and pstore_info.erase in order to
> support new pstore_info API.
> 
> Signed-off-by: Somya Anand <somyaanand214@gmail.com>

Hi Somya,

Looks better than the first version, but you still have a couple of
bugs that I found by taking a closer look.

> diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
> index e992abc..537c4b0 100644
> --- a/drivers/firmware/efi/efi-pstore.c
> +++ b/drivers/firmware/efi/efi-pstore.c
> @@ -32,7 +32,7 @@ struct pstore_read_data {
>  	u64 *id;
>  	enum pstore_type_id *type;
>  	int *count;
> -	struct timespec *timespec;
> +	struct timespec64 *ts64;
>  	bool *compressed;
>  	char **buf;
>  };

While renaming the struct member helps find all the users, I think it
would be better to keep the patch simple by using the existing name.

> @@ -63,8 +63,8 @@ static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
>  		   cb_data->type, &part, &cnt, &time, &data_type) == 5) {
>  		*cb_data->id = generic_id(time, part, cnt);
>  		*cb_data->count = cnt;
> -		cb_data->timespec->tv_sec = time;
> -		cb_data->timespec->tv_nsec = 0;
> +		cb_data->ts64->tv_sec = time;
> +		cb_data->ts64->tv_nsec = 0;
>  		if (data_type == 'C')
>  			*cb_data->compressed = true;
>  		else

'time' is still an unsigned long, so you haven't actually solved the problem
for 32-bit systems here.

> @@ -208,17 +208,17 @@ static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
>   *           and pstore will stop reading entry.
>   */
>  static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
> -			       int *count, struct timespec *timespec,
> +			       int *count, struct timespec *ts,
>  			       char **buf, bool *compressed,
>  			       struct pstore_info *psi)
>  {
> +	struct timespec64 time = timespec_to_timespec64(*ts);
>  	struct pstore_read_data data;
>  	ssize_t size;
> -
>  	data.id = id;
>  	data.type = type;
>  	data.count = count;
> -	data.timespec = timespec;
> +	data.ts64 = &time;
>  	data.compressed = compressed;
>  	data.buf = buf;
>  

This part is wrong because 'timespec' is an output argument, not
an input. See how data.timespec gets used in the functions called
from here.

> @@ -293,7 +293,7 @@ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
>  		 * holding multiple logs, remains.
>  		 */
>  		sprintf(name_old, "dump-type%u-%u-%lu", ed->type,
> -			(unsigned int)ed->id, ed->time.tv_sec);
> +			(unsigned int)ed->id, ed->ts64.tv_sec);
>  
>  		for (i = 0; i < DUMP_NAME_LEN; i++)
>  			efi_name_old[i] = name_old[i];

The compiler will warn about this when building for 32-bit systems, and
the warning is about an actual bug here that causes you to lose
data.

> @@ -319,8 +319,9 @@ static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
>  }
>  
>  static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
> -			    struct timespec time, struct pstore_info *psi)
> +			    struct timespec ts, struct pstore_info *psi)
>  {
> +	struct timespec64 time = timespec_to_timespec64(ts);
>  	struct pstore_erase_data edata;
>  	struct efivar_entry *entry = NULL;
>  	char name[DUMP_NAME_LEN];
> @@ -338,7 +339,7 @@ static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
>  	edata.id = part;
>  	edata.type = type;
>  	edata.count = count;
> -	edata.time = time;
> +	edata.ts64 = time;
>  	edata.name = efi_name;
>  
>  	efivar_entry_iter_begin();

This part looks good.

	Arnd
diff mbox

Patch

diff --git a/drivers/firmware/efi/efi-pstore.c b/drivers/firmware/efi/efi-pstore.c
index e992abc..537c4b0 100644
--- a/drivers/firmware/efi/efi-pstore.c
+++ b/drivers/firmware/efi/efi-pstore.c
@@ -32,7 +32,7 @@  struct pstore_read_data {
 	u64 *id;
 	enum pstore_type_id *type;
 	int *count;
-	struct timespec *timespec;
+	struct timespec64 *ts64;
 	bool *compressed;
 	char **buf;
 };
@@ -63,8 +63,8 @@  static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 		   cb_data->type, &part, &cnt, &time, &data_type) == 5) {
 		*cb_data->id = generic_id(time, part, cnt);
 		*cb_data->count = cnt;
-		cb_data->timespec->tv_sec = time;
-		cb_data->timespec->tv_nsec = 0;
+		cb_data->ts64->tv_sec = time;
+		cb_data->ts64->tv_nsec = 0;
 		if (data_type == 'C')
 			*cb_data->compressed = true;
 		else
@@ -73,8 +73,8 @@  static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 		   cb_data->type, &part, &cnt, &time) == 4) {
 		*cb_data->id = generic_id(time, part, cnt);
 		*cb_data->count = cnt;
-		cb_data->timespec->tv_sec = time;
-		cb_data->timespec->tv_nsec = 0;
+		cb_data->ts64->tv_sec = time;
+		cb_data->ts64->tv_nsec = 0;
 		*cb_data->compressed = false;
 	} else if (sscanf(name, "dump-type%u-%u-%lu",
 			  cb_data->type, &part, &time) == 3) {
@@ -85,8 +85,8 @@  static int efi_pstore_read_func(struct efivar_entry *entry, void *data)
 		 */
 		*cb_data->id = generic_id(time, part, 0);
 		*cb_data->count = 0;
-		cb_data->timespec->tv_sec = time;
-		cb_data->timespec->tv_nsec = 0;
+		cb_data->ts64->tv_sec = time;
+		cb_data->ts64->tv_nsec = 0;
 		*cb_data->compressed = false;
 	} else
 		return 0;
@@ -208,17 +208,17 @@  static int efi_pstore_sysfs_entry_iter(void *data, struct efivar_entry **pos)
  *           and pstore will stop reading entry.
  */
 static ssize_t efi_pstore_read(u64 *id, enum pstore_type_id *type,
-			       int *count, struct timespec *timespec,
+			       int *count, struct timespec *ts,
 			       char **buf, bool *compressed,
 			       struct pstore_info *psi)
 {
+	struct timespec64 time = timespec_to_timespec64(*ts);
 	struct pstore_read_data data;
 	ssize_t size;
-
 	data.id = id;
 	data.type = type;
 	data.count = count;
-	data.timespec = timespec;
+	data.ts64 = &time;
 	data.compressed = compressed;
 	data.buf = buf;
 
@@ -266,7 +266,7 @@  struct pstore_erase_data {
 	u64 id;
 	enum pstore_type_id type;
 	int count;
-	struct timespec time;
+	struct timespec64 ts64;
 	efi_char16_t *name;
 };
 
@@ -293,7 +293,7 @@  static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
 		 * holding multiple logs, remains.
 		 */
 		sprintf(name_old, "dump-type%u-%u-%lu", ed->type,
-			(unsigned int)ed->id, ed->time.tv_sec);
+			(unsigned int)ed->id, ed->ts64.tv_sec);
 
 		for (i = 0; i < DUMP_NAME_LEN; i++)
 			efi_name_old[i] = name_old[i];
@@ -319,8 +319,9 @@  static int efi_pstore_erase_func(struct efivar_entry *entry, void *data)
 }
 
 static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
-			    struct timespec time, struct pstore_info *psi)
+			    struct timespec ts, struct pstore_info *psi)
 {
+	struct timespec64 time = timespec_to_timespec64(ts);
 	struct pstore_erase_data edata;
 	struct efivar_entry *entry = NULL;
 	char name[DUMP_NAME_LEN];
@@ -338,7 +339,7 @@  static int efi_pstore_erase(enum pstore_type_id type, u64 id, int count,
 	edata.id = part;
 	edata.type = type;
 	edata.count = count;
-	edata.time = time;
+	edata.ts64 = time;
 	edata.name = efi_name;
 
 	efivar_entry_iter_begin();