On Wed, Jun 1, 2016 at 2:22 PM, Levy, Amir (Jer) <amir.jer.levy@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Andreas Noever [mailto:andreas.noever@xxxxxxxxx] >> Sent: Wednesday, May 25, 2016 02:25 >> To: Levy, Amir (Jer) <amir.jer.levy@xxxxxxxxx> >> Cc: Lukas Wunner <lukas@xxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; >> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Jamet, Michael >> <michael.jamet@xxxxxxxxx>; Alloun, Dan <dan.alloun@xxxxxxxxx>; >> Westerberg, Mika <mika.westerberg@xxxxxxxxx>; Svahn, Kai >> <kai.svahn@xxxxxxxxx>; Shevchenko, Andriy >> <andriy.shevchenko@xxxxxxxxx>; Winkler, Tomas >> <tomas.winkler@xxxxxxxxx> >> Subject: Re: [PATCH 0/6] thunderbolt: Introducing Thunderbolt(TM) >> networking >> >> On Tue, May 24, 2016 at 3:58 PM, Levy, Amir (Jer) <amir.jer.levy@xxxxxxxxx> >> wrote: >> >> -----Original Message----- >> >> From: Lukas Wunner [mailto:lukas@xxxxxxxxx] >> >> Sent: Tuesday, May 24, 2016 13:55 >> >> To: Levy, Amir (Jer) <amir.jer.levy@xxxxxxxxx> >> >> Cc: andreas.noever@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; >> >> bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Jamet, Michael >> >> <michael.jamet@xxxxxxxxx>; Alloun, Dan <dan.alloun@xxxxxxxxx>; >> >> Westerberg, Mika <mika.westerberg@xxxxxxxxx>; Svahn, Kai >> >> <kai.svahn@xxxxxxxxx>; Shevchenko, Andriy >> >> <andriy.shevchenko@xxxxxxxxx>; Winkler, Tomas >> >> <tomas.winkler@xxxxxxxxx> >> >> Subject: Re: [PATCH 0/6] thunderbolt: Introducing Thunderbolt(TM) >> >> networking >> >> >> >> Hi Amir, >> >> >> >> On Mon, May 23, 2016 at 11:48:50AM +0300, Amir Levy wrote: >> >> > Thunderbolt(TM) networking, introduced in these patches, provides >> >> > the capability of connecting hosts together by emulating an Ethernet >> adapter. >> >> >> >> Thank you for this contribution. I think it may take some time to >> >> review everything, for now just some general observations: >> >> >> >> - Up until now this driver was Mac-only. IIUC the functionality you're >> >> adding will also be used on non-Macs. That's an important change that >> >> should probably be made more explicit in the commit messages. Did you >> >> test your patches on a Mac to see that nothing regresses? >> > >> > Yes, we did some tests on MacBook Pro with Falcon Ridge. >> > Note that (as written in the cover letter), the functionality that we are >> adding is based on ICM (Intel Connection Manager, firmware). >> > These patches will keep the current functionality on Mac and will add the >> networking functionality on non-Macs. >> >> Hi Amir, >> >> First of all thanks for contributing this. It is certainly good to see Intel help out >> with the driver. > > Hi Andreas, > Thank you for the comments. > >> >> I only did a cursory read, but already have a few questions concerning the >> ICM and its relation to the rest of the driver: >> - Is the ICM only responsible for networking or does it also handle pci >> tunnels, DP, ...? > > ICM is responsible for PCI tunnels and DP as well. > >> - Your code only supports TB2,3. Afaik Apple supports IP over TB also on TB 1. >> What are the obstacles to support first generation chips? > > Intel decided to support Thunderbolt Networking from generation 2. > >> - Related to the above: if some of the registers are available only for some >> chips then please document this in nhi_regs.h (and other places). >> - Is it possible to use the ICM on Apple hardware? > > We don't know if Apple supports it or plans to support it. > >> >> Your patch essentially splits the driver into two. You add an if statement to >> nhi_probe which either runs your code or the old one. >> This is just silly (you might as well add a separate driver). At least parts of the >> functionality are already implemented. For example patch >> 2 has large overlap with what is currently handled by nhi.c/ctl.c >> (reading/writing the ring buffer and handling communication with the PDF). I >> understand that the interface provided by ring_rx/ring_tx is not a good >> match for high speed networking introduced in the later patches. But then >> you should refactor it to provide what you need instead of reimplementing >> all the functionality. > > We had a lot of discussions around the driver separation topic and we also consulted some Linux contributors in Intel and got to the conclusion that one driver will be better. > If you recommend to do it in separate driver, I'll send new patches. > Do you have preference how the separation should be? > I thought about defining icm_nhi.c as a module_init and in contrast to the original driver, if dmi_match(Apple) return error. Well I would prefer it to be in a single driver, but without duplicated functionality. There are two parts to your driver: 1) The part that talks to the ICM to setup thunderbolt paths between two controllers. 2) The network driver. I have no experience with networking and we have analoge to 2) in the current driver. So this is fine from my perspective. As far as I can tell the network driver just needs to be given a thunderbolt path (which rx/tx ring to use on the nhi) and is otherwise self contained. This is interesting to me since I would like to try to implement networking on Apple hardware. 1) communicates with the ICM by writing SW_TO_FW/FW_TO_SW packages to the control channel (or "raw mode", "PDF", ... not sure what the correct terminology is). The code to drive the control channel is already present in ctl.c (we just use raw config_read/write packages). Why can't you reuse all the plumbing code from there (pushing packages onto the ring, handling interrupts etc..). I think it should be possible to add support for the two new package types to ctl.c and then work with them from say thunderbolt_alloc_and_start (instead of scanning the root switch just talk to the icm...). Btw, what is the sequence of ICM commands needed to establish get a network path setup? Is that supposed to be done from userspace? >> >> Lastly, why do you need a user mode component? What does it do? > > The general concept was to keep as minimum as possible in kernel and move some of the logic to user space. > This will give us the option to have minimal changes in kernel module in the future, and to release features in a timely manner. > Also it is a preparation to some of the features that will require user interaction. > >> >> Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
