Masayuki Murayama wrote:
> Garret,
>
> This is my suggestion on the new hme:
>
>
>> Masayuki Murayama wrote:
>>
>>>> I got my second reviewer, so no need for anyone
>>>>
>> else
>>
>>>> to review it if
>>>> they don't want to.
>>>>
>>>>
>>> It is very good for you!
>>>
>>>
>>>
>>>> Of course, review feedback is
>>>> always appreciated...
>>>> I can always follow up with a 2nd RTI if it is
>>>> warranted. (Plus, I'll
>>>> be in this code again shortly, to add support for
>>>>
>> QFE
>>
>>>> devices.)
>>>>
>>>>
>>> I understand you hasten the code to be reviewed.
>>> I'll focus and review the VLAN tagging, GLDv3 and
>>> hw checksum part of the code in one or two days,
>>>
>> and will
>>
>>> post the result here.
>>>
>>>
>> Okay, great!
>>
>>
>
> line 3984-3985, hme_m_tx
> Messages blocks should be deallocated without transmitting if
> hme chipset stops transmitting packets when the link is down,
> I think.
> Actually it'll depend on hme chipset behavior. I recommend
> to ensure it by its datasheet.
>
I'm pretty sure the code is correct as it stands. The MAC controller is
fairly separate from the PHY, and the chip will properly return packets
with a carrier failure if the link is down.
> line 4772-4274, hmestart
> Doesn't it need to tell the protocol for the transmitting
> packet, UDP or TCP, to hme hardware?
> Nics that have the partial hardware checksum function for tx,
> like intel pro/1000 and Marvell PCIe based chipset, for which
> I've written drivers, requires to specify UDP or TCP in tx
> descriptors, because they cannot detect it automatically.
> How about hme chipset? The checksum calculations for UDP and
> TCP, are not same.
> In the current solaris hw checksum frame work, nics must
> support both of UDP and TCP, otherwise we cannot use it,
> I think.
>
No. We are registered for "partial" checksum, which means that it is a
simple ones-complement calculation over the specified region. The stack
will have compensated by putting the appropriate value in the csum field
so that the hardware checksum is "corrected." All the hardware has to
do is put the ones-complement checksum from the indicated region at the
specified offset.
> line 6032-6039: hmeread
> In my experience, partially calculated hw checksum for rx
> packets, must be applied on only TCP or UDP packets, not
> on other packets like ICMP. Actually it depends on hme
> chipset specification. Please ensure it.
>
HME doesn't care. You could run it over IPX packets. Of course, the
result would be kinda nonsensical.
Anyway, the stack won't supply checksum fields if the packet isn't
appropriate for this kind of offload.
> And I wonder all generations of hme chipset can detect
> IPv6 packets? Rx hw checksum function should be aware of
> protocols for rx packets.
>
Again, you don't have to know this. All the hardware does is generate a
checksum over the specified ethernet payload. The stack will "adjust"
the computed results to account for differences in protocols, etc.
> line 6037: hmeread
> I think the start offset parameter which is passed
> hcksum_assoc() should be offset to UDP or TCP header from
> IP header, instead of 0. The offset to checksum field
> should be passed to hcksum_assoc(), instead of 0.
>
Nope. The offset is the offset from the IP header. In our receive
function, the hardware generates the checksum from that IP header, and
the stack makes the appropriate adjustments.
> To debug this code is very difficult. Solaris ip layer
> passes rx packets when software checksum is ok, even if
> the hardware checksum isn't ok.
>
Yes, but there is statistic reporting for that. Trust me, when I had
the incorrect algorithm, I noticed. :-) A number of kinds of
connectivity just started failing.
> line 5858-5869: hmeread_dma
> same with what I suggested on hmeread
>
And same replies. :-)
-- Garrett
> Best Regards,
> -masa
>
>
> This message posted from opensolaris.org
> _______________________________________________
> networking-discuss mailing list
> [email protected]
>
_______________________________________________
networking-discuss mailing list
[email protected]
