Hello,
I apologize for the delay in my reply.
Thanks for the feedback. My comments/explanations are inline
On Sun, Aug 07, 2022 at 02:15:34PM +0200, Martin Husemann wrote:
> - it only happened to a single affected driver (probably as testing all the
> others was impossible)
That's correct. I wasn't in a position to test hardware other than dwc_gmac,
so I erred on the side of caution and changed only dwc_gmac.c (with a reasoably
large comment block explaining what we were doing and why).
> - none of the drivers pointed at as examples for stripping the bytes off
> "manually" has any special code for ATALK or AARP (like this change
> introduced for dwc_gmac.c)
Ah, that's a result of me wanting to reduce the commit message size to
something that could be easily digested. I reduced it too far.
What I meant by that was this:
I'm not really comfortable with a machine-independent driver having higher-level
protocol knowledge, and modifying packets based on what protocol was received.
To my mind, that should be handled further up the stack (in net/if_ethersubr.c,
ideally). But, again, going with the principles of least-change/least-surprise,
dwc_gmac.c seemed the least unpleasant place to put it.
Existing examples of mucking about with the packet data were:
* in ixp425_if_npe.c (in npe_rxdone(), line 1050, which indicated that it was
okay to modify the packets in-driver {and is done unilaterally, which further
indicates that this driver also doesn't work with AppleTalk}),
* in dev/ic/gem.c, gem_rint(), around line 2794, where the gem driver mucks
about with the checksum based on ETHERTYPE_*, which indicates that drivers
are allowed to modify packets based on packet type.
I wasn't saying that other drivers were special-handling AppleTalk in
particular, but that other drivers were modifying packet contents based on
the protocol of the packet concerned.
Sorry about having caused confusion with the commit comment. I'll be more
verbose in the future.
> So I wonder if we should redefine the meaning of M_HASFCS from "FCS included
> at the end of frame" to "Frame may have FCS included at the end if protocol
> uses a FCS", and then have if_ethersubr.c deal with it generically.
I was thinking that was the right thing to do strategically, but it'd be a large
change with the near-certainty of breaking things that I am not in a position to
test, so I went for the simpler solution.
Cheers, and thanks very much for the feedback,
--
-- Chris
GPG key fingerprint B566 051D D99D 37B6 41F2 7593 22EE 9E38 FBA7 7B34
Attachment:
signature.asc
Description: PGP signature