Message-ID: <alpine.LFD.2.21.1810090133440.15789@eddie.linux-mips.org>
Date:   Tue, 9 Oct 2018 23:57:36 +0100 (BST)
From:   "Maciej W. Rozycki" <macro@...ux-mips.org>
To:     netdev@...r.kernel.org
Subject: [PATCH net-next v2 0/2] FDDI: DEC FDDIcontroller 700 TURBOchannel
 adapter support

Hi,

 This is an update to <http://patchwork.ozlabs.org/patch/342737/>.  I 
believe I have addressed all the requests made in the previous review 
round.

 There is still one `checkpatch.pl' warning remaining:

WARNING: quoted string split across lines
#1652: FILE: drivers/net/fddi/defza.c:1442:
+       pr_info("%s: ROM rev. %.4s, firmware rev. %.4s, RMC rev. %.4s, "
+               "SMT ver. %u\n", fp->name, rom_rev, fw_rev, rmc_rev, smt_ver);

total: 0 errors, 1 warnings, 2458 lines checked

however I think the value of staying within 80 columns is higher than the 
value of having the string on a single line.  This is because with all the 
formatting specifiers there it is not directly greppable based on the 
final output produced to the kernel log on one hand, e.g.:

tc2: ROM rev. 1.0, firmware rev. 1.2, RMC rev. A, SMT ver. 1

while it can be easily tracked down by grepping for an obvious substring 
such as "RMC rev" on the other.

 The issue with MMIO barriers I discussed in the course of the original 
review turned out mostly irrelevant to this driver, because as I have 
learnt in a recent Alpha/Linux discussion starting here: 
<https://marc.info/?i=alpine.LRH.2.02.1808161556450.13597%20()%20file01%20!%20intranet%20!%20prod%20!%20int%20!%20rdu2%20!%20redhat%20!%20com> 
our MMIO API mandates the `readX' and `writeX' accessors to be strongly 
ordered with respect to each other, even if that is not implicitly 
enforced by hardware.

 Consequently I have removed all the explicit ordering barriers and 
instead submitted a fix for MIPS MMIO implementation, which currently does 
not guarantee strong ordering (the MIPS architecture does not define bus
ordering rules except in terms of SYNC barriers), as recorded here: 
<https://patchwork.linux-mips.org/project/linux-mips/list/?series=1538>.  

 Enforcing strong MMIO ordering can be costly however and is often 
unnecessary, e.g. when using PIO to access network frame data in onboard 
packet memory.  I have therefore retained the information that would be 
lost by the removal of barriers, by defining accessor wrappers suffixed by 
`_o' and `_u', for accesses that have to be ordered and can be unordered 
respectively.

 If we ever have an API defined for weakly-ordered MMIO accesses, then 
these wrappers can be redefined accordingly.  Right now they all expand to 
the respective `_relaxed' accessors, because, again, enforcing the 
ordering WRT DMA transfers can be costly and we don't need it here except 
in one place, where I chose to use explicit `dma_rmb' instead.

 Similarly I have replaced the completion barriers with a read back from 
