[Bug 92481] [Patch] Mostly cosmetic changes for drm_dp_mst_i2c_xfer in Linux 4.3-rc5

[bugzilla-daemon at freedesktop.org]

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 at lists.freedesktop.org
          Reporter: adam_richter2004 at 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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20151015/6c6787a2/attachment.html>