Fix double response - Githubissues

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus

Other

2 stars 1 forks source link

Fix double response #101

Closed davidker closed 7 years ago

davidker commented 7 years ago

Looking at bus_epilog, I believe we are sending a double response back to COMMAND. This needs to be fixed before error handling can be done.

davidker commented 7 years ago

Preliminary patches are in branch githubissue-101-upstream-next. Yes, the comments are bad. @selltc anyway you can do a quick look to see if you agree with where I'm going with this?

davidker commented 7 years ago

The branch githubissue-fixerrors has the changes including the error responses. Only thing left is the mapping functions for errno to controlvm error number.

selltc commented 7 years ago

I won't look at your patch comments, but I'll remark about anything I notice about the code in each github patch. (I realize you only asked for a "quick look", but I figure I might as well write stuff down if I see it.)

davidker commented 7 years ago

Thanks, I have noticed a couple of spots that I need to squash with other patches. But I definitely don't want you to take too much of your time.

selltc commented 7 years ago

I'm thru commit f03b26c41e4a717dd9f567458e5ae21ae3ed7e9b (controlvm_respond add error handling). I'll pick up the rest tomorrow.

In general, it would be nice to get to the point were functions like bus_create() and my_device_create() were returning error statuses, and the caller would be able to send the controlvm response if required.

davidker commented 7 years ago

Thanks Tim,

I agree with you in general, but then it comes down to the parahotplug message that is asynchronous, I'm not sure how that was gets treated. Do I return a positive message to delay? I'm not sure what happens there and the caller can't just return the response at that time.

selltc commented 7 years ago

It feels like you have several types of return values:

error (with error code)
success (with optional success code)
pending

if the return value is NOT pending, AND the controlvm request indicates that a response is required
    send a controlvm response with the appropriate error or success code

I think we just need to encode that in the simplest manner possible.

davidker commented 7 years ago

For pending, we normally just turn off the response_expected bit, that is what pending_msg_hdr is for. I wonder since we aren't releasing anything we just stick the insmsg in there unless we want to delay it.

davidker commented 7 years ago

Tim, I'm putting my updates based on your comments on githubissue-101-upstream-next-v2.

Also, I have not signed-off on them since when I move them into upstream-next my scripts will auto sign off on them.

selltc commented 7 years ago

I just glanced at githubissue-101-upstream-next-v3, including the comments, and it looks good.

I assume you will include in your cover-letter that one of the ultimate purposes of the set is to get rid of those epilog functions, because that will make following the patches easier.
I noticed that Sameer's patch, commit 85eab29b99a16affcd754d26d2cc20dd8b04cc99, has a githubissue annotation in the log comment; I assume your script will be getting rid of that.

davidker commented 7 years ago

Closing this last commit was committed at 9e9eec6b81b551b5da3d852526467498f4613140.