Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-19 Thread Logan Gunthorpe 


On 19/12/16 10:02 AM, Keith Busch wrote:
> Some of this would be simplified if you use the managed device API's:
> devm_request_irq, pcim_enable_device, pcim_iomap, etc...

Thanks Keith, I'll look into using those.

Logan

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-19 Thread Logan Gunthorpe 


On 19/12/16 10:02 AM, Keith Busch wrote:
> Some of this would be simplified if you use the managed device API's:
> devm_request_irq, pcim_enable_device, pcim_iomap, etc...

Thanks Keith, I'll look into using those.

Logan

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-19 Thread Keith Busch 
On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> Microsemi's "Switchtec" line of PCI switch devices is already
> supported by the kernel with standard PCI switch drivers. However, the
> Switchtec device advertises a special management endpoint which
> enables some additional functionality. This includes:
> 
>  * Packet and Byte Counters
>  * Firmware Upgrades
>  * Event and Error logs
>  * Querying port link status
>  * Custom user firmware commands
> 
> This patch introduces the switchtec kernel module which provides
> pci driver that exposes a char device. The char device provides
> userspace access to this interface through read, write and (optionally)
> poll calls. Currently no ioctls have been implemented but a couple
> may be added in a later revision.
> 
> A short text file is provided which documents the switchtec driver
> and outlines the semantics of using the char device.

Some of this would be simplified if you use the managed device API's:
devm_request_irq, pcim_enable_device, pcim_iomap, etc...

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-19 Thread Keith Busch 
On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> Microsemi's "Switchtec" line of PCI switch devices is already
> supported by the kernel with standard PCI switch drivers. However, the
> Switchtec device advertises a special management endpoint which
> enables some additional functionality. This includes:
> 
>  * Packet and Byte Counters
>  * Firmware Upgrades
>  * Event and Error logs
>  * Querying port link status
>  * Custom user firmware commands
> 
> This patch introduces the switchtec kernel module which provides
> pci driver that exposes a char device. The char device provides
> userspace access to this interface through read, write and (optionally)
> poll calls. Currently no ioctls have been implemented but a couple
> may be added in a later revision.
> 
> A short text file is provided which documents the switchtec driver
> and outlines the semantics of using the char device.

Some of this would be simplified if you use the managed device API's:
devm_request_irq, pcim_enable_device, pcim_iomap, etc...

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-18 Thread Greg Kroah-Hartman 
On Sun, Dec 18, 2016 at 10:20:47AM -0700, Logan Gunthorpe wrote:
> Hi Greg,
> 
> Thanks for the quick review!
> 
> On 18/12/16 12:51 AM, Greg Kroah-Hartman wrote:
> > On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> > > +struct switchtec_dev {
> > > + struct pci_dev *pdev;
> > > + struct msix_entry *msix;
> > > + struct device *dev;
> > > + struct kref kref;
> > 
> > Why do you have a pointer to a device, yet a kref as well?  Just have
> > this structure embed a 'struct device' in itself, like you did for a
> > kref, and you will be fine.  Otherwise you are dealing with two
> > different sets of reference counting here, for no good reason.
> 
> Ok, understood. I had referenced the device dax driver which did it this way
> in 4.8 but looks like it was changed to the way you suggest in 4.9.
> 
> > > +#define stdev_pdev(stdev) ((stdev)->pdev)
> > > +#define stdev_pdev_dev(stdev) (_pdev(stdev)->dev)
> > > +#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
> > > +#define stdev_dev(stdev) ((stdev)->dev)
> > 
> > Ick, just open code these please.  That's a huge hint your use of the
> > driver model is not correct :)
> 
> Ok, will do. For reference, I was copying
> 
> drivers/ntb/hw/intel/ntb_hw_intel.h
> 
> which does a similar thing.

No need to copy bad code, I suggest fixing that up as well :)

thanks,

greg k-h

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-18 Thread Greg Kroah-Hartman 
On Sun, Dec 18, 2016 at 10:20:47AM -0700, Logan Gunthorpe wrote:
> Hi Greg,
> 
> Thanks for the quick review!
> 
> On 18/12/16 12:51 AM, Greg Kroah-Hartman wrote:
> > On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> > > +struct switchtec_dev {
> > > + struct pci_dev *pdev;
> > > + struct msix_entry *msix;
> > > + struct device *dev;
> > > + struct kref kref;
> > 
> > Why do you have a pointer to a device, yet a kref as well?  Just have
> > this structure embed a 'struct device' in itself, like you did for a
> > kref, and you will be fine.  Otherwise you are dealing with two
> > different sets of reference counting here, for no good reason.
> 
> Ok, understood. I had referenced the device dax driver which did it this way
> in 4.8 but looks like it was changed to the way you suggest in 4.9.
> 
> > > +#define stdev_pdev(stdev) ((stdev)->pdev)
> > > +#define stdev_pdev_dev(stdev) (_pdev(stdev)->dev)
> > > +#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
> > > +#define stdev_dev(stdev) ((stdev)->dev)
> > 
> > Ick, just open code these please.  That's a huge hint your use of the
> > driver model is not correct :)
> 
> Ok, will do. For reference, I was copying
> 
> drivers/ntb/hw/intel/ntb_hw_intel.h
> 
> which does a similar thing.

No need to copy bad code, I suggest fixing that up as well :)

thanks,

greg k-h

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-18 Thread Logan Gunthorpe 

Hi Greg,

Thanks for the quick review!

On 18/12/16 12:51 AM, Greg Kroah-Hartman wrote:

On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:

+struct switchtec_dev {
+   struct pci_dev *pdev;
+   struct msix_entry *msix;
+   struct device *dev;
+   struct kref kref;


Why do you have a pointer to a device, yet a kref as well?  Just have
this structure embed a 'struct device' in itself, like you did for a
kref, and you will be fine.  Otherwise you are dealing with two
different sets of reference counting here, for no good reason.


Ok, understood. I had referenced the device dax driver which did it this 
way in 4.8 but looks like it was changed to the way you suggest in 4.9.



+#define stdev_pdev(stdev) ((stdev)->pdev)
+#define stdev_pdev_dev(stdev) (_pdev(stdev)->dev)
+#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
+#define stdev_dev(stdev) ((stdev)->dev)


Ick, just open code these please.  That's a huge hint your use of the
driver model is not correct :)


Ok, will do. For reference, I was copying

drivers/ntb/hw/intel/ntb_hw_intel.h

which does a similar thing.

Logan

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-18 Thread Logan Gunthorpe 

Hi Greg,

Thanks for the quick review!

On 18/12/16 12:51 AM, Greg Kroah-Hartman wrote:

On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:

+struct switchtec_dev {
+   struct pci_dev *pdev;
+   struct msix_entry *msix;
+   struct device *dev;
+   struct kref kref;


Why do you have a pointer to a device, yet a kref as well?  Just have
this structure embed a 'struct device' in itself, like you did for a
kref, and you will be fine.  Otherwise you are dealing with two
different sets of reference counting here, for no good reason.


Ok, understood. I had referenced the device dax driver which did it this 
way in 4.8 but looks like it was changed to the way you suggest in 4.9.



+#define stdev_pdev(stdev) ((stdev)->pdev)
+#define stdev_pdev_dev(stdev) (_pdev(stdev)->dev)
+#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
+#define stdev_dev(stdev) ((stdev)->dev)


Ick, just open code these please.  That's a huge hint your use of the
driver model is not correct :)


Ok, will do. For reference, I was copying

drivers/ntb/hw/intel/ntb_hw_intel.h

which does a similar thing.

