DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] staging: exfat: add millisecond support
@ 2019-09-08 12:48 Valentin Vidic
  2019-09-08 13:03 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 4+ messages in thread
From: Valentin Vidic @ 2019-09-08 12:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: devel, Valentin Vidic, Valdis Kletnieks, linux-kernel

Drop duplicated date_time_t struct and add millisecond handling for
create and modify time. Also drop millisecond field for access time
since it is not defined in the the spec.

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
 drivers/staging/exfat/exfat.h       |  38 +++---
 drivers/staging/exfat/exfat_core.c  |  31 ++++-
 drivers/staging/exfat/exfat_super.c | 174 ++++++++--------------------
 3 files changed, 92 insertions(+), 151 deletions(-)

diff --git a/drivers/staging/exfat/exfat.h b/drivers/staging/exfat/exfat.h
index 0aa14dea4e09..6491ea034928 100644
--- a/drivers/staging/exfat/exfat.h
+++ b/drivers/staging/exfat/exfat.h
@@ -241,16 +241,6 @@ static inline u16 get_row_index(u16 i)
 #define UNI_PAR_DIR_NAME        "\0.\0."
 #endif
 
-struct date_time_t {
-	u16      Year;
-	u16      Month;
-	u16      Day;
-	u16      Hour;
-	u16      Minute;
-	u16      Second;
-	u16      MilliSecond;
-};
-
 struct part_info_t {
 	u32      Offset;    /* start sector number of the partition */
 	u32      Size;      /* in sectors */
@@ -289,6 +279,16 @@ struct file_id_t {
 	u32      hint_last_clu;
 };
 
+struct timestamp_t {
+	u16      millisec;   /* 0 ~ 999              */
+	u16      sec;        /* 0 ~ 59               */
+	u16      min;        /* 0 ~ 59               */
+	u16      hour;       /* 0 ~ 23               */
+	u16      day;        /* 1 ~ 31               */
+	u16      mon;        /* 1 ~ 12               */
+	u16      year;       /* 0 ~ 127 (since 1980) */
+};
+
 struct dir_entry_t {
 	char Name[MAX_NAME_LENGTH * MAX_CHARSET_SIZE];
 
@@ -298,18 +298,9 @@ struct dir_entry_t {
 	u32 Attr;
 	u64 Size;
 	u32 NumSubdirs;
-	struct date_time_t CreateTimestamp;
-	struct date_time_t ModifyTimestamp;
-	struct date_time_t AccessTimestamp;
-};
-
-struct timestamp_t {
-	u16      sec;        /* 0 ~ 59               */
-	u16      min;        /* 0 ~ 59               */
-	u16      hour;       /* 0 ~ 23               */
-	u16      day;        /* 1 ~ 31               */
-	u16      mon;        /* 1 ~ 12               */
-	u16      year;       /* 0 ~ 127 (since 1980) */
+	struct timestamp_t CreateTimestamp;
+	struct timestamp_t ModifyTimestamp;
+	struct timestamp_t AccessTimestamp;
 };
 
 /* MS_DOS FAT partition boot record (512 bytes) */
@@ -453,8 +444,7 @@ struct file_dentry_t {
 	u8       access_date[2];
 	u8       create_time_ms;
 	u8       modify_time_ms;
-	u8       access_time_ms;
-	u8       reserved2[9];
+	u8       reserved2[10];
 };
 
 /* MS-DOS EXFAT stream extension directory entry (32 bytes) */
diff --git a/drivers/staging/exfat/exfat_core.c b/drivers/staging/exfat/exfat_core.c
index 995358cc7c79..e87119fa8c0a 100644
--- a/drivers/staging/exfat/exfat_core.c
+++ b/drivers/staging/exfat/exfat_core.c
@@ -1139,6 +1139,7 @@ void exfat_set_entry_size(struct dentry_t *p_entry, u64 size)
 void fat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 			u8 mode)
 {
+	u8 ms = 0;
 	u16 t = 0x00, d = 0x21;
 	struct dos_dentry_t *ep = (struct dos_dentry_t *)p_entry;
 
@@ -1146,6 +1147,7 @@ void fat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 	case TM_CREATE:
 		t = GET16_A(ep->create_time);
 		d = GET16_A(ep->create_date);
+		ms = ep->create_time_ms * 10;
 		break;
 	case TM_MODIFY:
 		t = GET16_A(ep->modify_time);
@@ -1159,11 +1161,17 @@ void fat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 	tp->day  = (d & 0x001F);
 	tp->mon  = (d >> 5) & 0x000F;
 	tp->year = (d >> 9);
+
+	if (ms >= 1000) {
+		ms -= 1000;
+		tp->sec++;
+	}
 }
 
 void exfat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 			  u8 mode)
 {
+	u8 ms = 0;
 	u16 t = 0x00, d = 0x21;
 	struct file_dentry_t *ep = (struct file_dentry_t *)p_entry;
 
@@ -1171,10 +1179,12 @@ void exfat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 	case TM_CREATE:
 		t = GET16_A(ep->create_time);
 		d = GET16_A(ep->create_date);
+		ms = ep->create_time_ms * 10;
 		break;
 	case TM_MODIFY:
 		t = GET16_A(ep->modify_time);
 		d = GET16_A(ep->modify_date);
+		ms = ep->modify_time_ms * 10;
 		break;
 	case TM_ACCESS:
 		t = GET16_A(ep->access_time);
@@ -1188,21 +1198,33 @@ void exfat_get_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 	tp->day  = (d & 0x001F);
 	tp->mon  = (d >> 5) & 0x000F;
 	tp->year = (d >> 9);
+
+	if (ms >= 1000) {
+		ms -= 1000;
+		tp->sec++;
+	}
 }
 
 void fat_set_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 			u8 mode)
 {
+	u8 ms;
 	u16 t, d;
 	struct dos_dentry_t *ep = (struct dos_dentry_t *)p_entry;
 
 	t = (tp->hour << 11) | (tp->min << 5) | (tp->sec >> 1);
 	d = (tp->year <<  9) | (tp->mon << 5) |  tp->day;
 
+	ms = tp->millisec;
+	if (tp->sec & 1) {
+		ms += 1000;
+	}
+
 	switch (mode) {
 	case TM_CREATE:
 		SET16_A(ep->create_time, t);
 		SET16_A(ep->create_date, d);
+		ep->create_time_ms = ms / 10;
 		break;
 	case TM_MODIFY:
 		SET16_A(ep->modify_time, t);
@@ -1214,20 +1236,28 @@ void fat_set_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 void exfat_set_entry_time(struct dentry_t *p_entry, struct timestamp_t *tp,
 			  u8 mode)
 {
+	u8 ms;
 	u16 t, d;
 	struct file_dentry_t *ep = (struct file_dentry_t *)p_entry;
 
 	t = (tp->hour << 11) | (tp->min << 5) | (tp->sec >> 1);
 	d = (tp->year <<  9) | (tp->mon << 5) |  tp->day;
 
+	ms = tp->millisec;
+	if (tp->sec & 1) {
+		ms += 1000;
+	}
+
 	switch (mode) {
 	case TM_CREATE:
 		SET16_A(ep->create_time, t);
 		SET16_A(ep->create_date, d);
+		ep->create_time_ms = ms / 10;
 		break;
 	case TM_MODIFY:
 		SET16_A(ep->modify_time, t);
 		SET16_A(ep->modify_date, d);
+		ep->modify_time_ms = ms / 10;
 		break;
 	case TM_ACCESS:
 		SET16_A(ep->access_time, t);
@@ -1456,7 +1486,6 @@ void init_file_entry(struct file_dentry_t *ep, u32 type)
 	exfat_set_entry_time((struct dentry_t *)ep, tp, TM_ACCESS);
 	ep->create_time_ms = 0;
 	ep->modify_time_ms = 0;
-	ep->access_time_ms = 0;
 }
 
 void init_strm_entry(struct strm_dentry_t *ep, u8 flags, u32 start_clu, u64 size)
diff --git a/drivers/staging/exfat/exfat_super.c b/drivers/staging/exfat/exfat_super.c
index 60dfea73a7a4..54b6c2ff3c96 100644
--- a/drivers/staging/exfat/exfat_super.c
+++ b/drivers/staging/exfat/exfat_super.c
@@ -56,18 +56,16 @@ static void exfat_write_super(struct super_block *sb);
 #define UNIX_SECS_2108    4354819200L
 
 /* Convert a FAT time/date pair to a UNIX date (seconds since 1 1 70). */
-static void exfat_time_fat2unix(struct exfat_sb_info *sbi,
-				struct timespec64 *ts, struct date_time_t *tp)
+static void exfat_time_fat2unix(struct timespec64 *ts, struct timestamp_t *tp)
 {
-	ts->tv_sec = mktime64(tp->Year + 1980, tp->Month + 1, tp->Day,
-			      tp->Hour, tp->Minute, tp->Second);
+	ts->tv_sec = mktime64(tp->year + 1980, tp->mon + 1, tp->day,
+			      tp->hour, tp->min, tp->sec);
 
-	ts->tv_nsec = tp->MilliSecond * NSEC_PER_MSEC;
+	ts->tv_nsec = tp->millisec * NSEC_PER_MSEC;
 }
 
 /* Convert linear UNIX date to a FAT time/date pair. */
-static void exfat_time_unix2fat(struct exfat_sb_info *sbi,
-				struct timespec64 *ts, struct date_time_t *tp)
+static void exfat_time_unix2fat(struct timespec64 *ts, struct timestamp_t *tp)
 {
 	time64_t second = ts->tv_sec;
 	struct tm tm;
@@ -75,69 +73,42 @@ static void exfat_time_unix2fat(struct exfat_sb_info *sbi,
 	time64_to_tm(second, 0, &tm);
 
 	if (second < UNIX_SECS_1980) {
-		tp->MilliSecond = 0;
-		tp->Second	= 0;
-		tp->Minute	= 0;
-		tp->Hour	= 0;
-		tp->Day		= 1;
-		tp->Month	= 1;
-		tp->Year	= 0;
+		tp->millisec	= 0;
+		tp->sec		= 0;
+		tp->min		= 0;
+		tp->hour	= 0;
+		tp->day		= 1;
+		tp->mon		= 1;
+		tp->year	= 0;
 		return;
 	}
 
 	if (second >= UNIX_SECS_2108) {
-		tp->MilliSecond = 999;
-		tp->Second	= 59;
-		tp->Minute	= 59;
-		tp->Hour	= 23;
-		tp->Day		= 31;
-		tp->Month	= 12;
-		tp->Year	= 127;
+		tp->millisec	= 999;
+		tp->sec		= 59;
+		tp->min		= 59;
+		tp->hour	= 23;
+		tp->day		= 31;
+		tp->mon		= 12;
+		tp->year	= 127;
 		return;
 	}
 
-	tp->MilliSecond = ts->tv_nsec / NSEC_PER_MSEC;
-	tp->Second	= tm.tm_sec;
-	tp->Minute	= tm.tm_min;
-	tp->Hour	= tm.tm_hour;
-	tp->Day		= tm.tm_mday;
-	tp->Month	= tm.tm_mon + 1;
-	tp->Year	= tm.tm_year + 1900 - 1980;
+	tp->millisec	= ts->tv_nsec / NSEC_PER_MSEC;
+	tp->sec		= tm.tm_sec;
+	tp->min		= tm.tm_min;
+	tp->hour	= tm.tm_hour;
+	tp->day		= tm.tm_mday;
+	tp->mon		= tm.tm_mon + 1;
+	tp->year	= tm.tm_year + 1900 - 1980;
 }
 
 struct timestamp_t *tm_current(struct timestamp_t *tp)
 {
-	time64_t second = ktime_get_real_seconds();
-	struct tm tm;
-
-	time64_to_tm(second, 0, &tm);
+	struct timespec64 ts;
 
-	if (second < UNIX_SECS_1980) {
-		tp->sec  = 0;
-		tp->min  = 0;
-		tp->hour = 0;
-		tp->day  = 1;
-		tp->mon  = 1;
-		tp->year = 0;
-		return tp;
-	}
-
-	if (second >= UNIX_SECS_2108) {
-		tp->sec  = 59;
-		tp->min  = 59;
-		tp->hour = 23;
-		tp->day  = 31;
-		tp->mon  = 12;
-		tp->year = 127;
-		return tp;
-	}
-
-	tp->sec  = tm.tm_sec;
-	tp->min  = tm.tm_min;
-	tp->hour = tm.tm_hour;
-	tp->day  = tm.tm_mday;
-	tp->mon  = tm.tm_mon + 1;
-	tp->year = tm.tm_year + 1900 - 1980;
+	ktime_get_real_ts64(&ts);
+	exfat_time_unix2fat(&ts, tp);
 
 	return tp;
 }
@@ -1502,7 +1473,6 @@ static int ffsReadStat(struct inode *inode, struct dir_entry_t *info)
 	int ret = FFS_SUCCESS;
 	struct chain_t dir;
 	struct uni_name_t uni_name;
-	struct timestamp_t tm;
 	struct dentry_t *ep, *ep2;
 	struct super_block *sb = inode->i_sb;
 	struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
@@ -1520,11 +1490,11 @@ static int ffsReadStat(struct inode *inode, struct dir_entry_t *info)
 		    (fid->entry == -1)) {
 			info->Attr = ATTR_SUBDIR;
 			memset((char *)&info->CreateTimestamp, 0,
-			       sizeof(struct date_time_t));
+			       sizeof(struct timestamp_t));
 			memset((char *)&info->ModifyTimestamp, 0,
-			       sizeof(struct date_time_t));
+			       sizeof(struct timestamp_t));
 			memset((char *)&info->AccessTimestamp, 0,
-			       sizeof(struct date_time_t));
+			       sizeof(struct timestamp_t));
 			strcpy(info->ShortName, ".");
 			strcpy(info->Name, ".");
 
@@ -1575,25 +1545,9 @@ static int ffsReadStat(struct inode *inode, struct dir_entry_t *info)
 	/* set FILE_INFO structure using the acquired struct dentry_t */
 	info->Attr = p_fs->fs_func->get_entry_attr(ep);
 
-	p_fs->fs_func->get_entry_time(ep, &tm, TM_CREATE);
-	info->CreateTimestamp.Year = tm.year;
-	info->CreateTimestamp.Month = tm.mon;
-	info->CreateTimestamp.Day = tm.day;
-	info->CreateTimestamp.Hour = tm.hour;
-	info->CreateTimestamp.Minute = tm.min;
-	info->CreateTimestamp.Second = tm.sec;
-	info->CreateTimestamp.MilliSecond = 0;
-
-	p_fs->fs_func->get_entry_time(ep, &tm, TM_MODIFY);
-	info->ModifyTimestamp.Year = tm.year;
-	info->ModifyTimestamp.Month = tm.mon;
-	info->ModifyTimestamp.Day = tm.day;
-	info->ModifyTimestamp.Hour = tm.hour;
-	info->ModifyTimestamp.Minute = tm.min;
-	info->ModifyTimestamp.Second = tm.sec;
-	info->ModifyTimestamp.MilliSecond = 0;
-
-	memset((char *) &info->AccessTimestamp, 0, sizeof(struct date_time_t));
+	p_fs->fs_func->get_entry_time(ep, &info->CreateTimestamp, TM_CREATE);
+	p_fs->fs_func->get_entry_time(ep, &info->ModifyTimestamp, TM_MODIFY);
+	memset((char *)&info->AccessTimestamp, 0, sizeof(struct timestamp_t));
 
 	*(uni_name.name) = 0x0;
 	/* XXX this is very bad for exfat cuz name is already included in es.
@@ -1652,7 +1606,6 @@ static int ffsWriteStat(struct inode *inode, struct dir_entry_t *info)
 {
 	sector_t sector = 0;
 	int ret = FFS_SUCCESS;
-	struct timestamp_t tm;
 	struct dentry_t *ep, *ep2;
 	struct entry_set_cache_t *es = NULL;
 	struct super_block *sb = inode->i_sb;
@@ -1699,22 +1652,8 @@ static int ffsWriteStat(struct inode *inode, struct dir_entry_t *info)
 	p_fs->fs_func->set_entry_attr(ep, info->Attr);
 
 	/* set FILE_INFO structure using the acquired struct dentry_t */
-	tm.sec  = info->CreateTimestamp.Second;
-	tm.min  = info->CreateTimestamp.Minute;
-	tm.hour = info->CreateTimestamp.Hour;
-	tm.day  = info->CreateTimestamp.Day;
-	tm.mon  = info->CreateTimestamp.Month;
-	tm.year = info->CreateTimestamp.Year;
-	p_fs->fs_func->set_entry_time(ep, &tm, TM_CREATE);
-
-	tm.sec  = info->ModifyTimestamp.Second;
-	tm.min  = info->ModifyTimestamp.Minute;
-	tm.hour = info->ModifyTimestamp.Hour;
-	tm.day  = info->ModifyTimestamp.Day;
-	tm.mon  = info->ModifyTimestamp.Month;
-	tm.year = info->ModifyTimestamp.Year;
-	p_fs->fs_func->set_entry_time(ep, &tm, TM_MODIFY);
-
+	p_fs->fs_func->set_entry_time(ep, &info->CreateTimestamp, TM_CREATE);
+	p_fs->fs_func->set_entry_time(ep, &info->ModifyTimestamp, TM_MODIFY);
 	p_fs->fs_func->set_entry_size(ep2, info->Size);
 
 	if (p_fs->vol_type != EXFAT) {
@@ -1941,7 +1880,6 @@ static int ffsReadDir(struct inode *inode, struct dir_entry_t *dir_entry)
 	sector_t sector;
 	struct chain_t dir, clu;
 	struct uni_name_t uni_name;
-	struct timestamp_t tm;
 	struct dentry_t *ep;
 	struct super_block *sb = inode->i_sb;
 	struct fs_info_t *p_fs = &(EXFAT_SB(sb)->fs_info);
@@ -2038,26 +1976,12 @@ static int ffsReadDir(struct inode *inode, struct dir_entry_t *dir_entry)
 			buf_lock(sb, sector);
 			dir_entry->Attr = fs_func->get_entry_attr(ep);
 
-			fs_func->get_entry_time(ep, &tm, TM_CREATE);
-			dir_entry->CreateTimestamp.Year = tm.year;
-			dir_entry->CreateTimestamp.Month = tm.mon;
-			dir_entry->CreateTimestamp.Day = tm.day;
-			dir_entry->CreateTimestamp.Hour = tm.hour;
-			dir_entry->CreateTimestamp.Minute = tm.min;
-			dir_entry->CreateTimestamp.Second = tm.sec;
-			dir_entry->CreateTimestamp.MilliSecond = 0;
-
-			fs_func->get_entry_time(ep, &tm, TM_MODIFY);
-			dir_entry->ModifyTimestamp.Year = tm.year;
-			dir_entry->ModifyTimestamp.Month = tm.mon;
-			dir_entry->ModifyTimestamp.Day = tm.day;
-			dir_entry->ModifyTimestamp.Hour = tm.hour;
-			dir_entry->ModifyTimestamp.Minute = tm.min;
-			dir_entry->ModifyTimestamp.Second = tm.sec;
-			dir_entry->ModifyTimestamp.MilliSecond = 0;
-
+			fs_func->get_entry_time(ep, &dir_entry->CreateTimestamp,
+						TM_CREATE);
+			fs_func->get_entry_time(ep, &dir_entry->ModifyTimestamp,
+						TM_MODIFY);
 			memset((char *)&dir_entry->AccessTimestamp, 0,
-			       sizeof(struct date_time_t));
+			       sizeof(struct timestamp_t));
 
 			*(uni_name.name) = 0x0;
 			fs_func->get_uni_name_from_ext_entry(sb, &dir, dentry,
@@ -3349,9 +3273,9 @@ static int exfat_fill_inode(struct inode *inode, struct file_id_t *fid)
 	inode->i_blocks = ((i_size_read(inode) + (p_fs->cluster_size - 1))
 				& ~((loff_t)p_fs->cluster_size - 1)) >> 9;
 
-	exfat_time_fat2unix(sbi, &inode->i_mtime, &info.ModifyTimestamp);
-	exfat_time_fat2unix(sbi, &inode->i_ctime, &info.CreateTimestamp);
-	exfat_time_fat2unix(sbi, &inode->i_atime, &info.AccessTimestamp);
+	exfat_time_fat2unix(&inode->i_mtime, &info.ModifyTimestamp);
+	exfat_time_fat2unix(&inode->i_ctime, &info.CreateTimestamp);
+	exfat_time_fat2unix(&inode->i_atime, &info.AccessTimestamp);
 
 	return 0;
 }
@@ -3412,8 +3336,6 @@ static void exfat_destroy_inode(struct inode *inode)
 
 static int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
 {
-	struct super_block *sb = inode->i_sb;
-	struct exfat_sb_info *sbi = EXFAT_SB(sb);
 	struct dir_entry_t info;
 
 	if (inode->i_ino == EXFAT_ROOT_INO)
@@ -3422,9 +3344,9 @@ static int exfat_write_inode(struct inode *inode, struct writeback_control *wbc)
 	info.Attr = exfat_make_attr(inode);
 	info.Size = i_size_read(inode);
 
-	exfat_time_unix2fat(sbi, &inode->i_mtime, &info.ModifyTimestamp);
-	exfat_time_unix2fat(sbi, &inode->i_ctime, &info.CreateTimestamp);
-	exfat_time_unix2fat(sbi, &inode->i_atime, &info.AccessTimestamp);
+	exfat_time_unix2fat(&inode->i_mtime, &info.ModifyTimestamp);
+	exfat_time_unix2fat(&inode->i_ctime, &info.CreateTimestamp);
+	exfat_time_unix2fat(&inode->i_atime, &info.AccessTimestamp);
 
 	ffsWriteStat(inode, &info);
 
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: exfat: add millisecond support
  2019-09-08 12:48 [PATCH] staging: exfat: add millisecond support Valentin Vidic
@ 2019-09-08 13:03 ` Greg Kroah-Hartman
  2019-09-08 14:47   ` Valentin Vidić
  0 siblings, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 13:03 UTC (permalink / raw)
  To: Valentin Vidic; +Cc: devel, Valdis Kletnieks, linux-kernel

On Sun, Sep 08, 2019 at 12:48:08PM +0000, Valentin Vidic wrote:
> Drop duplicated date_time_t struct and add millisecond handling for
> create and modify time. Also drop millisecond field for access time
> since it is not defined in the the spec.
> 
> Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
> ---
>  drivers/staging/exfat/exfat.h       |  38 +++---
>  drivers/staging/exfat/exfat_core.c  |  31 ++++-
>  drivers/staging/exfat/exfat_super.c | 174 ++++++++--------------------
>  3 files changed, 92 insertions(+), 151 deletions(-)

Please run checkpatch on your patches so that we don't have to go and
fix up those issues later on.

Also, can you break this up into smaller patches please?  You are doing
multiple things all at once.

And, are you sure about the millisecond field for access time stuff?  It
was obviously added for some reason (there are lots in the spec that the
code does not yet cover, this seems odd being the other way around).
Did you test it against any other operating system exfat images to
ensure that it really is not being used at all?  If so, which ones?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: exfat: add millisecond support
  2019-09-08 13:03 ` Greg Kroah-Hartman
@ 2019-09-08 14:47   ` Valentin Vidić
  2019-09-08 16:43     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 4+ messages in thread
From: Valentin Vidić @ 2019-09-08 14:47 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: devel, Valdis Kletnieks, linux-kernel

On Sun, Sep 08, 2019 at 02:03:37PM +0100, Greg Kroah-Hartman wrote:
> Please run checkpatch on your patches so that we don't have to go and
> fix up those issues later on.

Strange, it did not report anything for me:

total: 0 errors, 0 warnings, 0 checks, 439 lines checked
0001-staging-exfat-add-millisecond-support.patch has no obvious style problems and is ready for submission.

> Also, can you break this up into smaller patches please?  You are doing
> multiple things all at once.

Sure, I was just trying to improve the code a bit :)

> And, are you sure about the millisecond field for access time stuff?  It
> was obviously added for some reason (there are lots in the spec that the
> code does not yet cover, this seems odd being the other way around).
> Did you test it against any other operating system exfat images to
> ensure that it really is not being used at all?  If so, which ones?

Don't really have access to another OS, but here is what exfat-fuse has:

struct exfat_entry_meta1                        /* file or directory info (part 1) */
{
        uint8_t type;                                   /* EXFAT_ENTRY_FILE */
        uint8_t continuations;
        le16_t checksum;
        le16_t attrib;                                  /* combination of EXFAT_ATTRIB_xxx */
        le16_t __unknown1;
        le16_t crtime, crdate;                  /* creation date and time */
        le16_t mtime, mdate;                    /* latest modification date and time */
        le16_t atime, adate;                    /* latest access date and time */
        uint8_t crtime_cs;                              /* creation time in cs (centiseconds) */
        uint8_t mtime_cs;                               /* latest modification time in cs */
        uint8_t __unknown2[10];
}

The spec matches this and defines 3 additional UtcOffset fields that we don't use:

EntryType
SecondaryCount
SetChecksum
FileAttributes
Reserved1
CreateTimestamp
LastModifiedTimestamp
LastAccessedTimestamp
Create10msIncrement
LastModified10msIncrement

CreateUtcOffset (1 byte)
LastModifiedUtcOffset (1 byte)
LastAccessedUtcOffset (1 byte)
Reserved2 (7 bytes)

So I'm not sure where access_time_ms came from. In any case it was always set to
0 so it should not matter much?

-- 
Valentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] staging: exfat: add millisecond support
  2019-09-08 14:47   ` Valentin Vidić
@ 2019-09-08 16:43     ` Greg Kroah-Hartman
  0 siblings, 0 replies; 4+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-08 16:43 UTC (permalink / raw)
  To: Valentin Vidić; +Cc: devel, Valdis Kletnieks, linux-kernel

