32 bit systems using 'struct timeval' will break in the year 2038, so we modify the code appropriately.
This patch replaces the use of struct timeval and do_gettimeofday() with ktime_get_real_seconds() which returns a 64 bit value which is safer than struct timeval.
This patch also replaces u32 time storing variables with u64 variables to store the 64 bit seconds value returned by ktime_get_real_seconds()
Signed-off-by: Amitoj Kaur Chawla amitoj1606@gmail.com --- Changes in v2: -Converted u32 type to u64 type Changes in v3: -Converted u32 type to u64 type
drivers/scsi/bfa/bfa_defs_svc.h | 2 +- drivers/scsi/bfa/bfa_fcpim.c | 2 +- drivers/scsi/bfa/bfa_fcpim.h | 4 ++-- drivers/scsi/bfa/bfad_bsg.c | 6 +++--- 4 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/bfa/bfa_defs_svc.h b/drivers/scsi/bfa/bfa_defs_svc.h index e7acf41..0a31561 100644 --- a/drivers/scsi/bfa/bfa_defs_svc.h +++ b/drivers/scsi/bfa/bfa_defs_svc.h @@ -1211,7 +1211,7 @@ struct bfa_itnim_ioprofile_s { u32 clock_res_mul; u32 clock_res_div; u32 index; - u32 io_profile_start_time; /* IO profile start time */ + u64 io_profile_start_time; /* IO profile start time */ u32 iocomps[BFA_IOBUCKET_MAX]; /* IO completed */ struct bfa_itnim_latency_s io_latency; }; diff --git a/drivers/scsi/bfa/bfa_fcpim.c b/drivers/scsi/bfa/bfa_fcpim.c index d7385d1..224db4e 100644 --- a/drivers/scsi/bfa/bfa_fcpim.c +++ b/drivers/scsi/bfa/bfa_fcpim.c @@ -468,7 +468,7 @@ bfa_ioim_profile_start(struct bfa_ioim_s *ioim) }
bfa_status_t -bfa_fcpim_profile_on(struct bfa_s *bfa, u32 time) +bfa_fcpim_profile_on(struct bfa_s *bfa, u64 time) { struct bfa_itnim_s *itnim; struct bfa_fcpim_s *fcpim = BFA_FCPIM(bfa); diff --git a/drivers/scsi/bfa/bfa_fcpim.h b/drivers/scsi/bfa/bfa_fcpim.h index e693af6..274e3af 100644 --- a/drivers/scsi/bfa/bfa_fcpim.h +++ b/drivers/scsi/bfa/bfa_fcpim.h @@ -135,7 +135,7 @@ struct bfa_fcpim_s { struct bfa_fcpim_del_itn_stats_s del_itn_stats; bfa_boolean_t ioredirect; bfa_boolean_t io_profile; - u32 io_profile_start_time; + u64 io_profile_start_time; bfa_fcpim_profile_t profile_comp; bfa_fcpim_profile_t profile_start; }; @@ -309,7 +309,7 @@ bfa_status_t bfa_fcpim_port_iostats(struct bfa_s *bfa, struct bfa_itnim_iostats_s *stats, u8 lp_tag); void bfa_fcpim_add_stats(struct bfa_itnim_iostats_s *fcpim_stats, struct bfa_itnim_iostats_s *itnim_stats); -bfa_status_t bfa_fcpim_profile_on(struct bfa_s *bfa, u32 time); +bfa_status_t bfa_fcpim_profile_on(struct bfa_s *bfa, u64 time); bfa_status_t bfa_fcpim_profile_off(struct bfa_s *bfa);
#define bfa_fcpim_ioredirect_enabled(__bfa) \ diff --git a/drivers/scsi/bfa/bfad_bsg.c b/drivers/scsi/bfa/bfad_bsg.c index 023b9d4..92f5b0f 100644 --- a/drivers/scsi/bfa/bfad_bsg.c +++ b/drivers/scsi/bfa/bfad_bsg.c @@ -15,6 +15,7 @@ * General Public License for more details. */
+#include <linux/ktime.h> #include <linux/uaccess.h> #include "bfad_drv.h" #include "bfad_im.h" @@ -2093,13 +2094,12 @@ bfad_iocmd_fcpim_cfg_profile(struct bfad_s *bfad, void *cmd, unsigned int v_cmd) { struct bfa_bsg_fcpim_profile_s *iocmd = (struct bfa_bsg_fcpim_profile_s *)cmd; - struct timeval tv; unsigned long flags;
- do_gettimeofday(&tv); spin_lock_irqsave(&bfad->bfad_lock, flags); if (v_cmd == IOCMD_FCPIM_PROFILE_ON) - iocmd->status = bfa_fcpim_profile_on(&bfad->bfa, tv.tv_sec); + iocmd->status = bfa_fcpim_profile_on(&bfad->bfa, + ktime_get_real_seconds()); else if (v_cmd == IOCMD_FCPIM_PROFILE_OFF) iocmd->status = bfa_fcpim_profile_off(&bfad->bfa); spin_unlock_irqrestore(&bfad->bfad_lock, flags);
On Saturday 07 November 2015 02:36:35 Amitoj Kaur Chawla wrote:
diff --git a/drivers/scsi/bfa/bfa_defs_svc.h b/drivers/scsi/bfa/bfa_defs_svc.h index e7acf41..0a31561 100644 --- a/drivers/scsi/bfa/bfa_defs_svc.h +++ b/drivers/scsi/bfa/bfa_defs_svc.h @@ -1211,7 +1211,7 @@ struct bfa_itnim_ioprofile_s { u32 clock_res_mul; u32 clock_res_div; u32 index;
- u32 io_profile_start_time; /* IO profile start time */
- u64 io_profile_start_time; /* IO profile start time */ u32 iocomps[BFA_IOBUCKET_MAX]; /* IO completed */ struct bfa_itnim_latency_s io_latency;
};
How did you determine whether this is a safe change? What code reads the variable and what is done with it afterwards?
Arnd
On Sat, Nov 7, 2015 at 3:01 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 07 November 2015 02:36:35 Amitoj Kaur Chawla wrote:
diff --git a/drivers/scsi/bfa/bfa_defs_svc.h b/drivers/scsi/bfa/bfa_defs_svc.h index e7acf41..0a31561 100644 --- a/drivers/scsi/bfa/bfa_defs_svc.h +++ b/drivers/scsi/bfa/bfa_defs_svc.h @@ -1211,7 +1211,7 @@ struct bfa_itnim_ioprofile_s { u32 clock_res_mul; u32 clock_res_div; u32 index;
u32 io_profile_start_time; /* IO profile start time */
u64 io_profile_start_time; /* IO profile start time */ u32 iocomps[BFA_IOBUCKET_MAX]; /* IO completed */ struct bfa_itnim_latency_s io_latency;};
How did you determine whether this is a safe change? What code reads the variable and what is done with it afterwards?
Arnd
The only place bfa_itnim_ioprofile_s is used is in the function:
bfa_status_t bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, struct bfa_itnim_ioprofile_s *ioprofile) { .... itnim->ioprofile.io_profile_start_time = bfa_io_profile_start_time(itnim->bfa); .... }
bfa_io_profile_start_time is the macro:
#define bfa_io_profile_start_time(_bfa) \ ((_bfa)->modules.fcp_mod.fcpim.io_profile_start_time);
As far as I understood in the above code is that fcpim.io_profile_start_time is the structure bfa_fcpim_s io_profile_start_time variable.
Since we are talking about both structures' io_profile_start_time variable and converting both of them to u64, I thought it would be a safe change.
On Saturday 07 November 2015 04:08:16 Amitoj Kaur Chawla wrote:
On Sat, Nov 7, 2015 at 3:01 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 07 November 2015 02:36:35 Amitoj Kaur Chawla wrote:
diff --git a/drivers/scsi/bfa/bfa_defs_svc.h b/drivers/scsi/bfa/bfa_defs_svc.h index e7acf41..0a31561 100644 --- a/drivers/scsi/bfa/bfa_defs_svc.h +++ b/drivers/scsi/bfa/bfa_defs_svc.h
The only place bfa_itnim_ioprofile_s is used is in the function:
bfa_status_t bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, struct bfa_itnim_ioprofile_s *ioprofile) { .... itnim->ioprofile.io_profile_start_time = bfa_io_profile_start_time(itnim->bfa); .... }
bfa_io_profile_start_time is the macro:
#define bfa_io_profile_start_time(_bfa) \ ((_bfa)->modules.fcp_mod.fcpim.io_profile_start_time);
As far as I understood in the above code is that fcpim.io_profile_start_time is the structure bfa_fcpim_s io_profile_start_time variable.
Since we are talking about both structures' io_profile_start_time variable and converting both of them to u64, I thought it would be a safe change.
What I wrote was "Please try to find out if the ioprofile structure can be changed as well, and then either change it as well, or make document the place where the upper bits are lost, whichever is correct."
If the above was the only use of the variable, it would be write-only, and the correct change would be to eliminate it throughout the driver.
For all I can tell, the ioprofile variable is not unused, and its layout is fixed, so we cannot remove the io_profile_start_time or change it to another format. Can you look again and see how it gets used by the driver?
Arnd
On Sat, Nov 7, 2015 at 4:26 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 07 November 2015 04:08:16 Amitoj Kaur Chawla wrote:
On Sat, Nov 7, 2015 at 3:01 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 07 November 2015 02:36:35 Amitoj Kaur Chawla wrote:
The only place bfa_itnim_ioprofile_s is used is in the function:
bfa_status_t bfa_itnim_get_ioprofile(struct bfa_itnim_s *itnim, struct bfa_itnim_ioprofile_s *ioprofile) { .... itnim->ioprofile.io_profile_start_time = bfa_io_profile_start_time(itnim->bfa); .... }
bfa_io_profile_start_time is the macro:
#define bfa_io_profile_start_time(_bfa) \ ((_bfa)->modules.fcp_mod.fcpim.io_profile_start_time);
As far as I understood in the above code is that fcpim.io_profile_start_time is the structure bfa_fcpim_s io_profile_start_time variable.
Since we are talking about both structures' io_profile_start_time variable and converting both of them to u64, I thought it would be a safe change.
What I wrote was "Please try to find out if the ioprofile structure can be changed as well, and then either change it as well, or make document the place where the upper bits are lost, whichever is correct."
If the above was the only use of the variable, it would be write-only, and the correct change would be to eliminate it throughout the driver.
For all I can tell, the ioprofile variable is not unused, and its layout is fixed, so we cannot remove the io_profile_start_time or change it to another format. Can you look again and see how it gets used by the driver?
Arnd
I checked again and apart from the above code I mentioned, the ioprofile variable is used in two structures:
bfa_itnim_s which is defined in bfa_fcpim.h file
bfa_bsg_itnim_ioprofile_s defined in bfad_bsg.h file
However, the io_profile_start_time variable is not used anywhere else.
I'm sorry but I didn't understand why the layout is fixed?
On Saturday 07 November 2015 19:47:49 Amitoj Kaur Chawla wrote:
On Sat, Nov 7, 2015 at 4:26 AM, Arnd Bergmann arnd@arndb.de wrote:
For all I can tell, the ioprofile variable is not unused, and its layout is fixed, so we cannot remove the io_profile_start_time or change it to another format. Can you look again and see how it gets used by the driver?
I checked again and apart from the above code I mentioned, the ioprofile variable is used in two structures:
bfa_itnim_s which is defined in bfa_fcpim.h file bfa_bsg_itnim_ioprofile_s defined in bfad_bsg.h fileHowever, the io_profile_start_time variable is not used anywhere else.
I'm sorry but I didn't understand why the layout is fixed?
The way to find it is as follows: it's normally a reasonable assumption that you can 'git grep' for a struct member by name and find all users. However, if you find that it's write-only or read-only, it's more likely that grep is missing something important compared with a bug in the driver.
When the struct member is not used anywhere but only written, look for any code location that copies the entire struct, or another struct that contains a reference to this struct. (Same thing if it's never written but only read).
The ioprofile data gets passed around through a small number of functions, try to understand what kind of data it contains and where it gets stored in the end. I admit this is a bit tricky here, in this case you can stop tracing the callers at the point where you get to FC_BSG_HST_VENDOR and find out what that is about.
Arnd
On Sun, Nov 8, 2015 at 3:23 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 07 November 2015 19:47:49 Amitoj Kaur Chawla wrote:
On Sat, Nov 7, 2015 at 4:26 AM, Arnd Bergmann arnd@arndb.de wrote:
For all I can tell, the ioprofile variable is not unused, and its layout is fixed, so we cannot remove the io_profile_start_time or change it to another format. Can you look again and see how it gets used by the driver?
I checked again and apart from the above code I mentioned, the ioprofile variable is used in two structures:
bfa_itnim_s which is defined in bfa_fcpim.h file bfa_bsg_itnim_ioprofile_s defined in bfad_bsg.h fileHowever, the io_profile_start_time variable is not used anywhere else.
I'm sorry but I didn't understand why the layout is fixed?
The way to find it is as follows: it's normally a reasonable assumption that you can 'git grep' for a struct member by name and find all users. However, if you find that it's write-only or read-only, it's more likely that grep is missing something important compared with a bug in the driver.
When the struct member is not used anywhere but only written, look for any code location that copies the entire struct, or another struct that contains a reference to this struct. (Same thing if it's never written but only read).
The ioprofile data gets passed around through a small number of functions, try to understand what kind of data it contains and where it gets stored in the end. I admit this is a bit tricky here, in this case you can stop tracing the callers at the point where you get to FC_BSG_HST_VENDOR and find out what that is about.
Arnd
After looking into this further the struct bfa_itnim_ioprofile_s *ioprofile data is being used in several structures in this way:
the structure "bfa_itnim_ioprofile_s" referenced by the structure "bfa_itnim_s" referenced by the structure "bfa_s" referenced by the structure "bfad_s" referenced by bfad_im_bsg_vendor_request() function
The bfad_im_bsg_vendor_request() function is called for the case FC_BSG_HST_VENDOR in bfad_bsg.c file
On Tuesday 10 November 2015 19:27:49 Amitoj Kaur Chawla wrote:
On Sun, Nov 8, 2015 at 3:23 AM, Arnd Bergmann arnd@arndb.de wrote:
On Saturday 07 November 2015 19:47:49 Amitoj Kaur Chawla wrote:
On Sat, Nov 7, 2015 at 4:26 AM, Arnd Bergmann arnd@arndb.de wrote:
For all I can tell, the ioprofile variable is not unused, and its layout is fixed, so we cannot remove the io_profile_start_time or change it to another format. Can you look again and see how it gets used by the driver?
I checked again and apart from the above code I mentioned, the ioprofile variable is used in two structures:
bfa_itnim_s which is defined in bfa_fcpim.h file bfa_bsg_itnim_ioprofile_s defined in bfad_bsg.h fileHowever, the io_profile_start_time variable is not used anywhere else.
I'm sorry but I didn't understand why the layout is fixed?
The way to find it is as follows: it's normally a reasonable assumption that you can 'git grep' for a struct member by name and find all users. However, if you find that it's write-only or read-only, it's more likely that grep is missing something important compared with a bug in the driver.
When the struct member is not used anywhere but only written, look for any code location that copies the entire struct, or another struct that contains a reference to this struct. (Same thing if it's never written but only read).
The ioprofile data gets passed around through a small number of functions, try to understand what kind of data it contains and where it gets stored in the end. I admit this is a bit tricky here, in this case you can stop tracing the callers at the point where you get to FC_BSG_HST_VENDOR and find out what that is about.
After looking into this further the struct bfa_itnim_ioprofile_s *ioprofile data is being used in several structures in this way:
the structure "bfa_itnim_ioprofile_s" referenced by the structure "bfa_itnim_s" referenced by the structure "bfa_s" referenced by the structure "bfad_s" referenced by bfad_im_bsg_vendor_request() function
The bfad_im_bsg_vendor_request() function is called for the case FC_BSG_HST_VENDOR in bfad_bsg.c file
Correct. So this is a vendor specific command that can get sent from user space to retrieve the profile data. We can't change that format because we don't want to break existing applications using it, so this has to be documented carefully.
An easy way out would be to leave a cast to 'time_t' in the last place that references this, to leave it up to the next person who wants to clean up the driver. Then you only do the other parts that have an easier solution.
You could also do the same thing as a an intermediate step, and do a final patch on top that removes the time_t and explains in the changelog what the impact of removing that is.
Arnd
On Tue, Nov 10, 2015 at 10:01 PM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 10 November 2015 19:27:49 Amitoj Kaur Chawla wrote:
After looking into this further the struct bfa_itnim_ioprofile_s *ioprofile data is being used in several structures in this way:
the structure "bfa_itnim_ioprofile_s" referenced by the structure "bfa_itnim_s" referenced by the structure "bfa_s" referenced by the structure "bfad_s" referenced by bfad_im_bsg_vendor_request() function
The bfad_im_bsg_vendor_request() function is called for the case FC_BSG_HST_VENDOR in bfad_bsg.c file
Correct. So this is a vendor specific command that can get sent from user space to retrieve the profile data. We can't change that format because we don't want to break existing applications using it, so this has to be documented carefully.
An easy way out would be to leave a cast to 'time_t' in the last place that references this, to leave it up to the next person who wants to clean up the driver. Then you only do the other parts that have an easier solution.
Just to ensure that I'm understanding correctly,
So this is what we have presently: struct bfa_itnim_ioprofile_s { ... u32 io_profile_start_time; /* IO profile start time */ ... };
and I make the following change:
struct bfa_itnim_ioprofile_s { ... time_t io_profile_start_time; /* IO profile start time */ ... };
You could also do the same thing as a an intermediate step, and do a final patch on top that removes the time_t and explains in the changelog what the impact of removing that is.
Arnd
The above change would become an intermediate change, and I send a separate(final) patch using time64_t? Also, explain in the changelog that this being used in a vendor specific command to retrieve profile data in both patches and will have an impact when the command is sent from user space?
On Tuesday 10 November 2015 22:21:50 Amitoj Kaur Chawla wrote:
On Tue, Nov 10, 2015 at 10:01 PM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 10 November 2015 19:27:49 Amitoj Kaur Chawla wrote:
After looking into this further the struct bfa_itnim_ioprofile_s *ioprofile data is being used in several structures in this way:
the structure "bfa_itnim_ioprofile_s" referenced by the structure "bfa_itnim_s" referenced by the structure "bfa_s" referenced by the structure "bfad_s" referenced by bfad_im_bsg_vendor_request() function
The bfad_im_bsg_vendor_request() function is called for the case FC_BSG_HST_VENDOR in bfad_bsg.c file
Correct. So this is a vendor specific command that can get sent from user space to retrieve the profile data. We can't change that format because we don't want to break existing applications using it, so this has to be documented carefully.
An easy way out would be to leave a cast to 'time_t' in the last place that references this, to leave it up to the next person who wants to clean up the driver. Then you only do the other parts that have an easier solution.
Just to ensure that I'm understanding correctly,
So this is what we have presently: struct bfa_itnim_ioprofile_s { ... u32 io_profile_start_time; /* IO profile start time */ ... };
and I make the following change:
struct bfa_itnim_ioprofile_s { ... time_t io_profile_start_time; /* IO profile start time */ ... };
No, that would be broken on 64-bit systems because you are changing the layout of the structure. I meant adding a cast to the assignment as in
itnim->ioprofile.io_profile_start_time = (u32)(time_t)bfa_io_profile_start_time(itnim->bfa);
There are many ways to do this, the above is just what I'd pick to make it obvious that this is where we lose the upper bits. In particular, this will still show up as a build error when we finally remove the time_t definition.
You could also do the same thing as a an intermediate step, and do a final patch on top that removes the time_t and explains in the changelog what the impact of removing that is.
The above change would become an intermediate change, and I send a separate(final) patch using time64_t?
The final patch would remove the time_t cast, to make it build when that is no longer defined. I'm unsure about whether we want this change to be part of the kernel though, so keeping it separate gives the maintainer the choice whether to apply it or not.
Also, explain in the changelog that this being used in a vendor specific command to retrieve profile data in both patches and will have an impact when the command is sent from user space?
yes, that definitely helps.
Arnd
On Wed, Nov 11, 2015 at 1:22 AM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 10 November 2015 22:21:50 Amitoj Kaur Chawla wrote:
On Tue, Nov 10, 2015 at 10:01 PM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 10 November 2015 19:27:49 Amitoj Kaur Chawla wrote:
After looking into this further the struct bfa_itnim_ioprofile_s *ioprofile data is being used in several structures in this way:
the structure "bfa_itnim_ioprofile_s" referenced by the structure "bfa_itnim_s" referenced by the structure "bfa_s" referenced by the structure "bfad_s" referenced by bfad_im_bsg_vendor_request() function
The bfad_im_bsg_vendor_request() function is called for the case FC_BSG_HST_VENDOR in bfad_bsg.c file
Correct. So this is a vendor specific command that can get sent from user space to retrieve the profile data. We can't change that format because we don't want to break existing applications using it, so this has to be documented carefully.
An easy way out would be to leave a cast to 'time_t' in the last place that references this, to leave it up to the next person who wants to clean up the driver. Then you only do the other parts that have an easier solution.
Just to ensure that I'm understanding correctly,
So this is what we have presently: struct bfa_itnim_ioprofile_s { ... u32 io_profile_start_time; /* IO profile start time */ ... };
and I make the following change:
struct bfa_itnim_ioprofile_s { ... time_t io_profile_start_time; /* IO profile start time */ ... };
No, that would be broken on 64-bit systems because you are changing the layout of the structure. I meant adding a cast to the assignment as in
itnim->ioprofile.io_profile_start_time = (u32)(time_t)bfa_io_profile_start_time(itnim->bfa);There are many ways to do this, the above is just what I'd pick to make it obvious that this is where we lose the upper bits. In particular, this will still show up as a build error when we finally remove the time_t definition.
You could also do the same thing as a an intermediate step, and do a final patch on top that removes the time_t and explains in the changelog what the impact of removing that is.
The above change would become an intermediate change, and I send a separate(final) patch using time64_t?
The final patch would remove the time_t cast, to make it build when that is no longer defined. I'm unsure about whether we want this change to be part of the kernel though, so keeping it separate gives the maintainer the choice whether to apply it or not.
I don't get a build error when I remove the time_t cast. Where am I going wrong?
On Thursday 12 November 2015 14:12:52 Amitoj Kaur Chawla wrote:
On Wed, Nov 11, 2015 at 1:22 AM, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 10 November 2015 22:21:50 Amitoj Kaur Chawla wrote:
On Tue, Nov 10, 2015 at 10:01 PM, Arnd Bergmann arnd@arndb.de wrote:
You could also do the same thing as a an intermediate step, and do a final patch on top that removes the time_t and explains in the changelog what the impact of removing that is.
The above change would become an intermediate change, and I send a separate(final) patch using time64_t?
The final patch would remove the time_t cast, to make it build when that is no longer defined. I'm unsure about whether we want this change to be part of the kernel though, so keeping it separate gives the maintainer the choice whether to apply it or not.
I don't get a build error when I remove the time_t cast. Where am I going wrong?
I meant the reverse: if we remove the definition of time_t, we get a build error unless we also remove that cast.
At the moment, removing the time_t definition will break countless other things in the kernel, so that's hard to try out, but you can imagine what error you would get.
Arnd
On Thu, Nov 12, 2015 at 2:55 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 12 November 2015 14:12:52 Amitoj Kaur Chawla wrote:
I don't get a build error when I remove the time_t cast. Where am I going wrong?
I meant the reverse: if we remove the definition of time_t, we get a build error unless we also remove that cast.
At the moment, removing the time_t definition will break countless other things in the kernel, so that's hard to try out, but you can imagine what error you would get.
Arnd
Oh okay. I got it now. Thank you. So keeping in mind that we are eventually removing the time_t definition from the kernel since that is our goal I should send a patch on top.
On Thursday 12 November 2015 17:52:51 Amitoj Kaur Chawla wrote:
On Thu, Nov 12, 2015 at 2:55 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 12 November 2015 14:12:52 Amitoj Kaur Chawla wrote:
I don't get a build error when I remove the time_t cast. Where am I going wrong?
I meant the reverse: if we remove the definition of time_t, we get a build error unless we also remove that cast.
At the moment, removing the time_t definition will break countless other things in the kernel, so that's hard to try out, but you can imagine what error you would get.
Oh okay. I got it now. Thank you. So keeping in mind that we are eventually removing the time_t definition from the kernel since that is our goal I should send a patch on top.
Ok. Just be aware that this one-line patch is the hardest part here, because the patch description needs to give the reviewers all the information to decide whether to take it as-is and live with the overflow in the future, or redesign their ioctl interface and change their user-space side instead.
Arnd
On Thu, Nov 12, 2015 at 6:37 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 12 November 2015 17:52:51 Amitoj Kaur Chawla wrote:
On Thu, Nov 12, 2015 at 2:55 PM, Arnd Bergmann arnd@arndb.de wrote:
On Thursday 12 November 2015 14:12:52 Amitoj Kaur Chawla wrote:
I don't get a build error when I remove the time_t cast. Where am I going wrong?
I meant the reverse: if we remove the definition of time_t, we get a build error unless we also remove that cast.
At the moment, removing the time_t definition will break countless other things in the kernel, so that's hard to try out, but you can imagine what error you would get.
Oh okay. I got it now. Thank you. So keeping in mind that we are eventually removing the time_t definition from the kernel since that is our goal I should send a patch on top.
Ok. Just be aware that this one-line patch is the hardest part here, because the patch description needs to give the reviewers all the information to decide whether to take it as-is and live with the overflow in the future, or redesign their ioctl interface and change their user-space side instead.
Arnd
Oh okay. I'll do my best at describe everything in the changelog.
-
Amitoj Kaur Chawla -
Arnd Bergmann