https://bugs.freedesktop.org/show_bug.cgi?id=92481
Bug ID: 92481 Summary: [Patch] Mostly cosmetic changes for drm_dp_mst_i2c_xfer in Linux 4.3-rc5 Product: DRI Version: unspecified Hardware: Other OS: Linux (All) Status: NEW Severity: normal Priority: medium Component: DRM/other Assignee: dri-devel@lists.freedesktop.org Reporter: adam_richter2004@yahoo.com
Created attachment 118905 --> https://bugs.freedesktop.org/attachment.cgi?id=118905&action=edit Numerous mostly cosmetic fixed to drm_dp_mst_i2c_xfer in linux 4.3-rc5/drivers/gpu/drm/drm_dp_mst_topology.c AFTER Dave Airlie's other changes have been applied
Thanks to Dave Airlie and Daniel Vitter for the the fixes to drm_dp_mst_i2c_xfer to avoid possibly sending unitialized data in DisplayPort multistream tranport i2c queries and to enforce the limit on the number of i2c transactions in a single i2c request to avoid a possible buffer overflow.
The attached patch is based on the file with Dave's aforementioned changes applied. It is an a bunch of mostly cosmetic ("maintainability") changes, which I will list below:
a. Move the parameter validations to before the call to drm_dp_get_validated_mstb_ref(), so that, if they fail, they do not need to use "goto out", thereby reducing the number of goto's and the longest distance between a goto and its target label. I imagine it also makes these rare failure cases a few nanoseconds faster without delaying the common case.
b. Because of the above, txmsg no longer needs to be initialized to NULL.
c. Have different error messages and error codes (E2BIG, and EINVAL) for num > 4 and the i2c transaction not ending with a read statement, for clearer debugging if either of these errors should occur, which should help in cases where the errors only occur sporadically or the person observing the error cannot easily recompile and install a new kernel to get more information. Error reproduction is precious, so it's best not to waste them with unnecessary ambiguity. These errors previous returned EIO, but EIO connotes a complaint from the hardware, hence the change to E2BIG and EINVAL. By the way, I am assuming that these conditions really can be caused by user level code accessing /dev/i2c... If not, then I would be happy to replace them with BUG_ON() statements.
d. Delete the comment "see if last msg is a read", since (c) makes it redundant due to the clearer diagnostic message "Final DP-MST I2C transaction was not a read".
e. Since we're concerned about invalid i2c message parameters resulting in invalid memory references, also guard against num <= 0. Return -EDOM in this case, since that really would cause a problem with the mathematical domain of a function, because the line "...num_transactions = num - 1" would result in -1 being cast into 255 for the 8 bit field num_transactions.
f. Eliminate the variable "reading", which was computed and used only once, immediately after it was computed.
g. Be more friendly to the optimizer by using unlikely() (and likely() instead of unlikely() in one place to reduce the number of parentheses).
h. Be more friendly to the optimizer and maybe make the code more readable by consolidating the seven computions of "num - 1" into a new variable, "count". I know that having two varaibles named "num" and "count" where count == num - 1 is not the greatest naming convention. Please feel free to rename. There actually is one place toward the end of the function where "num" is used (rather than num - 1).
h. Do the check for num - 1 > DP_REMOTE_I2C_READ_MAX_TRANSACTIONS in a manner not susceptible to integer overflow.
I realize that this patch conflates all these different minor changes. I would be willing to submit these changes individually or in a few smaller groups if necessary.
By the way, the attached patch assumes is against Linux 4.3-rc5 after Dave Airlie's patch from http://lists.freedesktop.org/archives/dri-devel/2015-October/092465.html is applied.
Anyhow, I hope this patch is helpful.
Adam