On Sun, Sep 08, 2019 at 02:47:35PM +0000, Valentin Vidić wrote:
> On Sun, Sep 08, 2019 at 02:03:37PM +0100, Greg Kroah-Hartman wrote:
> > Please run checkpatch on your patches so that we don't have to go and
> > fix up those issues later on.
> 
> Strange, it did not report anything for me:
> 
> total: 0 errors, 0 warnings, 0 checks, 439 lines checked
> 0001-staging-exfat-add-millisecond-support.patch has no obvious style problems and is ready for submission.

See my response to the broken out patch as to where it should have
complained.

> > Also, can you break this up into smaller patches please?  You are doing
> > multiple things all at once.
> 
> Sure, I was just trying to improve the code a bit :)

No problem, it's much appreciated.

> > And, are you sure about the millisecond field for access time stuff?  It
> > was obviously added for some reason (there are lots in the spec that the
> > code does not yet cover, this seems odd being the other way around).
> > Did you test it against any other operating system exfat images to
> > ensure that it really is not being used at all?  If so, which ones?
> 
> Don't really have access to another OS, but here is what exfat-fuse has:
> 
> struct exfat_entry_meta1                        /* file or directory info (part 1) */
> {
>         uint8_t type;                                   /* EXFAT_ENTRY_FILE */
>         uint8_t continuations;
>         le16_t checksum;
>         le16_t attrib;                                  /* combination of EXFAT_ATTRIB_xxx */
>         le16_t __unknown1;
>         le16_t crtime, crdate;                  /* creation date and time */
>         le16_t mtime, mdate;                    /* latest modification date and time */
>         le16_t atime, adate;                    /* latest access date and time */
>         uint8_t crtime_cs;                              /* creation time in cs (centiseconds) */
>         uint8_t mtime_cs;                               /* latest modification time in cs */
>         uint8_t __unknown2[10];
> }
> 
> The spec matches this and defines 3 additional UtcOffset fields that we don't use:

I would only go off of the spec, who knows where exfat-fuse got its
information from :)

> EntryType
> SecondaryCount
> SetChecksum
> FileAttributes
> Reserved1
> CreateTimestamp
> LastModifiedTimestamp
> LastAccessedTimestamp
> Create10msIncrement
> LastModified10msIncrement
> 
> CreateUtcOffset (1 byte)
> LastModifiedUtcOffset (1 byte)
> LastAccessedUtcOffset (1 byte)
> Reserved2 (7 bytes)
> 
> So I'm not sure where access_time_ms came from. In any case it was always set to
> 0 so it should not matter much?

If it really is CreateUtcOffset, we should use that.

For things like messing with fields, testing on another operating system
to ensure we got this right is going to be very essential.  Virtual
machines of osx or windows might be a good way to do that.

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-08 12:48 [PATCH] staging: exfat: add millisecond support Valentin Vidic
2019-09-08 13:03 ` Greg Kroah-Hartman
2019-09-08 14:47   ` Valentin Vidić
2019-09-08 16:43     ` Greg Kroah-Hartman

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org driverdev-devel@archiver.kernel.org
	public-inbox-index driverdev-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox