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