davidker / unisys

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

move guestlinuxdebug.h #68

Closed mriswyth closed 7 years ago

mriswyth commented 8 years ago

guestlinuxdebug.h is only used in visorbus. Can it be moved into visorbus and visorbus_main.c?

Kanban-1338

selltc commented 8 years ago

I was going to say fine, but then I noticed that everything involved with issuing postcodes is in guestlinuxdebug.h, and issuing postcodes is something that can be generally useful to any s-Par guest driver. Given that, should we still move it?

If visorbus provided an interface to its function drivers for issuing postcodes that did not require guestlinuxdebug.h, I would say go ahead and move it. But because it does NOT provide such an interface, I'm not sure.

selltc commented 8 years ago

Something I DID notice that didn't seem right was the fact guestlinuxdebug.h uses ISSUE_IO_VMCALL_POSTCODE_SEVERITY, but does NOT itself include the required header file that defines that, vmcallinterface.h. It instead relies on its users to also include vmcallinterface.h, but vmcallinterface.h is in the visorbus/ directory, NOT the include/ directory. That seems messed up. I think those files want to live in the same directory.

davidker commented 8 years ago

My vote is for visorbus to expose something to the other drivers if they want to use it

mriswyth commented 8 years ago

I have pushed a potential patchset at githubissue#68-upstream-next.

I'm pretty sure @davidker has asked us to base on a different branch now, but I started with upstream-next and I really view this push as discussion point. As the @selltc and @davidker discussion above shows this wasn't just a move guestlinuxdebug.h type update. I'm really not sold on the final result of my changes, but I do believe that there are specific commits that are worth doing even if we don't make it to the end of postcodes being done by a function in visorbus.

davidker commented 8 years ago

Basing off of upstream-next is probably still appropriate, but it isn't what I'm currently pushing. But I will work with it :). I really need to have things settle down at some point.

davidker commented 8 years ago

I like the changes. I'm wondering if we should squash e0d3f4f6d1d9a661bc2b092babbf368c3f15621e and 0f5fc9c014e19732494062fa0f0c5e72bfd6342f. Also I wonder if we should hold off exporting that function until we have a function driver wanting to use it. I'm fine at that point exporting it.

davidker commented 8 years ago

I have a concern with 3d45c4b381e0d0ea1b285a5c96bf2872b604f5d8, do we want to redefine postcode values without bumping versions of things? What tools we have as prescreening that look at POSTCODES might get confused when they decode them since they are using the older versions (and private versions) of the postcodes. I'm wondering if we should just reserve the fields.

mriswyth commented 8 years ago

I have pushed a new branch at githubissue#68-upstream-next-2. (My naming has gotten worse, but while discussion is active I didn't know that I wanted to overwrite a discussed branch).

I think that I have addressed all the concerns discussed and above. For this round I chose 0xFX as the values for the "upstream" visorbus identifiers and I chose to make the numbers go down.

selltc commented 8 years ago

So I THOUGHT I was looking thru all of these, and after I totally finished, I realized I looked at the wrong set of patches! (I looked thru your first set from 6/3 instead of your set from today.) So you can ignore my comments in commit 3d45c4b381e0d0ea1b285a5c96bf2872b604f5d8 and commit e0d3f4f6d1d9a661bc2b092babbf368c3f15621e.

selltc commented 8 years ago

Your most-recent patchset looks great Bryan. I recommend just to make sure that your cover-letter clearly states your end-goal to replace clunky macros in guestlinuxdebug.h with a simple functional interface. It might be helpful to mention that there are intervening patches that change guestlinuxdebug.h prior to its ultimate removal, to facilitate simpler+smaller patches. That will clue-in any inspector not to focus too much on transient changes to guestlinuxdebug.h.

davidker commented 8 years ago

There is an issue with a049bb9f193948e0fd18ddd77535cd5a540413da compiling, look at the commit itself for more details. It gets fixed by 016b06252a93ab1cd3b4247b895a6f1ef831610d but needs to be fixed in the patch itself.

mriswyth commented 8 years ago

I'm really sorry about that.

New branch at githubissue#68-upstream-next-3.

New replacement commits are

davidker commented 8 years ago

@mriswyth, thanks!

I plan on cherry-picking the rest of those commits over keeping the other commits from v2.

David Kershner

davidker commented 7 years ago

Okay, this issue I think is the one that is still tracking postcodes correct?

If not, it should be closed because we are done all the initial include file cleanups.

davidker commented 7 years ago

Closing this issue, postcodes are gone. Hopefully I can get CONTROLVM RESPONSE values back.