Logan

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-18 Thread Greg Kroah-Hartman 
On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> +struct switchtec_dev {
> + struct pci_dev *pdev;
> + struct msix_entry *msix;
> + struct device *dev;
> + struct kref kref;

Why do you have a pointer to a device, yet a kref as well?  Just have
this structure embed a 'struct device' in itself, like you did for a
kref, and you will be fine.  Otherwise you are dealing with two
different sets of reference counting here, for no good reason.

> +
> + unsigned int event_irq;
> +
> + void __iomem *mmio;
> + struct mrpc_regs __iomem *mmio_mrpc;
> + struct ntb_info_regs __iomem *mmio_ntb;
> + struct part_cfg_regs __iomem *mmio_part_cfg;
> +
> + /*
> +  * The mrpc mutex must be held when accessing the other
> +  * mrpc fields
> +  */
> + struct mutex mrpc_mutex;
> + struct list_head mrpc_queue;
> + int mrpc_busy;
> + struct work_struct mrpc_work;
> + struct delayed_work mrpc_timeout;
> +};
> +
> +#define stdev_pdev(stdev) ((stdev)->pdev)
> +#define stdev_pdev_dev(stdev) (_pdev(stdev)->dev)
> +#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
> +#define stdev_dev(stdev) ((stdev)->dev)

Ick, just open code these please.  That's a huge hint your use of the
driver model is not correct :)

thanks,

greg k-h

Re: [RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-18 Thread Greg Kroah-Hartman 
On Sat, Dec 17, 2016 at 10:09:22AM -0700, Logan Gunthorpe wrote:
> +struct switchtec_dev {
> + struct pci_dev *pdev;
> + struct msix_entry *msix;
> + struct device *dev;
> + struct kref kref;

Why do you have a pointer to a device, yet a kref as well?  Just have
this structure embed a 'struct device' in itself, like you did for a
kref, and you will be fine.  Otherwise you are dealing with two
different sets of reference counting here, for no good reason.

> +
> + unsigned int event_irq;
> +
> + void __iomem *mmio;
> + struct mrpc_regs __iomem *mmio_mrpc;
> + struct ntb_info_regs __iomem *mmio_ntb;
> + struct part_cfg_regs __iomem *mmio_part_cfg;
> +
> + /*
> +  * The mrpc mutex must be held when accessing the other
> +  * mrpc fields
> +  */
> + struct mutex mrpc_mutex;
> + struct list_head mrpc_queue;
> + int mrpc_busy;
> + struct work_struct mrpc_work;
> + struct delayed_work mrpc_timeout;
> +};
> +
> +#define stdev_pdev(stdev) ((stdev)->pdev)
> +#define stdev_pdev_dev(stdev) (_pdev(stdev)->dev)
> +#define stdev_name(stdev) pci_name(stdev_pdev(stdev))
> +#define stdev_dev(stdev) ((stdev)->dev)

Ick, just open code these please.  That's a huge hint your use of the
driver model is not correct :)

thanks,

greg k-h

[RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-17 Thread Logan Gunthorpe 
Microsemi's "Switchtec" line of PCI switch devices is already
supported by the kernel with standard PCI switch drivers. However, the
Switchtec device advertises a special management endpoint which
enables some additional functionality. This includes:

 * Packet and Byte Counters
 * Firmware Upgrades
 * Event and Error logs
 * Querying port link status
 * Custom user firmware commands

This patch introduces the switchtec kernel module which provides
pci driver that exposes a char device. The char device provides
userspace access to this interface through read, write and (optionally)
poll calls. Currently no ioctls have been implemented but a couple
may be added in a later revision.

A short text file is provided which documents the switchtec driver
and outlines the semantics of using the char device.

A WIP userspace tool which utilizes this interface is available
at [1]. This tool takes
inspiration (and borrows some code) from nvme-cli [2].

[1] https://github.com/sbates130272/switchtec-user
[2] https://github.com/linux-nvme/nvme-cli

Signed-off-by: Logan Gunthorpe 
Signed-off-by: Stephen Bates 
---
 Documentation/switchtec.txt|  54 +++
 MAINTAINERS|   9 +
 drivers/pci/Kconfig|   1 +
 drivers/pci/Makefile   |   1 +
 drivers/pci/switch/Kconfig |  13 +
 drivers/pci/switch/Makefile|   1 +
 drivers/pci/switch/switchtec.c | 824 +
 drivers/pci/switch/switchtec.h | 119 ++
 8 files changed, 1022 insertions(+)
 create mode 100644 Documentation/switchtec.txt
 create mode 100644 drivers/pci/switch/Kconfig
 create mode 100644 drivers/pci/switch/Makefile
 create mode 100644 drivers/pci/switch/switchtec.c
 create mode 100644 drivers/pci/switch/switchtec.h

diff --git a/Documentation/switchtec.txt b/Documentation/switchtec.txt
new file mode 100644
index 000..04657ce
--- /dev/null
+++ b/Documentation/switchtec.txt
@@ -0,0 +1,54 @@
+
+Linux Switchtec Support
+
+
+Microsemi's "Switchtec" line of PCI switch devices is already
+supported by the kernel with standard PCI switch drivers. However, the
+Switchtec device advertises a special management endpoint which
+enables some additional functionality. This includes:
+
+ * Packet and Byte Counters
+ * Firmware Upgrades
+ * Event and Error logs
+ * Querying port link status
+ * Custom user firmware commands
+
+The switchtec kernel module implements this functionality.
+
+
+
+Interface
+=
+
+The primary means of communicating with the Switchtec management firmware is
+through the Memory-mapped Remote Procedure Call (MRPC) interface.
+Commands are submitted to the interface with a 4-byte command
+identifier and up to 1KB of command specific data. The firmware will
+respond with a 4 bytes return code and up to 1KB of command specific
+data. The interface only processes a single command at a time.
+
+
+Userspace Interface
+===
+
+The MRPC interface will be exposed to userspace through a simple char
+device: /dev/switchtec#, one for each management endpoint in the system.
+
+The char device has the following semantics:
+
+ * A write must consist of at least 4 bytes and no more than 1028 bytes.
+   The first four bytes will be interpreted as the command to run and
+   the remainder will be used as the input data. A write will send the
+   command to the firmware to begin processing.
+
+ * Each write must be followed by exactly one read. Any double write will
+   produce an error and any read that doesn't follow a write will
+   produce an error.
+
+ * A read will block until the firmware completes the command and return
+   the four bytes of status plus up to 1024 bytes of output data. (The
+   length will be specified by the size parameter of the read call --
+   reading less than 4 bytes will produce an error.
+
+ * The poll call will also be supported for userspace applications that
+   need to do other things while waiting for the command to complete.
diff --git a/MAINTAINERS b/MAINTAINERS
index 63cefa6..1e21505 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9288,6 +9288,15 @@ S:   Maintained
 F: Documentation/devicetree/bindings/pci/aardvark-pci.txt
 F: drivers/pci/host/pci-aardvark.c
 
+PCI DRIVER FOR MICROSEMI SWITCHTEC
+M: Kurt Schwemmer 
+M: Stephen Bates 
+M: Logan Gunthorpe 
+L: linux-...@vger.kernel.org
+S: Maintained
+F: Documentation/switchtec.txt
+F: drivers/pci/switch/switchtec*
+
 PCI DRIVER FOR NVIDIA TEGRA
 M: Thierry Reding 
 L: linux-te...@vger.kernel.org
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 6555eb7..f72e8c5 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -133,3 +133,4 @@ config PCI_HYPERV
 
 source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/host/Kconfig"
+source

[RFC 1/1] MicroSemi Switchtec management interface driver

2016-12-17 Thread Logan Gunthorpe 
Microsemi's "Switchtec" line of PCI switch devices is already
supported by the kernel with standard PCI switch drivers. However, the
Switchtec device advertises a special management endpoint which
enables some additional functionality. This includes:

 * Packet and Byte Counters
 * Firmware Upgrades
 * Event and Error logs
 * Querying port link status
 * Custom user firmware commands

This patch introduces the switchtec kernel module which provides
pci driver that exposes a char device. The char device provides
userspace access to this interface through read, write and (optionally)
poll calls. Currently no ioctls have been implemented but a couple
may be added in a later revision.

A short text file is provided which documents the switchtec driver
and outlines the semantics of using the char device.

A WIP userspace tool which utilizes this interface is available
at [1]. This tool takes
inspiration (and borrows some code) from nvme-cli [2].

[1] https://github.com/sbates130272/switchtec-user
[2] https://github.com/linux-nvme/nvme-cli

Signed-off-by: Logan Gunthorpe 
Signed-off-by: Stephen Bates 
---
 Documentation/switchtec.txt|  54 +++
 MAINTAINERS|   9 +
 drivers/pci/Kconfig|   1 +
 drivers/pci/Makefile   |   1 +
 drivers/pci/switch/Kconfig |  13 +
 drivers/pci/switch/Makefile|   1 +
 drivers/pci/switch/switchtec.c | 824 +
 drivers/pci/switch/switchtec.h | 119 ++
 8 files changed, 1022 insertions(+)
 create mode 100644 Documentation/switchtec.txt
 create mode 100644 drivers/pci/switch/Kconfig
 create mode 100644 drivers/pci/switch/Makefile
 create mode 100644 drivers/pci/switch/switchtec.c
 create mode 100644 drivers/pci/switch/switchtec.h

diff --git a/Documentation/switchtec.txt b/Documentation/switchtec.txt
new file mode 100644
index 000..04657ce
--- /dev/null
+++ b/Documentation/switchtec.txt
@@ -0,0 +1,54 @@
+
+Linux Switchtec Support
+
+
+Microsemi's "Switchtec" line of PCI switch devices is already
+supported by the kernel with standard PCI switch drivers. However, the
+Switchtec device advertises a special management endpoint which
+enables some additional functionality. This includes:
+
+ * Packet and Byte Counters
+ * Firmware Upgrades
+ * Event and Error logs
+ * Querying port link status
+ * Custom user firmware commands
+
+The switchtec kernel module implements this functionality.
+
+
+
+Interface
+=
+
+The primary means of communicating with the Switchtec management firmware is
+through the Memory-mapped Remote Procedure Call (MRPC) interface.
+Commands are submitted to the interface with a 4-byte command
+identifier and up to 1KB of command specific data. The firmware will
+respond with a 4 bytes return code and up to 1KB of command specific
+data. The interface only processes a single command at a time.
+
+
+Userspace Interface
+===
+
+The MRPC interface will be exposed to userspace through a simple char
+device: /dev/switchtec#, one for each management endpoint in the system.
+
+The char device has the following semantics:
+
+ * A write must consist of at least 4 bytes and no more than 1028 bytes.
+   The first four bytes will be interpreted as the command to run and
+   the remainder will be used as the input data. A write will send the
+   command to the firmware to begin processing.
+
+ * Each write must be followed by exactly one read. Any double write will
+   produce an error and any read that doesn't follow a write will
+   produce an error.
+
+ * A read will block until the firmware completes the command and return
+   the four bytes of status plus up to 1024 bytes of output data. (The
+   length will be specified by the size parameter of the read call --
+   reading less than 4 bytes will produce an error.
+
+ * The poll call will also be supported for userspace applications that
+   need to do other things while waiting for the command to complete.
diff --git a/MAINTAINERS b/MAINTAINERS
index 63cefa6..1e21505 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9288,6 +9288,15 @@ S:   Maintained
 F: Documentation/devicetree/bindings/pci/aardvark-pci.txt
 F: drivers/pci/host/pci-aardvark.c
 
+PCI DRIVER FOR MICROSEMI SWITCHTEC
+M: Kurt Schwemmer 
+M: Stephen Bates 
+M: Logan Gunthorpe 
+L: linux-...@vger.kernel.org
+S: Maintained
+F: Documentation/switchtec.txt
+F: drivers/pci/switch/switchtec*
+
 PCI DRIVER FOR NVIDIA TEGRA
 M: Thierry Reding 
 L: linux-te...@vger.kernel.org
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 6555eb7..f72e8c5 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -133,3 +133,4 @@ config PCI_HYPERV
 
 source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/host/Kconfig"
+source "drivers/pci/switch/Kconfig"
diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
index 8db5079..15b46dd 100644
--- a/drivers/pci/Makefile
+++