On 2016-06-08 Andreas Noever wrote: > 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...). Intel handles Thunderbolt developments on non-MAC systems only (with ICM). Intel doesn’t maintain, develop and publish Thunderbolt SW code running on Apple HW for OS X and Linux. This is the reason there is minimal code in the probe function, that branches to 2 separate modules. I can remove this from probe and produce two separate drivers, as I suggested. > > Btw, what is the sequence of ICM commands needed to establish get a > network path setup? Is that supposed to be done from userspace? The network path establishment is divided between user-space (Daemon) and driver. It starts in user-space and ends in driver. The hand over from Daemon to driver is in nhi_genl_approve_networking. > > >> > >> 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 ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
