Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1443373 > unrolled thread
| Started by | Amir Levy <amir.jer.levy@intel.com> |
|---|---|
| First post | 2016-07-14 13:30 +0200 |
| Last post | 2016-07-17 09:10 +0200 |
| Articles | 13 — 6 participants |
Back to article view | Back to linux.kernel
[PATCH v3 0/8] thunderbolt: Introducing Thunderbolt(TM) networking Amir Levy <amir.jer.levy@intel.com> - 2016-07-14 13:30 +0200
[PATCH v3 2/8] thunderbolt: Updating device IDs Amir Levy <amir.jer.levy@intel.com> - 2016-07-14 13:40 +0200
Re: [PATCH v3 2/8] thunderbolt: Updating device IDs David Miller <davem@davemloft.net> - 2016-07-15 20:50 +0200
RE: [PATCH v3 2/8] thunderbolt: Updating device IDs "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-15 21:00 +0200
Re: [PATCH v3 2/8] thunderbolt: Updating device IDs David Miller <davem@davemloft.net> - 2016-07-16 00:00 +0200
RE: [PATCH v3 2/8] thunderbolt: Updating device IDs "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-18 12:00 +0200
RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-14 17:00 +0200
RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) "Winkler, Tomas" <tomas.winkler@intel.com> - 2016-07-16 10:50 +0200
RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) "Rosen, Rami" <rami.rosen@intel.com> - 2016-07-14 17:10 +0200
RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-14 20:40 +0200
RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) "Rosen, Rami" <rami.rosen@intel.com> - 2016-07-14 21:10 +0200
Re: [PATCH v3 6/8] thunderbolt: Networking state machine Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-07-15 02:30 +0200
RE: [PATCH v3 6/8] thunderbolt: Networking state machine "Levy, Amir (Jer)" <amir.jer.levy@intel.com> - 2016-07-17 09:10 +0200
| From | Amir Levy <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-14 13:30 +0200 |
| Subject | [PATCH v3 0/8] thunderbolt: Introducing Thunderbolt(TM) networking |
| Message-ID | <rUN05-BW-3@gated-at.bofh.it> |
This is version 3 of Thunderbolt(TM) driver for non-Apple hardware. Changes since v2: - Moved ICM code to subdirectory - thunderbolt/icm These patches were pushed to GitHub where they can be reviewed more comfortably with green/red highlighting: https://github.com/01org/thunderbolt-software-kernel-tree Daemon code: https://github.com/01org/thunderbolt-software-daemon For reference, here's a link to version 2: [v2]: https://lkml.org/lkml/2016/6/29/160 Amir Levy (8): thunderbolt: Macro rename thunderbolt: Updating device IDs thunderbolt: Updating the register definitions thunderbolt: Kconfig for Thunderbolt(TM) networking thunderbolt: Communication with the ICM (firmware) thunderbolt: Networking state machine thunderbolt: Networking transmit and receive thunderbolt: Networking doc Documentation/00-INDEX | 2 + Documentation/thunderbolt-networking.txt | 135 ++ drivers/thunderbolt/Kconfig | 25 +- drivers/thunderbolt/Makefile | 3 +- drivers/thunderbolt/icm/Makefile | 28 + drivers/thunderbolt/icm/icm_nhi.c | 1631 +++++++++++++++++++++ drivers/thunderbolt/icm/icm_nhi.h | 84 ++ drivers/thunderbolt/icm/net.c | 2277 ++++++++++++++++++++++++++++++ drivers/thunderbolt/icm/net.h | 274 ++++ drivers/thunderbolt/nhi_regs.h | 115 +- include/linux/pci_ids.h | 44 +- 11 files changed, 4592 insertions(+), 26 deletions(-) create mode 100644 Documentation/thunderbolt-networking.txt create mode 100644 drivers/thunderbolt/icm/Makefile create mode 100644 drivers/thunderbolt/icm/icm_nhi.c create mode 100644 drivers/thunderbolt/icm/icm_nhi.h create mode 100644 drivers/thunderbolt/icm/net.c create mode 100644 drivers/thunderbolt/icm/net.h -- 2.7.4
[toc] | [next] | [standalone]
| From | Amir Levy <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-14 13:40 +0200 |
| Subject | [PATCH v3 2/8] thunderbolt: Updating device IDs |
| Message-ID | <rUN9L-G3-1@gated-at.bofh.it> |
| In reply to | #1443373 |
Adding the new Thunderbolt(TM) device IDs to the list. Signed-off-by: Amir Levy <amir.jer.levy@intel.com> --- include/linux/pci_ids.h | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index c58752f..2d4cc50 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -2604,24 +2604,32 @@ #define PCI_DEVICE_ID_INTEL_82441 0x1237 #define PCI_DEVICE_ID_INTEL_82380FB 0x124b #define PCI_DEVICE_ID_INTEL_82439 0x1250 -#define PCI_DEVICE_ID_INTEL_LIGHT_RIDGE 0x1513 /* Tbt 1 Gen 1 */ -#define PCI_DEVICE_ID_INTEL_EAGLE_RIDGE 0x151a -#define PCI_DEVICE_ID_INTEL_LIGHT_PEAK 0x151b -#define PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C 0x1547 /* Tbt 1 Gen 2 */ -#define PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C 0x1548 -#define PCI_DEVICE_ID_INTEL_PORT_RIDGE 0x1549 -#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI 0x1566 /* Tbt 1 Gen 3 */ -#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE 0x1567 -#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI 0x1568 -#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE 0x1569 -#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI 0x156a /* Thunderbolt 2 */ -#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE 0x156b -#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI 0x156c -#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE 0x156d -#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI 0x1575 /* Thunderbolt 3 */ -#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE 0x1576 -#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI 0x1577 -#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE 0x1578 +#define PCI_DEVICE_ID_INTEL_LIGHT_RIDGE 0x1513 /* Tbt 1 Gen 1 */ +#define PCI_DEVICE_ID_INTEL_EAGLE_RIDGE 0x151a +#define PCI_DEVICE_ID_INTEL_LIGHT_PEAK 0x151b +#define PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C 0x1547 /* Tbt 1 Gen 2 */ +#define PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_2C 0x1548 +#define PCI_DEVICE_ID_INTEL_PORT_RIDGE 0x1549 +#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_NHI 0x1566 /* Tbt 1 Gen 3 */ +#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_2C_BRIDGE 0x1567 +#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_NHI 0x1568 +#define PCI_DEVICE_ID_INTEL_REDWOOD_RIDGE_4C_BRIDGE 0x1569 +#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI 0x156a /* Thunderbolt 2 */ +#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE 0x156b +#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI 0x156c +#define PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE 0x156d +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI 0x1575 /* Thunderbolt 3 */ +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE 0x1576 +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI 0x1577 +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE 0x1578 +#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_NHI 0x157d /* Tbt 2 Low Pwr */ +#define PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE 0x157e +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI 0x15bf /* Tbt 3 Low Pwr */ +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE 0x15c0 +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI 0x15d2 /* Thunderbolt 3 */ +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE 0x15d3 +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI 0x15d9 +#define PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE 0x15da #define PCI_DEVICE_ID_INTEL_80960_RP 0x1960 #define PCI_DEVICE_ID_INTEL_82840_HB 0x1a21 #define PCI_DEVICE_ID_INTEL_82845_HB 0x1a30 -- 2.7.4
[toc] | [prev] | [next] | [standalone]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2016-07-15 20:50 +0200 |
| Subject | Re: [PATCH v3 2/8] thunderbolt: Updating device IDs |
| Message-ID | <rVgls-1VF-13@gated-at.bofh.it> |
| In reply to | #1443374 |
From: Amir Levy <amir.jer.levy@intel.com> Date: Thu, 14 Jul 2016 14:28:16 +0300 > Adding the new Thunderbolt(TM) device IDs to the list. > > Signed-off-by: Amir Levy <amir.jer.levy@intel.com> Unless these PCI-IDs, all of them, are going to be used in multiple spots in the kernel, it is not appropriate to add them here. They belong as private macros in the drivers themselves, instead.
[toc] | [prev] | [next] | [standalone]
| From | "Levy, Amir (Jer)" <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-15 21:00 +0200 |
| Subject | RE: [PATCH v3 2/8] thunderbolt: Updating device IDs |
| Message-ID | <rVgv7-202-3@gated-at.bofh.it> |
| In reply to | #1444514 |
On Fri, Jul 15 2016, 09:49 PM, David Miller wrote: > From: Amir Levy <amir.jer.levy@intel.com> > Date: Thu, 14 Jul 2016 14:28:16 +0300 > > > Adding the new Thunderbolt(TM) device IDs to the list. > > > > Signed-off-by: Amir Levy <amir.jer.levy@intel.com> > > Unless these PCI-IDs, all of them, are going to be used in multiple spots in the > kernel, it is not appropriate to add them here. > > They belong as private macros in the drivers themselves, instead. They might be used: http://www.spinics.net/lists/linux-pci/msg51331.html
[toc] | [prev] | [next] | [standalone]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2016-07-16 00:00 +0200 |
| Subject | Re: [PATCH v3 2/8] thunderbolt: Updating device IDs |
| Message-ID | <rVjjj-3Ig-11@gated-at.bofh.it> |
| In reply to | #1444516 |
From: "Levy, Amir (Jer)" <amir.jer.levy@intel.com> Date: Fri, 15 Jul 2016 18:56:39 +0000 > On Fri, Jul 15 2016, 09:49 PM, David Miller wrote: >> From: Amir Levy <amir.jer.levy@intel.com> >> Date: Thu, 14 Jul 2016 14:28:16 +0300 >> >> > Adding the new Thunderbolt(TM) device IDs to the list. >> > >> > Signed-off-by: Amir Levy <amir.jer.levy@intel.com> >> >> Unless these PCI-IDs, all of them, are going to be used in multiple spots in the >> kernel, it is not appropriate to add them here. >> >> They belong as private macros in the drivers themselves, instead. > > They might be used: http://www.spinics.net/lists/linux-pci/msg51331.html Then move them to the common location when this "might" thing actually happens.
[toc] | [prev] | [next] | [standalone]
| From | "Levy, Amir (Jer)" <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-18 12:00 +0200 |
| Subject | RE: [PATCH v3 2/8] thunderbolt: Updating device IDs |
| Message-ID | <rWdvc-4xq-35@gated-at.bofh.it> |
| In reply to | #1444633 |
On Sat, Jul 16 2016, 12:53 AM, David Miller wrote: > From: "Levy, Amir (Jer)" <amir.jer.levy@intel.com> > Date: Fri, 15 Jul 2016 18:56:39 +0000 > > > On Fri, Jul 15 2016, 09:49 PM, David Miller wrote: > >> From: Amir Levy <amir.jer.levy@intel.com> > >> Date: Thu, 14 Jul 2016 14:28:16 +0300 > >> > >> > Adding the new Thunderbolt(TM) device IDs to the list. > >> > > >> > Signed-off-by: Amir Levy <amir.jer.levy@intel.com> > >> > >> Unless these PCI-IDs, all of them, are going to be used in multiple > >> spots in the kernel, it is not appropriate to add them here. > >> > >> They belong as private macros in the drivers themselves, instead. > > > > They might be used: > > http://www.spinics.net/lists/linux-pci/msg51331.html > > Then move them to the common location when this "might" thing actually > happens. There are several contradicting comments about this change (http://www.spinics.net/lists/linux-pci/msg51331.html and http://www.spinics.net/lists/linux-pci/msg51302.html) I've decided for now to make the new device ids private to the driver. This can be changed later when updating quirks for Apple hardware. Regards, Amir
[toc] | [prev] | [next] | [standalone]
| From | "Levy, Amir (Jer)" <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-14 17:00 +0200 |
| Subject | RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) |
| Message-ID | <rUQhj-2yA-7@gated-at.bofh.it> |
| In reply to | #1443373 |
Hi Tomas,
Thanks for your comments.
On Thu, Jul 14 2016, 03:44 PM, Winkler, Tomas wrote:
> > +/* NHI genetlink commands */
> > +enum {
> > + NHI_CMD_UNSPEC,
> > + NHI_CMD_SUBSCRIBE,
> > + NHI_CMD_UNSUBSCRIBE,
> > + NHI_CMD_QUERY_INFORMATION,
> > + NHI_CMD_MSG_TO_ICM,
> > + NHI_CMD_MSG_FROM_ICM,
> > + NHI_CMD_MAILBOX,
> > + NHI_CMD_APPROVE_TBT_NETWORKING,
> > + NHI_CMD_ICM_IN_SAFE_MODE,
> > + __NHI_CMD_MAX,
> > +};
> > +#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
> NHI_CMD_MAX = NHI_CMD_ICM_IN_SAFE_MODE ?
>
This template is used a lot with (generic) netlink.
Few examples:
http://lxr.free-electrons.com/source/drivers/acpi/event.c#L62
http://lxr.free-electrons.com/source/include/uapi/linux/irda.h#L224
It is easier to maintain - adding entry in the list will automatically update MAX.
[...]
> > + u32 status;
> > +
> > + status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
> > +
> > + if (status & REG_FW_STS_NVM_AUTH_DONE)
> > + break;
> > + msleep(30);
>
> 30 is big number, for polling, what is behind this?
>
The NVM authentication can take time for ICM.
This number comes from experiments.
[...]
> > +static struct tbt_nhi_ctxt *nhi_search_ctxt(u32 id)
> > +{
> > + struct tbt_nhi_ctxt *nhi_ctxt;
> > +
> > + list_for_each_entry(nhi_ctxt, &controllers_list, node)
> > + if (nhi_ctxt->id == id)
> > + return nhi_ctxt;
>
> Don't you need to lock this list with the controllers_list_rwsem ?
>
This is a helper function for searching the list.
The callers take and release the lock.
[...]
> > + bool nvm_auth_on_boot : 1;
> Don't use bool with bit fields use u32
What is the concern here?
If it is size of bool, it doesn't matter for this struct.
It is more readable that the expected value is bool,
and it is used a lot in kernel, for example:
http://lxr.free-electrons.com/source/include/linux/pm.h#L558
[toc] | [prev] | [next] | [standalone]
| From | "Winkler, Tomas" <tomas.winkler@intel.com> |
|---|---|
| Date | 2016-07-16 10:50 +0200 |
| Subject | RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) |
| Message-ID | <rVtsl-1wZ-5@gated-at.bofh.it> |
| In reply to | #1443496 |
> -----Original Message-----
> From: Levy, Amir (Jer)
> Sent: Thursday, July 14, 2016 17:50
> To: Winkler, Tomas <tomas.winkler@intel.com>;
> andreas.noever@gmail.com; gregkh@linuxfoundation.org;
> bhelgaas@google.com
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; thunderbolt-linux <thunderbolt-linux@intel.com>;
> Westerberg, Mika <mika.westerberg@intel.com>
> Subject: RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM
> (firmware)
>
> Hi Tomas,
> Thanks for your comments.
>
> On Thu, Jul 14 2016, 03:44 PM, Winkler, Tomas wrote:
> > > +/* NHI genetlink commands */
> > > +enum {
> > > + NHI_CMD_UNSPEC,
> > > + NHI_CMD_SUBSCRIBE,
> > > + NHI_CMD_UNSUBSCRIBE,
> > > + NHI_CMD_QUERY_INFORMATION,
> > > + NHI_CMD_MSG_TO_ICM,
> > > + NHI_CMD_MSG_FROM_ICM,
> > > + NHI_CMD_MAILBOX,
> > > + NHI_CMD_APPROVE_TBT_NETWORKING,
> > > + NHI_CMD_ICM_IN_SAFE_MODE,
> > > + __NHI_CMD_MAX,
> > > +};
> > > +#define NHI_CMD_MAX (__NHI_CMD_MAX - 1)
> > NHI_CMD_MAX = NHI_CMD_ICM_IN_SAFE_MODE ?
> >
>
> This template is used a lot with (generic) netlink.
> Few examples:
> http://lxr.free-electrons.com/source/drivers/acpi/event.c#L62
> http://lxr.free-electrons.com/source/include/uapi/linux/irda.h#L224
> It is easier to maintain - adding entry in the list will automatically update
> MAX.
Fair enough.
> [...]
>
> > > + u32 status;
> > > +
> > > + status = ioread32(nhi_ctxt->iobase + REG_FW_STS);
> > > +
> > > + if (status & REG_FW_STS_NVM_AUTH_DONE)
> > > + break;
> > > + msleep(30);
> >
> > 30 is big number, for polling, what is behind this?
> >
>
> The NVM authentication can take time for ICM.
> This number comes from experiments.
This deserve some comment, this look very random.
> [...]
>
> > > +static struct tbt_nhi_ctxt *nhi_search_ctxt(u32 id) {
> > > + struct tbt_nhi_ctxt *nhi_ctxt;
> > > +
> > > + list_for_each_entry(nhi_ctxt, &controllers_list, node)
> > > + if (nhi_ctxt->id == id)
> > > + return nhi_ctxt;
> >
> > Don't you need to lock this list with the controllers_list_rwsem ?
> >
>
> This is a helper function for searching the list.
> The callers take and release the lock.
Since this doesn't fit the patterns in your code, the function deserves a comment, that it should be used under lock.
>
> [...]
>
> > > + bool nvm_auth_on_boot : 1;
> > Don't use bool with bit fields use u32
>
> What is the concern here?
> If it is size of bool, it doesn't matter for this struct.
> It is more readable that the expected value is bool, and it is used a lot in
> kernel, for example:
> http://lxr.free-electrons.com/source/include/linux/pm.h#L558
Hmm, actually a nice feature I wasn't aware of, save some space and got bool type checking.
Thanks
[toc] | [prev] | [next] | [standalone]
| From | "Rosen, Rami" <rami.rosen@intel.com> |
|---|---|
| Date | 2016-07-14 17:10 +0200 |
| Subject | RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) |
| Message-ID | <rUQr0-2Rf-29@gated-at.bofh.it> |
| In reply to | #1443373 |
Hi Amir,
Here are my 2 cents:
This method always returns true, should be void (unless you will change PDF_ERROR_NOTIFICATION or other pdf values to return false), and likewise its invocation should not check return value.
> +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> + enum pdf_value pdf,
> + const u8 *msg, u32 msg_len)
> +{
> + /*
> + * preparation for messages that won't be sent,
> + * currently unused in this patch.
> + */
> + bool send_event = true;
> +
> + switch (pdf) {
> + case PDF_ERROR_NOTIFICATION:
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x PDF_ERROR_NOTIFICATION %hhu
> msg len %u\n",
> + nhi_ctxt->id, msg[11], msg_len);
> + /* fallthrough */
> + case PDF_WRITE_CONFIGURATION_REGISTERS:
> + /* fallthrough */
> + case PDF_READ_CONFIGURATION_REGISTERS:
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(&nhi_ctxt->send_sem);
> + }
> + break;
> +
> + case PDF_FW_TO_SW_RESPONSE:
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(&nhi_ctxt->send_sem);
> + }
> + break;
> +
> + default:
> + dev_warn(&nhi_ctxt->pdev->dev,
> + "controller id %#x pdf %u isn't handled/expected\n",
> + nhi_ctxt->id, pdf);
> + break;
> + }
> +
> + return send_event;
> +}
> +
This methods always returns 0, should be void.
> +static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt-
> >send_sem)
> +{
> + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> + void __iomem *rx_reg, *tx_reg;
> + u32 rx_reg_val, tx_reg_val;
> +
> + /* must be after negotiation_events, since messages might be sent
> */
> + nhi_ctxt->d0_exit = true;
> +
> + rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> + (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> + rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> + tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> + (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> + tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> + /* disable RX flow control */
> + iowrite32(rx_reg_val, rx_reg);
> + /* disable TX flow control */
> + iowrite32(tx_reg_val, tx_reg);
> + /* disable RX ring */
> + iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> +
> + mutex_lock(&nhi_ctxt->d0_exit_mailbox_mutex);
> + mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> +
> + cancel_work_sync(&nhi_ctxt->icm_msgs_work);
> +
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + nhi_ctxt->ignore_icm_resp = false;
> + /*
> + * if there is response, it is lost, so unlock the send
> + * for the next resume.
> + */
> + up(&nhi_ctxt->send_sem);
> + }
> +
> + mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> + mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> +
> + /* wait for all TX to finish */
> + usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> +
> + /* disable all interrupts */
> + iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> + /* disable TX ring */
> + iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> +
> + return 0;
> +}
> +
This methods also always returns 0, should be void.
> +static int nhi_resume(struct device *dev) __acquires(&nhi_ctxt-
> >send_sem)
> +{
> + dma_addr_t phys;
> + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> + struct tbt_buf_desc *desc;
> + void __iomem *iobase = nhi_ctxt->iobase;
> + void __iomem *reg;
> + int i;
> +
> + if (nhi_ctxt->msix_entries) {
> + iowrite32(ioread32(iobase + REG_DMA_MISC) |
> +
> REG_DMA_MISC_INT_AUTO_CLEAR,
> + iobase + REG_DMA_MISC);
> + /*
> + * Vector #0, which is TX complete to ICM,
> + * isn't been used currently.
> + */
> + nhi_set_int_vec(nhi_ctxt, 0, 1);
> +
> + for (i = 2; i < nhi_ctxt->num_vectors; i++)
> + nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> (i/2),
> + i);
> + }
> +
> + /* configure TX descriptors */
> + for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> + i < TBT_ICM_RING_NUM_TX_BUFS;
> + i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> + desc = &nhi_ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> + desc->phys = cpu_to_le64(phys);
> + desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> + }
> + /* configure RX descriptors */
> + for (i = 0;
> + i < TBT_ICM_RING_NUM_RX_BUFS;
> + i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> + desc = &nhi_ctxt->icm_ring_shared_mem->rx_buf_desc[i];
> + desc->phys = cpu_to_le64(phys);
> + desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> + DESC_ATTR_INT_EN);
> + }
> +
> + /* configure throttling rate for interrupts */
> + for (i = 0, reg = iobase + REG_INT_THROTTLING_RATE;
> + i < NUM_INT_VECTORS;
> + i++, reg += REG_INT_THROTTLING_RATE_STEP) {
> + iowrite32(USEC_TO_256_NSECS(128), reg);
> + }
> +
> + /* configure TX for ICM ring */
> + reg = iobase + REG_TX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> + phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> + offsetof(struct tbt_icm_ring_shared_memory, tx_buf_desc);
> + iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> + iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> + iowrite32((TBT_ICM_RING_NUM_TX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> + REG_RING_SIZE_MASK,
> + reg + REG_RING_SIZE_OFFSET);
> +
> + reg = iobase + REG_TX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> + iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> + /* configure RX for ICM ring */
> + reg = iobase + REG_RX_RING_BASE + (TBT_ICM_RING_NUM *
> REG_RING_STEP);
> + phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> + offsetof(struct tbt_icm_ring_shared_memory, rx_buf_desc);
> + iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> + iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> + iowrite32(((TBT_ICM_RING_NUM_RX_BUFS <<
> REG_RING_SIZE_SHIFT) &
> + REG_RING_SIZE_MASK) |
> + ((TBT_ICM_RING_MAX_FRAME_SIZE <<
> REG_RING_BUF_SIZE_SHIFT) &
> + REG_RING_BUF_SIZE_MASK),
> + reg + REG_RING_SIZE_OFFSET);
> + iowrite32(((TBT_ICM_RING_NUM_RX_BUFS - 1) <<
> REG_RING_CONS_SHIFT) &
> + REG_RING_CONS_MASK,
> + reg + REG_RING_CONS_PROD_OFFSET);
> +
> + reg = iobase + REG_RX_OPTIONS_BASE +
> (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> + iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> +
> + /* enable RX interrupt */
> + RING_INT_ENABLE_RX(iobase, TBT_ICM_RING_NUM, nhi_ctxt-
> >num_paths);
> +
> + if (likely((atomic_read(&subscribers) > 0) &&
> + nhi_nvm_authenticated(nhi_ctxt))) {
> + down(&nhi_ctxt->send_sem);
> + nhi_ctxt->d0_exit = false;
> + mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> + /*
> + * interrupts are enabled here before send due to
> + * implicit barrier in mutex
> + */
> + nhi_send_driver_ready_command(nhi_ctxt);
> + mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> + } else {
> + nhi_ctxt->d0_exit = false;
> + }
> +
> + return 0;
> +}
Regards,
Rami Rosen
Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | "Levy, Amir (Jer)" <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-14 20:40 +0200 |
| Subject | RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) |
| Message-ID | <rUTIe-4Mw-37@gated-at.bofh.it> |
| In reply to | #1443515 |
On Thu, Jul 14 2016, 06:08 PM, Rosen, Rami wrote:
> Hi Amir,
Hi Rami,
> Here are my 2 cents:
>
> This method always returns true, should be void (unless you will change
> PDF_ERROR_NOTIFICATION or other pdf values to return false), and
> likewise its invocation should not check return value.
>
This patch is the communication with the FW.
The network functionality is added in the next patches in the series
and with it, more messages from FW.
Indeed this function always returns true in this patch,
but while writing it, I predicted that the network functionality will
use the send_event flag differently.
You can see the documentation of send_event - currently unused in this patch.
I don't see any harm that in this patch, this function will always return true,
while applying the rest of the series will change it.
> > +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> > + enum pdf_value pdf,
> > + const u8 *msg, u32 msg_len)
> > +{
> > + /*
> > + * preparation for messages that won't be sent,
> > + * currently unused in this patch.
> > + */
> > + bool send_event = true;
> > +
> > + switch (pdf) {
> > + case PDF_ERROR_NOTIFICATION:
> > + dev_err(&nhi_ctxt->pdev->dev,
> > + "controller id %#x PDF_ERROR_NOTIFICATION %hhu
> > msg len %u\n",
> > + nhi_ctxt->id, msg[11], msg_len);
> > + /* fallthrough */
> > + case PDF_WRITE_CONFIGURATION_REGISTERS:
> > + /* fallthrough */
> > + case PDF_READ_CONFIGURATION_REGISTERS:
> > + if (nhi_ctxt->wait_for_icm_resp) {
> > + nhi_ctxt->wait_for_icm_resp = false;
> > + up(&nhi_ctxt->send_sem);
> > + }
> > + break;
> > +
> > + case PDF_FW_TO_SW_RESPONSE:
> > + if (nhi_ctxt->wait_for_icm_resp) {
> > + nhi_ctxt->wait_for_icm_resp = false;
> > + up(&nhi_ctxt->send_sem);
> > + }
> > + break;
> > +
> > + default:
> > + dev_warn(&nhi_ctxt->pdev->dev,
> > + "controller id %#x pdf %u isn't handled/expected\n",
> > + nhi_ctxt->id, pdf);
> > + break;
> > + }
> > +
> > + return send_event;
> > +}
> > +
>
> This methods always returns 0, should be void.
>
The prototype of suspend is return int:
http://lxr.free-electrons.com/source/include/linux/pm.h#L295
> > +static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt-
> > >send_sem)
> > +{
> > + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> > + void __iomem *rx_reg, *tx_reg;
> > + u32 rx_reg_val, tx_reg_val;
> > +
> > + /* must be after negotiation_events, since messages might be sent
> > */
> > + nhi_ctxt->d0_exit = true;
> > +
> > + rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> > + (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> > + rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> > + tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> > + (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> > + tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> > + /* disable RX flow control */
> > + iowrite32(rx_reg_val, rx_reg);
> > + /* disable TX flow control */
> > + iowrite32(tx_reg_val, tx_reg);
> > + /* disable RX ring */
> > + iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> > +
> > + mutex_lock(&nhi_ctxt->d0_exit_mailbox_mutex);
> > + mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> > +
> > + cancel_work_sync(&nhi_ctxt->icm_msgs_work);
> > +
> > + if (nhi_ctxt->wait_for_icm_resp) {
> > + nhi_ctxt->wait_for_icm_resp = false;
> > + nhi_ctxt->ignore_icm_resp = false;
> > + /*
> > + * if there is response, it is lost, so unlock the send
> > + * for the next resume.
> > + */
> > + up(&nhi_ctxt->send_sem);
> > + }
> > +
> > + mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> > + mutex_unlock(&nhi_ctxt->d0_exit_mailbox_mutex);
> > +
> > + /* wait for all TX to finish */
> > + usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> > +
> > + /* disable all interrupts */
> > + iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> > + /* disable TX ring */
> > + iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> > +
> > + return 0;
> > +}
> > +
>
> This methods also always returns 0, should be void.
>
The prototype of resume is return int:
http://lxr.free-electrons.com/source/include/linux/pm.h#L295
> > +static int nhi_resume(struct device *dev) __acquires(&nhi_ctxt-
> > >send_sem)
> > +{
> > + dma_addr_t phys;
> > + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> > + struct tbt_buf_desc *desc;
> > + void __iomem *iobase = nhi_ctxt->iobase;
> > + void __iomem *reg;
> > + int i;
> > +
> > + if (nhi_ctxt->msix_entries) {
> > + iowrite32(ioread32(iobase + REG_DMA_MISC) |
> > +
> > REG_DMA_MISC_INT_AUTO_CLEAR,
> > + iobase + REG_DMA_MISC);
> > + /*
> > + * Vector #0, which is TX complete to ICM,
> > + * isn't been used currently.
> > + */
> > + nhi_set_int_vec(nhi_ctxt, 0, 1);
> > +
> > + for (i = 2; i < nhi_ctxt->num_vectors; i++)
> > + nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> > (i/2),
> > + i);
> > + }
> > +
> > + /* configure TX descriptors */
> > + for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> > + i < TBT_ICM_RING_NUM_TX_BUFS;
> > + i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> > + desc = &nhi_ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> > + desc->phys = cpu_to_le64(phys);
> > + desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> > + }
> > + /* configure RX descriptors */
> > + for (i = 0;
> > + i < TBT_ICM_RING_NUM_RX_BUFS;
> > + i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> > + desc = &nhi_ctxt->icm_ring_shared_mem->rx_buf_desc[i];
> > + desc->phys = cpu_to_le64(phys);
> > + desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS |
> > + DESC_ATTR_INT_EN);
> > + }
> > +
> > + /* configure throttling rate for interrupts */
> > + for (i = 0, reg = iobase + REG_INT_THROTTLING_RATE;
> > + i < NUM_INT_VECTORS;
> > + i++, reg += REG_INT_THROTTLING_RATE_STEP) {
> > + iowrite32(USEC_TO_256_NSECS(128), reg);
> > + }
> > +
> > + /* configure TX for ICM ring */
> > + reg = iobase + REG_TX_RING_BASE + (TBT_ICM_RING_NUM *
> > REG_RING_STEP);
> > + phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> > + offsetof(struct tbt_icm_ring_shared_memory, tx_buf_desc);
> > + iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> > + iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> > + iowrite32((TBT_ICM_RING_NUM_TX_BUFS <<
> > REG_RING_SIZE_SHIFT) &
> > + REG_RING_SIZE_MASK,
> > + reg + REG_RING_SIZE_OFFSET);
> > +
> > + reg = iobase + REG_TX_OPTIONS_BASE +
> > (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> > + iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> > +
> > + /* configure RX for ICM ring */
> > + reg = iobase + REG_RX_RING_BASE + (TBT_ICM_RING_NUM *
> > REG_RING_STEP);
> > + phys = nhi_ctxt->icm_ring_shared_mem_dma_addr +
> > + offsetof(struct tbt_icm_ring_shared_memory, rx_buf_desc);
> > + iowrite32(lower_32_bits(phys), reg + REG_RING_PHYS_LO_OFFSET);
> > + iowrite32(upper_32_bits(phys), reg + REG_RING_PHYS_HI_OFFSET);
> > + iowrite32(((TBT_ICM_RING_NUM_RX_BUFS <<
> > REG_RING_SIZE_SHIFT) &
> > + REG_RING_SIZE_MASK) |
> > + ((TBT_ICM_RING_MAX_FRAME_SIZE <<
> > REG_RING_BUF_SIZE_SHIFT) &
> > + REG_RING_BUF_SIZE_MASK),
> > + reg + REG_RING_SIZE_OFFSET);
> > + iowrite32(((TBT_ICM_RING_NUM_RX_BUFS - 1) <<
> > REG_RING_CONS_SHIFT) &
> > + REG_RING_CONS_MASK,
> > + reg + REG_RING_CONS_PROD_OFFSET);
> > +
> > + reg = iobase + REG_RX_OPTIONS_BASE +
> > (TBT_ICM_RING_NUM*REG_OPTS_STEP);
> > + iowrite32(REG_OPTS_RAW | REG_OPTS_VALID, reg);
> > +
> > + /* enable RX interrupt */
> > + RING_INT_ENABLE_RX(iobase, TBT_ICM_RING_NUM, nhi_ctxt-
> > >num_paths);
> > +
> > + if (likely((atomic_read(&subscribers) > 0) &&
> > + nhi_nvm_authenticated(nhi_ctxt))) {
> > + down(&nhi_ctxt->send_sem);
> > + nhi_ctxt->d0_exit = false;
> > + mutex_lock(&nhi_ctxt->d0_exit_send_mutex);
> > + /*
> > + * interrupts are enabled here before send due to
> > + * implicit barrier in mutex
> > + */
> > + nhi_send_driver_ready_command(nhi_ctxt);
> > + mutex_unlock(&nhi_ctxt->d0_exit_send_mutex);
> > + } else {
> > + nhi_ctxt->d0_exit = false;
> > + }
> > +
> > + return 0;
> > +}
>
>
> Regards,
> Rami Rosen
> Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | "Rosen, Rami" <rami.rosen@intel.com> |
|---|---|
| Date | 2016-07-14 21:10 +0200 |
| Subject | RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware) |
| Message-ID | <rUUbg-5bU-23@gated-at.bofh.it> |
| In reply to | #1443675 |
Hi, Amir, >This patch is the communication with the FW. >The network functionality is added in the next patches in the series >and with it, more messages from FW. >Indeed this function always returns true in this patch, >but while writing it, I predicted that the network functionality will >use the send_event flag differently. >You can see the documentation of send_event - currently unused in this patch. >I don't see any harm that in this patch, this function will always return true, >while applying the rest of the series will change it. Ok, seems reasonable. >The prototype of suspend is return int: >http://lxr.free-electrons.com/source/include/linux/pm.h#L295 >The prototype of resume is return int: >http://lxr.free-electrons.com/source/include/linux/pm.h#L295 You are right about this, I missed the macro invocation in your code, SET_SYSTEM_SLEEP_PM_OPS(nhi_suspend, nhi_resume), apologies. Regards, Rami Rosen Intel Corporation
[toc] | [prev] | [next] | [standalone]
| From | Paul Gortmaker <paul.gortmaker@windriver.com> |
|---|---|
| Date | 2016-07-15 02:30 +0200 |
| Subject | Re: [PATCH v3 6/8] thunderbolt: Networking state machine |
| Message-ID | <rUZaW-8gA-7@gated-at.bofh.it> |
| In reply to | #1443373 |
On Thu, Jul 14, 2016 at 7:28 AM, Amir Levy <amir.jer.levy@intel.com> wrote:
> Negotiation states that a peer goes through in order to establish
> the communication with the second peer.
> This includes communication with upper layer and additional
> infrastructure support to communicate with the second peer through ICM.
>
> Signed-off-by: Amir Levy <amir.jer.levy@intel.com>
> ---
> drivers/thunderbolt/icm/Makefile | 2 +-
> drivers/thunderbolt/icm/icm_nhi.c | 304 ++++++++++++++-
> drivers/thunderbolt/icm/net.c | 802 ++++++++++++++++++++++++++++++++++++++
> drivers/thunderbolt/icm/net.h | 74 ++++
> 4 files changed, 1171 insertions(+), 11 deletions(-)
> create mode 100644 drivers/thunderbolt/icm/net.c
>
> diff --git a/drivers/thunderbolt/icm/Makefile b/drivers/thunderbolt/icm/Makefile
> index 3adfc35..624ee31 100644
> --- a/drivers/thunderbolt/icm/Makefile
> +++ b/drivers/thunderbolt/icm/Makefile
> @@ -25,4 +25,4 @@
> ################################################################################
>
> obj-${CONFIG_THUNDERBOLT_ICM} += thunderbolt-icm.o
> -thunderbolt-icm-objs := icm_nhi.o
> +thunderbolt-icm-objs := icm_nhi.o net.o
> diff --git a/drivers/thunderbolt/icm/icm_nhi.c b/drivers/thunderbolt/icm/icm_nhi.c
> index 9d178a5..060bb38 100644
> --- a/drivers/thunderbolt/icm/icm_nhi.c
> +++ b/drivers/thunderbolt/icm/icm_nhi.c
> @@ -101,6 +101,12 @@ static const struct nla_policy nhi_genl_policy[NHI_ATTR_MAX + 1] = {
> .len = TBT_ICM_RING_MAX_FRAME_SIZE },
> [NHI_ATTR_MSG_FROM_ICM] = { .type = NLA_BINARY,
> .len = TBT_ICM_RING_MAX_FRAME_SIZE },
> + [NHI_ATTR_LOCAL_ROUTE_STRING] = {.len = sizeof(struct route_string)},
> + [NHI_ATTR_LOCAL_UNIQUE_ID] = { .len = sizeof(unique_id) },
> + [NHI_ATTR_REMOTE_UNIQUE_ID] = { .len = sizeof(unique_id) },
> + [NHI_ATTR_LOCAL_DEPTH] = { .type = NLA_U8, },
> + [NHI_ATTR_ENABLE_FULL_E2E] = { .type = NLA_FLAG, },
> + [NHI_ATTR_MATCH_FRAME_ID] = { .type = NLA_FLAG, },
> };
>
> /* NHI genetlink family */
> @@ -531,6 +537,29 @@ int nhi_mailbox(struct tbt_nhi_ctxt *nhi_ctxt, u32 cmd, u32 data, bool deinit)
> return 0;
> }
>
> +static inline bool nhi_is_path_disconnected(u32 cmd, u8 num_ports)
> +{
> + return (cmd >= DISCONNECT_PORT_A_INTER_DOMAIN_PATH &&
> + cmd < (DISCONNECT_PORT_A_INTER_DOMAIN_PATH + num_ports));
> +}
> +
> +static int nhi_mailbox_disconn_path(struct tbt_nhi_ctxt *nhi_ctxt, u32 cmd)
> + __releases(&controllers_list_rwsem)
> +{
> + struct port_net_dev *port;
> + u32 port_num = cmd - DISCONNECT_PORT_A_INTER_DOMAIN_PATH;
> +
> + port = &(nhi_ctxt->net_devices[port_num]);
> + mutex_lock(&port->state_mutex);
> +
> + up_read(&controllers_list_rwsem);
> + port->medium_sts = MEDIUM_READY_FOR_APPROVAL;
> + if (port->net_dev)
> + negotiation_events(port->net_dev, MEDIUM_DISCONNECTED);
> + mutex_unlock(&port->state_mutex);
> + return 0;
> +}
> +
> static int nhi_mailbox_generic(struct tbt_nhi_ctxt *nhi_ctxt, u32 mb_cmd)
> __releases(&controllers_list_rwsem)
> {
> @@ -579,13 +608,93 @@ static int nhi_genl_mailbox(__always_unused struct sk_buff *u_skb,
> down_read(&controllers_list_rwsem);
>
> nhi_ctxt = nhi_search_ctxt(*(u32 *)info->userhdr);
> - if (nhi_ctxt && !nhi_ctxt->d0_exit)
> - return nhi_mailbox_generic(nhi_ctxt, mb_cmd);
> + if (nhi_ctxt && !nhi_ctxt->d0_exit) {
> +
> + /* rwsem is released later by the below functions */
> + if (nhi_is_path_disconnected(cmd, nhi_ctxt->num_ports))
> + return nhi_mailbox_disconn_path(nhi_ctxt, cmd);
> + else
> + return nhi_mailbox_generic(nhi_ctxt, mb_cmd);
> +
> + }
>
> up_read(&controllers_list_rwsem);
> return -ENODEV;
> }
>
> +static int nhi_genl_approve_networking(__always_unused struct sk_buff *u_skb,
> + struct genl_info *info)
> +{
> + struct tbt_nhi_ctxt *nhi_ctxt;
> + struct route_string *route_str;
> + int res = -ENODEV;
> + u8 port_num;
> +
> + if (!info || !info->userhdr || !info->attrs ||
> + !info->attrs[NHI_ATTR_LOCAL_ROUTE_STRING] ||
> + !info->attrs[NHI_ATTR_LOCAL_UNIQUE_ID] ||
> + !info->attrs[NHI_ATTR_REMOTE_UNIQUE_ID] ||
> + !info->attrs[NHI_ATTR_LOCAL_DEPTH])
> + return -EINVAL;
> +
> + /*
> + * route_str is an unique topological address
> + * used for approving remote controller
> + */
> + route_str = nla_data(info->attrs[NHI_ATTR_LOCAL_ROUTE_STRING]);
> + /* extracts the port we're connected to */
> + port_num = PORT_NUM_FROM_LINK(L0_PORT_NUM(route_str->lo));
> +
> + down_read(&controllers_list_rwsem);
> +
> + nhi_ctxt = nhi_search_ctxt(*(u32 *)info->userhdr);
> + if (nhi_ctxt && !nhi_ctxt->d0_exit) {
> + struct port_net_dev *port;
> +
> + if (port_num >= nhi_ctxt->num_ports) {
> + res = -EINVAL;
> + goto free_ctl_list;
> + }
> +
> + port = &(nhi_ctxt->net_devices[port_num]);
> +
> + mutex_lock(&port->state_mutex);
> + up_read(&controllers_list_rwsem);
> +
> + if (port->medium_sts != MEDIUM_READY_FOR_APPROVAL) {
> + dev_info(&nhi_ctxt->pdev->dev,
> + "%s: controller id %#x in state %u <> MEDIUM_READY_FOR_APPROVAL\n",
> + __func__, nhi_ctxt->id, port->medium_sts);
> + goto unlock;
> + }
> +
> + port->medium_sts = MEDIUM_READY_FOR_CONNECTION;
> +
> + if (!port->net_dev) {
> + port->net_dev = nhi_alloc_etherdev(nhi_ctxt, port_num,
> + info);
> + if (!port->net_dev) {
> + mutex_unlock(&port->state_mutex);
> + return -ENOMEM;
> + }
> + } else {
> + nhi_update_etherdev(nhi_ctxt, port->net_dev, info);
> +
> + negotiation_events(port->net_dev,
> + MEDIUM_READY_FOR_CONNECTION);
> + }
> +
> +unlock:
> + mutex_unlock(&port->state_mutex);
> +
> + return 0;
> + }
> +
> +free_ctl_list:
> + up_read(&controllers_list_rwsem);
> +
> + return res;
> +}
>
> static int nhi_genl_send_msg(struct tbt_nhi_ctxt *nhi_ctxt, enum pdf_value pdf,
> const u8 *msg, u32 msg_len)
> @@ -635,17 +744,169 @@ genl_put_reply_failure:
> return res;
> }
>
> +static bool nhi_handle_inter_domain_msg(struct tbt_nhi_ctxt *nhi_ctxt,
> + struct thunderbolt_ip_header *hdr)
> +{
> + struct port_net_dev *port;
> + u8 port_num;
> +
> + const unique_id_be proto_uuid = APPLE_THUNDERBOLT_IP_PROTOCOL_UUID;
> +
> + if (memcmp(proto_uuid, hdr->apple_tbt_ip_proto_uuid,
> + sizeof(proto_uuid)) != 0) {
> + dev_dbg(&nhi_ctxt->pdev->dev,
> + "controller id %#x XDomain discovery message\n",
> + nhi_ctxt->id);
> + return true;
> + }
> +
> + dev_dbg(&nhi_ctxt->pdev->dev,
> + "controller id %#x ThunderboltIP %u\n",
> + nhi_ctxt->id, be32_to_cpu(hdr->packet_type));
> +
> + port_num = PORT_NUM_FROM_LINK(
> + L0_PORT_NUM(be32_to_cpu(hdr->route_str.lo)));
> +
> + if (unlikely(port_num >= nhi_ctxt->num_ports)) {
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x invalid port %u in ThunderboltIP message\n",
> + nhi_ctxt->id, port_num);
> + return false;
> + }
> +
> + port = &(nhi_ctxt->net_devices[port_num]);
> + mutex_lock(&port->state_mutex);
> + if (likely(port->net_dev != NULL))
> + negotiation_messages(port->net_dev, hdr);
> + else
> + dev_notice(&nhi_ctxt->pdev->dev,
> + "controller id %#x port %u in ThunderboltIP message was not initialized\n",
> + nhi_ctxt->id, port_num);
> + mutex_unlock(&port->state_mutex);
> +
> + return false;
> +}
> +
> +static void nhi_handle_notification_msg(struct tbt_nhi_ctxt *nhi_ctxt,
> + const u8 *msg)
> +{
> + struct port_net_dev *port;
> + u8 port_num;
> +
> +#define INTER_DOMAIN_LINK_SHIFT 0
> +#define INTER_DOMAIN_LINK_MASK GENMASK(2, INTER_DOMAIN_LINK_SHIFT)
> + switch (msg[3]) {
> +
> + case NC_INTER_DOMAIN_CONNECTED:
> + port_num = PORT_NUM_FROM_MSG(msg[5]);
> +#define INTER_DOMAIN_APPROVED BIT(3)
> + if (likely(port_num < nhi_ctxt->num_ports)) {
> + if (!(msg[5] & INTER_DOMAIN_APPROVED))
> + nhi_ctxt->net_devices[
> + port_num].medium_sts =
> + MEDIUM_READY_FOR_APPROVAL;
> + } else {
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x invalid port %u in inter domain connected message\n",
> + nhi_ctxt->id, port_num);
> + }
> + break;
> +
> + case NC_INTER_DOMAIN_DISCONNECTED:
> + port_num = PORT_NUM_FROM_MSG(msg[5]);
> +
> + if (unlikely(port_num >= nhi_ctxt->num_ports)) {
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x invalid port %u in inter domain disconnected message\n",
> + nhi_ctxt->id, port_num);
> + break;
> + }
> +
> + port = &(nhi_ctxt->net_devices[port_num]);
> + mutex_lock(&port->state_mutex);
> + port->medium_sts = MEDIUM_DISCONNECTED;
> +
> + if (likely(port->net_dev != NULL))
> + negotiation_events(port->net_dev,
> + MEDIUM_DISCONNECTED);
> + else
> + dev_notice(&nhi_ctxt->pdev->dev,
> + "controller id %#x port %u in inter domain disconnected message was not initialized\n",
> + nhi_ctxt->id, port_num);
> + mutex_unlock(&port->state_mutex);
> + break;
> + }
> +}
> +
> +static bool nhi_handle_icm_response_msg(struct tbt_nhi_ctxt *nhi_ctxt,
> + const u8 *msg)
> +{
> + struct port_net_dev *port;
> + bool send_event = true;
> + u8 port_num;
> +
> + if (nhi_ctxt->ignore_icm_resp &&
> + msg[3] == RC_INTER_DOMAIN_PKT_SENT) {
> + nhi_ctxt->ignore_icm_resp = false;
> + send_event = false;
> + }
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(&nhi_ctxt->send_sem);
> + }
> +
> + if (msg[3] == RC_APPROVE_INTER_DOMAIN_CONNEXION) {
> +#define APPROVE_INTER_DOMAIN_ERROR BIT(0)
> + if (unlikely(msg[2] & APPROVE_INTER_DOMAIN_ERROR)) {
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x inter domain approve error\n",
> + nhi_ctxt->id);
> + return send_event;
> + }
> + port_num = PORT_NUM_FROM_LINK((msg[5]&INTER_DOMAIN_LINK_MASK)>>
> + INTER_DOMAIN_LINK_SHIFT);
> +
> + if (unlikely(port_num >= nhi_ctxt->num_ports)) {
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x invalid port %u in inter domain approve message\n",
> + nhi_ctxt->id, port_num);
> + return send_event;
> + }
> +
> + port = &(nhi_ctxt->net_devices[port_num]);
> + mutex_lock(&port->state_mutex);
> + port->medium_sts = MEDIUM_CONNECTED;
> +
> + if (likely(port->net_dev != NULL))
> + negotiation_events(port->net_dev, MEDIUM_CONNECTED);
> + else
> + dev_err(&nhi_ctxt->pdev->dev,
> + "controller id %#x port %u in inter domain approve message was not initialized\n",
> + nhi_ctxt->id, port_num);
> + mutex_unlock(&port->state_mutex);
> + }
> +
> + return send_event;
> +}
> +
> static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> enum pdf_value pdf,
> const u8 *msg, u32 msg_len)
> {
> - /*
> - * preparation for messages that won't be sent,
> - * currently unused in this patch.
> - */
> bool send_event = true;
>
> switch (pdf) {
> + case PDF_INTER_DOMAIN_REQUEST:
> + case PDF_INTER_DOMAIN_RESPONSE:
> + send_event = nhi_handle_inter_domain_msg(
> + nhi_ctxt,
> + (struct thunderbolt_ip_header *)msg);
> + break;
> +
> + case PDF_FW_TO_SW_NOTIFICATION:
> + nhi_handle_notification_msg(nhi_ctxt, msg);
> + break;
> +
> case PDF_ERROR_NOTIFICATION:
> dev_err(&nhi_ctxt->pdev->dev,
> "controller id %#x PDF_ERROR_NOTIFICATION %hhu msg len %u\n",
> @@ -661,10 +922,7 @@ static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> break;
>
> case PDF_FW_TO_SW_RESPONSE:
> - if (nhi_ctxt->wait_for_icm_resp) {
> - nhi_ctxt->wait_for_icm_resp = false;
> - up(&nhi_ctxt->send_sem);
> - }
> + send_event = nhi_handle_icm_response_msg(nhi_ctxt, msg);
> break;
>
> default:
> @@ -869,6 +1127,12 @@ static const struct genl_ops nhi_ops[] = {
> .doit = nhi_genl_mailbox,
> .flags = GENL_ADMIN_PERM,
> },
> + {
> + .cmd = NHI_CMD_APPROVE_TBT_NETWORKING,
> + .policy = nhi_genl_policy,
> + .doit = nhi_genl_approve_networking,
> + .flags = GENL_ADMIN_PERM,
> + },
> };
>
> static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt->send_sem)
> @@ -876,6 +1140,17 @@ static int nhi_suspend(struct device *dev) __releases(&nhi_ctxt->send_sem)
> struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> void __iomem *rx_reg, *tx_reg;
> u32 rx_reg_val, tx_reg_val;
> + int i;
> +
> + for (i = 0; i < nhi_ctxt->num_ports; i++) {
> + struct port_net_dev *port = &nhi_ctxt->net_devices[i];
> +
> + mutex_lock(&port->state_mutex);
> + port->medium_sts = MEDIUM_DISCONNECTED;
> + if (port->net_dev)
> + negotiation_events(port->net_dev, MEDIUM_DISCONNECTED);
> + mutex_unlock(&port->state_mutex);
> + }
>
> /* must be after negotiation_events, since messages might be sent */
> nhi_ctxt->d0_exit = true;
> @@ -1035,6 +1310,15 @@ static void icm_nhi_remove(struct pci_dev *pdev)
>
> nhi_suspend(&pdev->dev);
>
> + for (i = 0; i < nhi_ctxt->num_ports; i++) {
> + mutex_lock(&nhi_ctxt->net_devices[i].state_mutex);
> + if (nhi_ctxt->net_devices[i].net_dev) {
> + nhi_dealloc_etherdev(nhi_ctxt->net_devices[i].net_dev);
> + nhi_ctxt->net_devices[i].net_dev = NULL;
> + }
> + mutex_unlock(&nhi_ctxt->net_devices[i].state_mutex);
> + }
> +
> if (nhi_ctxt->net_workqueue)
> destroy_workqueue(nhi_ctxt->net_workqueue);
>
> diff --git a/drivers/thunderbolt/icm/net.c b/drivers/thunderbolt/icm/net.c
> new file mode 100644
> index 0000000..e983dfb
> --- /dev/null
> +++ b/drivers/thunderbolt/icm/net.c
> @@ -0,0 +1,802 @@
> +/*******************************************************************************
> + *
> + * Intel Thunderbolt(TM) driver
> + * Copyright(c) 2014 - 2016 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program. If not, see <http://www.gnu.org/licenses/>.
> + *
> + * The full GNU General Public License is included in this distribution in
> + * the file called "COPYING".
> + *
> + * Contact Information:
> + * Intel Thunderbolt Mailing List <thunderbolt-software@lists.01.org>
> + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
> + *
> + ******************************************************************************/
> +
> +#include <linux/module.h>
I did not see anything like module_init in this driver; I think it
seems like you
want linux/moduleparam.h here.
Thanks,
Paul.
--
> +#include <linux/etherdevice.h>
> +#include <linux/crc32.h>
> +#include <linux/prefetch.h>
> +#include <linux/highmem.h>
> +#include <linux/if_vlan.h>
> +#include <linux/jhash.h>
> +#include <linux/vmalloc.h>
> +#include <net/ip6_checksum.h>
> +#include "icm_nhi.h"
> +#include "net.h"
>
[toc] | [prev] | [next] | [standalone]
| From | "Levy, Amir (Jer)" <amir.jer.levy@intel.com> |
|---|---|
| Date | 2016-07-17 09:10 +0200 |
| Subject | RE: [PATCH v3 6/8] thunderbolt: Networking state machine |
| Message-ID | <rVOn8-6ad-7@gated-at.bofh.it> |
| In reply to | #1443829 |
On Fri, Jul 15 2016, 03:25 AM, Paul Gortmaker wrote: > > diff --git a/drivers/thunderbolt/icm/net.c > > b/drivers/thunderbolt/icm/net.c new file mode 100644 index > > 0000000..e983dfb > > --- /dev/null > > +++ b/drivers/thunderbolt/icm/net.c > > @@ -0,0 +1,802 @@ > > > +/********************************************************* > *********** > > +*********** > > + * > > + * Intel Thunderbolt(TM) driver > > + * Copyright(c) 2014 - 2016 Intel Corporation. > > + * > > + * This program is free software; you can redistribute it and/or > > +modify it > > + * under the terms and conditions of the GNU General Public License, > > + * version 2, as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope it will be useful, but > > +WITHOUT > > + * ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY > > +or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public > > +License for > > + * more details. > > + * > > + * You should have received a copy of the GNU General Public License > > +along > > + * with this program. If not, see <http://www.gnu.org/licenses/>. > > + * > > + * The full GNU General Public License is included in this > > +distribution in > > + * the file called "COPYING". > > + * > > + * Contact Information: > > + * Intel Thunderbolt Mailing List <thunderbolt-software@lists.01.org> > > + * Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR > > +97124-6497 > > + * > > + > > > +********************************************************* > ************ > > +*********/ > > + > > +#include <linux/module.h> > > I did not see anything like module_init in this driver; I think it seems like you > want linux/moduleparam.h here. > Hi Paul, It looks like leftover from the early development stages. I'll remove it. The module_init is in icm_nhi.c. Thanks, Amir
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web