search

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

Cleanup/Remove version.h #90

Closed jaftrain closed 7 years ago

jaftrain commented 7 years ago

Kanboard 24482 GitHub Issue 90 GitHub Issue 90 Branch

davidker commented 7 years ago

Jon, when you git finished with your changes, can you look at github-issue #71 and refactor its solution?

davidker commented 7 years ago

Patches look good for this series. But looking through the code, I'm wondering about the version used in the struct visor_driver. It contains the version and I'm wondering if we no longer need that.

In the function fix_vbus_dev_info we call: bus_device_info_init(&dev_info, chan_type_name, visordrv->name, visordrv->version);

You updated the other calls to that function to use the kernel call, but not this one. I'm wondering if we need to call that. Maybe the fact that we aren't seeing the versions of the other drivers is because we are having issues calling fix_vbus_dev_info. @selltc, thoughts as well?

jaftrain commented 7 years ago

Looks like I missed the const char *version in struct visor_driver. It's only being set to 1.0.0.0 in visornic_main.c so I don't see why it shouldn't be removed.

As for fix_vbus_dev_info() I initially thought it was fixing the info for a driver by using existing info, but it looks to be filling it it, so I can update that bus_device_info_init call with the kernel version.

I think the reason we are not seeing the version of the drivers is because bus_device_info_init() which populates the infostrsin the ultra_vbus_deviceinfois only ever called in visorbus_main.c.

jaftrain commented 7 years ago

Missed removing MODULE_VERSION("1.0.0.0") in visornic_main.c because it didn't include version.h. I'll address that.

davidker commented 7 years ago

Correct, but when the drivers register with the bus driver, we should be updating the bus_device_info to add them to the list of devices that are currently in use and what drivers they are. That work is probably outside of this github issue, but it is the beginnings of the issue to understand why we aren't logging functional driver version information. (And yes, functional driver is a Windows thing, sorry)

davidker commented 7 years ago

I'm wondering if we get rid of version or if we put the version of the kernel in version? What would be easier or are we saying if you have a Visorbus kernel driver you CAN NOT install our rpm drivers. @selltc do we have the ability to prevent that? How does it even know in modules?

selltc commented 7 years ago

I looked at commit 3d7ad09bb5de4379cdd4d6f109cc6e38c938c551 (stating: unisys: remove vertag), and added some comments there. Typo there: "stating" --> "staging".

selltc commented 7 years ago

In commit c66b6d661ab7ecdfcf591442b84bff7454d44094 (staging: unisys: visorbus: remove driver version from visorbus_main.c), you removed an argument passed in the invocations of bus_device_info_init(), but you didn't actually remove the corresponding parameter from the bus_device_info_init() function parameters until commit 3d7ad09 (stating: unisys: remove vertag). Doesn't this mean that the build would be broken between these 2 patches? If so, that's a no no.

selltc commented 7 years ago 
bus_device_info_init(&dev_info, chan_type_name,
visordrv->name, visordrv->version);

You updated the other calls to that function to use the kernel call, but not this one. I'm wondering if we > need to call that. Maybe the fact that we aren't seeing the versions of the other drivers is because we > are having issues calling fix_vbus_dev_info.

I agree this call should look like the other two, and pass in the kernel version.

Are you guys suggesting that when you take a livedump, that you do NOT see the driver versions being reported correctly in a livedump? E.g., take a livedump, crack it, open a part-*-Dump.txt for console, iovm, or interconnect, search for "client driver". E.g., looks like this for Diag guest in IOVM dump:

===== /proc/uislib/vbus/9/info =====
Client device / client driver info for s-Par Diag partition (vbus #9):
   chipset         uislib          4.3.0.25318 20141209AM Dec  9 2014 00:56:13 diag64
   clientbus       virtpci         4.3.0.25318 20141209AM Dec  9 2014 00:56:17 diag64
[0]vNIC            uisvirtnic      4.3.0.25318 20141209AM Dec  9 2014 00:56:20 diag64
[1]vHBA            uisvirthba      4.3.0.25318 20141209AM Dec  9 2014 00:56:19 diag64
=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*
davidker commented 7 years ago

Tim, correct when Jon takes the livedump we only see the list for the chipset and clientbus. We are still investigating what the problem is and it probably has to do something with our evil counterpart virtpci being missing.

selltc commented 7 years ago

correct when Jon takes the livedump we only see the list for the chipset and clientbus

If it helps, the info for the other ("function") drivers SHOULD be getting filled in when visordriver_probe_device() calls fix_vbus_dev_info().

are we saying if you have a Visorbus kernel driver you CAN NOT install our rpm drivers

I think we would be painting ourselves into a corner if we attempted to enforce that.

davidker commented 7 years ago

Jon, you might want to wait on you removing the version and instead just set the .version in visornic and the other drivers to the kernel version. That might be the simplest and lets us have the RPM version of other drivers and the kernel version of the bus driver.

selltc commented 7 years ago

I like that idea too, but I'm hoping the community won't yell, as from their perspective it's going to appear extraneous.

jaftrain commented 7 years ago

I cleaned up the patch series and pushed up the githubissue 90 branch addressing @selltc and @davidker's comments. I haven't run the smoke test yet to determine what is visible as on a live system / live dump. I'm planning on refactoring issue #71 githubissue-71-for-later based on the changes in this issue.

selltc commented 7 years ago

Tim, correct when Jon takes the livedump we only see the list for the chipset and clientbus.

Interesting. I just confirmed this same bad behavior with my rpm drivers on the for-later-rhel branch.

selltc commented 7 years ago

Comments on githubissue-71-for-later:

selltc commented 7 years ago

Tim, correct when Jon takes the livedump we only see the list for the chipset and clientbus.

Interesting. I just confirmed this same bad behavior with my rpm drivers on the for-later-rhel branch.

I've got most of this figured out, and will commit a fix shortly. It turns out there is more than one problem here, and want to get them all before claiming victory.

davidker commented 7 years ago

Nice Tim!!

I am interested in what the fixes are and will send them up as soon as they are available.

selltc commented 7 years ago

I committed a series of patches today, including the core fix for this problem:

commit 7fdf9485df1292c4ca1520aa55283ae123a5748b staging: unisys: visorbus: fix vbus info generated for s-Par livedumps

to the for-later-rhel branch:

davidker commented 7 years ago

Tim, are you planning on making upstream-next version of the patches or should I make those and send them upstream (at least for the ones that fix the problem).

selltc commented 7 years ago

I see you just submitted a version-related series for upstream-next. So a few of my for-later-rhel patches are moot for upstream-next, and others are different. I'll take a peek after I sync-up rhel in SVN.

selltc commented 7 years ago

For upstream-next, I have pushed 3 commits to githubissue#90 branch:

davidker commented 7 years ago

Tim, should you do an "unsigned int" instead of u32 for 0b86000 or are you just trying to keep that conversion consistent with what is in the channel? I wonder if we should just force the type case there if need be.

I also had to shrink your fix typos commit message, since it was starting to wrap. I'll cc you on my send-email so you can see the changes. Or you can just look in upstream-next.

Also, thankfully for-later seems to apply cleanly on top of the patches.

jaftrain commented 7 years ago

Comments on githubissue-71-for-later:

commit 4a9acae (staging: unisys: make MODULE_DESCRIPTIONs consistent) - in one place you use "buses", and another "busses". I don't really care which you use, just so you are consistent.

I am refactoring my patches for issue #71 based on the changes in this issue (#90) and your comments. I moved Kanboard 1375 back to work in progress. Once I push up the refactored patches I'll move the task to back to the Ready for Inspection column.

commit a37e935 (staging: unisys: visorbus: dump visor driver versions in bus channel) - I personally think this will have a better chance of flying if you don't change the device_info_init() parameters (i.e., remove the ker_ver parameter), and have the callers just pass VERSION instead of the utsname()->release() they were passing before. But I don't care if you explicitly use utsname()->release() within device_info_init() if you really want to also include the kernel version in the version string. But I am interested to hear your's and David's opinion.

commit 4768a15adc83ce87c5e98d53887422db63c21b78 addresses this. Let me know if this approach is suitable.

davidker commented 7 years ago

I would like to know the version of the driver more than the version of the OS, in general we don't know the version of OSes unless the customer supplies that to us, we do however historically know what levels of drivers they are running when looking at the hardware dumps. My biggest concern is can we tell when we are in a position of mixed drivers. I really really really need to know when we have half kernel drivers and half picked up from elsewhere.

davidker commented 7 years ago

Tim, I'm falling partition desktop testing with:

[kershnda@redhat-upstream bin]$ ssh tcsxeon-spar "bash -lc 'cd selltc && ./pdtest-upstreamlinux.sh'" xvfb-run: error: Xvfb failed to start pdtest failed: partition desktop execution failed

Though the console appears to be functional, so I am just chalking this up to the script for now.

selltc commented 7 years ago

commit 4768a15 addresses this. Let me know if this approach is suitable.

Yep. That commit looks good. Thanks.

I would like to know the version of the driver more than the version of the OS, in general we don't know the version of OSes unless the customer supplies that to us, we do however historically know what levels of drivers they are running when looking at the hardware dumps. My biggest concern is can we tell when we are in a position of mixed drivers. I really really really need to know when we have half kernel drivers and half picked up from elsewhere.

As we just discussed, my commits on the for-later-rhel branch indeed DO include the driver version instead of the OS version, and those rpm versions of the drivers are the ones where this will actually matter. Re mixed versions: we will never do a partial installation of the rpm drivers; i.e., an rpm will always include every driver. As we discussed, if someone builds and runs a kernel that includes inbuilt versions of our drivers (which would NOT be a typical scenario; it would require expert an expert), then installs the rpm, it is likely to work just fine, but will probably fail spectacularly when you attempt to boot it for the first time. We agreed that we would add some postcodes to the for-later-rhel drivers so that we could at least detect this scenario in a livedump if it ever happened.

selltc commented 7 years ago

Tim, I'm falling partition desktop testing with:

    [kershnda@redhat-upstream bin]$ ssh tcsxeon-spar "bash -lc 'cd selltc && ./pdtest-upstreamlinux.sh'"
    xvfb-run: error: Xvfb failed to start
    pdtest failed: partition desktop execution failed

Though the console appears to be functional, so I am just chalking this up to the script for now.

Typically, I've solved problems like that by killing an already-running hung xvfb process (on tcsxeon), e.g.:

[spar@tcsxeon ~]$ ps -ef | grep xvfb
spar     18103 18081  0 Sep15 ?        00:00:00 /bin/sh /usr/local/bin/xvfb-run --server-args=-screen 0 1024x768x16 /usr/lib/usyscon_partitiondesktop/usyscon.sh //t710-1-smsui /vmid={72120221-5200-1042-8031-C3C04F304C31} /playbackscript=/srv/fs/spar/selltc/pdtest-upstreamlinux-recording.txt
davidker commented 7 years ago

I killed the script, but also had to kill the server as well (the server name is Xvfb, so much for grep with the -i ) :)

selltc commented 7 years ago

Yeah, "xvfb-run" is the wrapper script that starts "Xvfb". Forgot about that; sorry.

selltc commented 7 years ago

Greg accepted these 3 commits into staging-next on 9/28/2016:

As we half-anticipated, however, Greg DID complain about the fact that our sysfs entry should be a debugfs entry:

You do realize that sysfs files are supposed to only be "one value per file", right? This is going to have to change soon, if you want this code out of staging.

I'll take these patches now, but please work on a sane API (this really looks like something for debugfs as no userspace tools should be relying on it...)

I think a debugfs entry should be ok, as I believe the client_bus_info sysfs entry as we currently have it is mainly only used as a debugging aid for us to confirm that the vbus info we have written is valid. If we are running a Linux without debugfs enabled, we can also validate this info by just taking an s-Par livedump.

If this is ok with you @davidker, I'll just create another issue for it.

I also suspect that we can now close this issue now that Greg has accepted the 3 patches; please correct me if I'm wrong.

davidker commented 7 years ago

I am fine with this and we should be able to close the ticket.