> > I still can't get yout point, sorry. > > Maybe we are having some terminology mismatch. > > > > I'm *not* assuming that "only the low 2 bits a must be zero". > > I'm assuming that the low two bits must be same as they are in > > NET_IP_ALIGN constant. > > No you're not; see the expression above. Doing what you just described > would be ((pkt & ~3) | NET_IP_ALIGN), plus/minus 4 ...
Ohohoh ... Let's remember some simple math.
Question:
given integer M, integer A and integer B such that 0<=B<M, calculate
lowest integer X such that (A - X) mod M is B.
Solution:
Let's first write obvious formulas:
if (A mod M) >= B, then X = (A mod M) - B (1)
if (A mod M) < B, then X = (A mod M) - B + M (2)
However, because it's obvious that 0<=X<M, we may add 'mod M' to the
right
of both above equations:
(1): X = ((A mod M) - B) mod M
(2): X = ((A mod M) - B + M) mod M
Since for any z, ((z + M) mod M) is equal to (z mod M), the above two
formulas are equal.
Since for any y and z, (((z mod M) - y) mod M) is equal to ((z - y) mod
M), both of the formulas below are equal to ((A - B) mod M)
So the result is: X = ((A - B) mod M), for any A, B and M.
In our case, A is pkt, B is NET_IP_ALIGN, and M is 4.
So offset is (pkt - NET_IP_ALIGN) mod 4.
Or (pkt - NET_IP_ALIGN) & 3, which is the same, but without expensive
modulus operation.
Although the above proof may look long, actual the result is very obvious
for anyone who is more or less aware of modulus calculations. When I was
writing the patch, I just wrote that code line without stopping.
Anyway, since the patch we are talking about fixes real issue (kernel
crashes without it on hardware I'm working on), I believe our disagreement
needs to be resolved somehow.
Options:
(1) keep my original form, which is
addr = (unsigned long)pkt;
offset = NET_IP_ALIGN ? (addr - NET_IP_ALIGN) & 3 : 0;
if (offset) {
... /* move data */
}
(2) keep code in my original form, but add some math explanation in
comment:
addr = (unsigned long)pkt;
/* If NET_IP_ALIGN is non-zero, and (pkt & 3) is not equal to
* NET_IP_ALIGN, we need to move pkt backwards, such that
* this alignment requirement is met. We always have room of 4 bytes
* below pkt - this space was used inside urb for size field.
*
* If (addr & 3) > NET_IP_ALIGN, required offset is
* ((addr & 3) - NET_IP_ALIGN)
* if (addr & 3) < NET_IP_ALIGN), required offset is
* ((addr & 3) + 4 - NET_IP_ALIGN)
* Because of basic properties of operations, both these formulas are
* equal to
* (addr - NET_IP_ALIGN) & 3
*
* When this is zero, move is not required. When this is nonzero, it is
* the required move offset.
*/
offset = NET_IP_ALIGN ? (addr - NET_IP_ALIGN) & 3 : 0;
if (offset) {
... /* move data */
}
(3) or you prefer to ignore all the math and write everything in code:
if (!NET_IP_ALIGN)
return;
addr = (unsigned long)pkt;
if ((addr & 3) > NET_IP_ALIGN)
offset = (addr & 3) - NET_IP_ALIGN;
else if ((addr & 3) < NET_IP_ALIGN)
offset = (addr & 3) + 4 - NET_IP_ALIGN;
else
return;
... /* move data */
(4) or maybe you better like this version:
if (!NET_IP_ALIGN || ((addr & 3) == NET_IP_ALIGN))
return;
addr = (unsigned long)pkt;
new_addr = (addr & ~3) + NET_IP_ALIGN;
if (new_addr > addr)
new_addr -= 4;
offset = addr - new_addr;
... /* move data */
Which one of these options do you prefer?
> > >> And there is at least 4 bytes available before pkt (that was used
> > >> for frame size word in urb space).
> > >
> > > Then it's worth a comment highlighting that fact. In the context of
> > > that function, it's not clear.
> >
> > Sorry, but there *is* such a comment in the patch :).
> > Please re-read the patch :)
>
> In _that_ function, where the confusion could arise, I saw no such
> comment.
Oh yes, it's just above the function, 5 lines above the point of
question ... probably too far away ...
> Can you resend the latest version of your patch?
Will do as soon as we will agree in offset calculation code :)
Nikita
pgpq6fbUrUTNh.pgp
Description: PGP signature
------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys - and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
