Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move ipts_fw_config.bin into the companion driver #11
Move ipts_fw_config.bin into the companion driver #11
Changes from all commits
a1531a7
e1cfd84
636e23d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason you picked mutex over spin here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because qzed told me so when writing the original implementation :P (in
ipts-fw.c
which was deleted).I am not that familiar with synchronisation inside of the kernel, so I just kept it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning was mostly: Mutex is the standard tool and it's initialization code, so we don't need the performance a spin-lock would give us here. Also the lock is being held during IO operations (
request_firmware
), so not really small critical sections.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this function be in the header as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both,
ipts_add_companion
andipts_remove_companion
are exported in the public header file (include/linux/ipts-companion.h
) for other modules to use. The other functions are internal use by IPTS itself, so they are exported in the local header file insidedriver/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole function can be written better without any gotos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my answer above: Not using gotos would yield duplicated
mutex_unlock
calls all over the place, and using goto in such a case is what the kernel coding guidelines advise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not necessarily for throwing out the gotos, but I think we can reduce the code beneath the mutex. I think actually only the code until (including)
*cfg = ipts_alloc_fw_list(ipts_companion->firmware_config);
needs to be synchronized.ipts_request_firmware
does its own checks and synchronization, and the rest is stuff on data that we own at that point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also with that you'll probably get rid of one goto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into how to reduce the amount of gotos in that function, but I found no nice way that allows for all the fallback behaviour that is there, and that doesn't require duplicating the call to
mutex_unlock
.If you want me to change it, I will look into it again ofc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was something like this:
You only need the mutex to guard the direct
ipts_companion
access, the rest doesn't need to be in the critical section (especially asipts_request_firmware
does all that itself). I'd still keep the goto for the fallback as I don't see a cleaner option.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yeah, that is definitly nicer. Updated the function, thank you.