the respective MMIO location (all adapter MMIO registers can be read with 
no side effects incurred), which will serve its purpose on the basis of 
MMIO being strongly ordered (although a read from TURBOchannel is going to 
be slower than `iob', making the delay incurred unnecessarily longer).

 And last but not least, I have split off the SMT Tx network tap support 
to a separate change, 2/2 in this series, so that it does not block the 
driver proper and can be discussed separately.

 I think it has value in that it makes the view of the outgoing network 
traffic complete, as if one actually physically tapped into the outgoing 
line of the ring, between the station being examined and its downstream 
neighbour.  Without this part only traffic passed from applications 
through the whole protocol stack can be captured and this is only a part 
of the view.

 With the `dev_queue_xmit_nit' interface now exported it's only 
`ptype_all' that remains private, and to define a properly abstracted API 
I propose to provide am exported `dev_nit_active' predicate that tells 
whether any taps are active.  This predicate is then used accordingly.

 NB if there is a long-term maintenance concern about the `dev_nit_active' 
predicate, then well, corresponding inline code currently present in 
`xmit_one' has to be maintained anyway, and if the resulting changes 
require `defza' to be updated accordingly, then I am going to handle it; 
after some 20 years with Linux it's not that I am going to disappear 
anywhere anytime.  And once I am dead, which is inevitably going to happen 
sooner or later, then the driver can simply be ripped from the kernel.  
Though I suspect that at that point no DECstation Linux users may survive 
anymore, even though hardware, being as sturdy as it is, likely will.

 I have a patch for `tcpdump' to actually decode SMT frames, which I plan 
to upstream sometime.  Here's a sample of SMT traffic captured through the 
`defza' driver in a small network of 4 stations and no concentrators, 
printed in the most verbose mode:

01:16:59.138381 4f 00:60:b0:58:41:e7 00:60:b0:58:41:e7 73: SMT NIF ann vid:1 tid:00000270 sid:00-00-00-60-b0-58-41-e7 len:40: UNA: 00 00 00 06 0d 1a 02 ae StationDescr: 00 01 02 00 StationState: 00 00 30 00 MACFrameStatusFunctions.3: 00 00 00 01
01:17:00.332750 4f 08:00:2b:a3:a3:29 08:00:2b:a3:a3:29 73: SMT NIF ann vid:1 tid:0000013b sid:00-00-08-00-2b-a3-a3-29 len:40: UNA: 00 00 00 06 0d 1a 82 e7 StationDescr: 00 01 02 00 StationState: 00 00 30 00 MACFrameStatusFunctions.3: 00 00 00 01
01:17:00.354479 4f 00:60:b0:58:40:75 00:60:b0:58:40:75 73: SMT NIF ann vid:1 tid:0000029c sid:00-00-00-60-b0-58-40-75 len:40: UNA: 00 00 10 00 d4 74 b6 ae StationDescr: 00 01 02 00 StationState: 00 00 31 00 MACFrameStatusFunctions.3: 00 00 00 01
01:17:00.442175 4f 00:60:b0:58:41:e7 Broadcast 73: SMT NIF req vid:1 tid:00000271 sid:00-00-00-60-b0-58-41-e7 len:40: UNA: 00 00 00 06 0d 1a 02 ae StationDescr: 00 01 02 00 StationState: 00 00 30 00 MACFrameStatusFunctions.3: 00 00 00 01
01:17:00.448657 41 08:00:2b:a3:a3:29 00:60:b0:58:41:e7 73: SMT NIF rsp vid:1 tid:00000271 sid:00-00-08-00-2b-a3-a3-29 len:40: UNA: 00 00 00 06 0d 1a 82 e7 StationDescr: 00 01 02 00 StationState: 00 00 30 00 MACFrameStatusFunctions.3: 00 00 00 01
01:17:01.015152 4f 08:00:2b:a3:a3:29 Broadcast 73: SMT NIF req vid:1 tid:0000013c sid:00-00-08-00-2b-a3-a3-29 len:40: UNA: 00 00 00 06 0d 1a 82 e7 StationDescr: 00 01 02 00 StationState: 00 00 30 00 MACFrameStatusFunctions.3: 00 00 00 01
01:17:01.111644 41 08:00:2b:2e:6d:75 08:00:2b:a3:a3:29 73: SMT NIF rsp vid:1 tid:0000013c sid:00-00-08-00-2b-2e-6d-75 len:40: UNA: 00 00 10 00 d4 c5 c5 94 StationDescr: 00 01 01 00 StationState: 00 00 11 00 MACFrameStatusFunctions.2: 00 00 00 01
01:17:04.814603 4f 08:00:2b:2e:6d:75 Broadcast 73: SMT NIF req vid:1 tid:0000013c sid:00-00-08-00-2b-2e-6d-75 len:40: UNA: 00 00 10 00 d4 c5 c5 94 StationDescr: 00 01 01 00 StationState: 00 00 11 00 MACFrameStatusFunctions.2: 00 00 00 01
01:17:04.814939 4f 08:00:2b:2e:6d:75 Broadcast 73: SMT NIF req vid:1 tid:0000013c sid:00-00-08-00-2b-2e-6d-75 len:40: UNA: 00 00 10 00 d4 c5 c5 94 StationDescr: 00 01 01 00 StationState: 00 00 11 00 MACFrameStatusFunctions.2: 00 00 00 01
01:17:04.820960 4f 08:00:2b:2e:6d:75 08:00:2b:2e:6d:75 73: SMT NIF ann vid:1 tid:0000013b sid:00-00-08-00-2b-2e-6d-75 len:40: UNA: 00 00 10 00 d4 c5 c5 94 StationDescr: 00 01 01 00 StationState: 00 00 11 00 MACFrameStatusFunctions.2: 00 00 00 01

 Questions, comments?  Otherwise, please apply.

  Maciej

Powered by blists - more mailing lists