> -----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. > > 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�����٥
