New Microsemi PCI Switch Management Driver

Changes since v4:

* Turns out pushing the pci release code into the device release function didn't work as I would have liked. If you try to unbind the device with an instance open, then you hit a kernel bug at drivers/pci/msi.c:371. (This didn't occur in v3.)

To solve this, we've moved the pci release code back into the unregister function and reintroduced an alive flag. This time, however, the alive flag is protected by mrpc_mutex and we're very careful about what happens to devices still in use (they should all be released through the timeout path and an ENODEV error returned to userspace; while new commands are blocked with the same error).

Changes since v3:

* Removed stdev_is_alive and moved pci deinit functions
into the device release function which only occurs after all cdev instances are closed. (per Bjorn)
* Reduced locking in read/write functions (per Bjorn)
* Use pci_alloc_irq_vectors to vastly improve ISR initialization (Bjorn)
* A few other minor nits and cleanup as noticed by Bjorn

Changes since v2:

* Collected reviewed and tested tags
* Very minor fix to the error path in the create function

Changes since v1:

* Rebased onto 4.10-rc6 (cleanly)
* Split the patch into a few more easily digestible patches (as suggested by Greg Kroah-Hartman)
* Folded switchtec.c into switchtec.h (per Greg)
* Fixed a bunch of 32bit build warnings caught by the kbuild test robot
* Fixed some issues in the documentation so it has a proper reStructredText format (as noted by Jonathan Corbet)
* Fixed padding and sizes in the IOCTL structures as noticed by Emil Velikov and used pahole to verify their consistency across 32 and 64 bit builds
* Reworked one of the IOCTL interfaces to be more future proof (per Emil).

Changes since RFC:

* Fixed incorrect use of the drive model as pointed out by Greg Kroah-Hartman
* Used devm functions as suggested by Keith Busch
* Added a handful of sysfs attributes to the switchtec class
* Added a handful of IOCTLs to the switchtec device
* A number of miscellaneous bug fixes

--

Hi,

This is a continuation of the RFC we posted lasted month [1] which proposes a management driver for Microsemi's Switchtec line of PCI switches. This hardware is still looking to be used in the Open Compute Platform

To make this entirely clear: the Switchtec products are compliant with the PCI specifications and are supported today with the standard in-kernel driver. However, these devices also expose a management endpoint on a separate PCI function address which can be used to perform some advanced operations. This is a driver for that function. See the patch for more information.

Since the RFC, we've made the changes requested by Greg Kroah-Hartman and Keith Busch, and we've also fleshed out a number of features. We've added a couple of IOCTLs and sysfs attributes which are documented in the patch. Significant work has also been done on the userspace tool which is available under a GPL license at [2]. We've also had testing done by some of the interested parties.

We hope to see this work included in either 4.11 or 4.12 assuming a smooth review process.

The patch is based off of the v4.10-rc6 release.

Thanks for your review,

Logan

[1] https://www.spinics.net/lists/linux-pci/msg56897.html
[2] https://github.com/sbates130272/switchtec-user

Logan Gunthorpe (4):
MicroSemi Switchtec management interface driver
switchtec: Add user interface documentation
switchtec: Add sysfs attributes to the Switchtec driver
switchtec: Add IOCTLs to the Switchtec driver

 Documentation/ABI/testing/sysfs-class-switchtec |   96 ++
 Documentation/ioctl/ioctl-number.txt            |    1 +
 Documentation/switchtec.txt                     |   80 ++
 MAINTAINERS                                     |   11 +
 drivers/pci/Kconfig                             |    1 +
 drivers/pci/Makefile                            |    1 +
 drivers/pci/switch/Kconfig                      |   13 +
 drivers/pci/switch/Makefile                     |    1 +
 drivers/pci/switch/switchtec.c                  | 1535 +++++++++++++++++++++++
 include/uapi/linux/switchtec_ioctl.h            |  132 ++
 10 files changed, 1871 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-switchtec
 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 include/uapi/linux/switchtec_ioctl.h

--
2.1.4

Microsemi's "Switchtec" line of PCI switch devices is already well supported by the kernel with standard PCI switch drivers. However, the Switchtec device advertises a special management endpoint with a separate PCI function address and class code. This endpoint enables some additional functionality which includes:

* Packet and Byte Counters
* Switch 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.

A userspace tool and library which utilizes this interface is available at [1]. This tool takes inspiration (and borrows some code) from nvme-cli [2]. The tool is largely complete at this time but additional features may be added in the future.

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

---
 MAINTAINERS                    |   8 +
 drivers/pci/Kconfig            |   1 +
 drivers/pci/Makefile           |   1 +
 drivers/pci/switch/Kconfig     |  13 +
 drivers/pci/switch/Makefile    |   1 +
 drivers/pci/switch/switchtec.c | 955 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 979 insertions(+)
 create mode 100644 drivers/pci/switch/Kconfig
 create mode 100644 drivers/pci/switch/Makefile
 create mode 100644 drivers/pci/switch/switchtec.c

index 527d137..a57686f 100644
@@ -9506,6 +9506,14 @@ 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 <kurt.schwemmer at microsemi.com>
+M:	Stephen Bates <stephen.bates at microsemi.com>
+M:	Logan Gunthorpe <logang at deltatee.com>
+L:	linux-pci at vger.kernel.org
+S:	Maintained
+F:	drivers/pci/switch/switchtec*
+
 PCI DRIVER FOR NVIDIA TEGRA
 M:	Thierry Reding <thierry.reding at gmail.com>
 L:	linux-tegra at vger.kernel.org

index 6555eb7..f72e8c5 100644
@@ -133,3 +133,4 @@ config PCI_HYPERV
 
 source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/host/Kconfig"
+source "drivers/pci/switch/Kconfig"

index 8db5079..15b46dd 100644
@@ -68,3 +68,4 @@ ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
 
 # PCI host controller drivers
 obj-y += host/
+obj-y += switch/

new file mode 100644
index 0000000..4c49648
@@ -0,0 +1,13 @@
+menu "PCI switch controller drivers"
+	depends on PCI
+
+config PCI_SW_SWITCHTEC
+	tristate "MicroSemi Switchtec PCIe Switch Management Driver"
+	help
+	 Enables support for the management interface for the MicroSemi
+	 Switchtec series of PCIe switches. Supports userspace access
+	 to submit MRPC commands to the switch via /dev/switchtecX
+	 devices. See <file:Documentation/switchtec.txt> for more
+	 information.
+
+endmenu

new file mode 100644
index 0000000..37d8cfb
@@ -0,0 +1 @@
+obj-$(CONFIG_PCI_SW_SWITCHTEC) += switchtec.o

2.1.4

This adds standard documentation for the sysfs switchtec attributes and a RST formatted text file which documents the char device interface. Jonathan Corbet has indicated he will move this to a new user-space developer documentation book once it's created.

---
 Documentation/switchtec.txt | 53 +++++++++++++++++++++++++++++++++++++++++++++
 MAINTAINERS                 |  1 +
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/switchtec.txt

new file mode 100644
index 0000000..4bced4c
@@ -0,0 +1,53 @@
+========================
+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.

index a57686f..aa702b0 100644
@@ -9512,6 +9512,7 @@ M:	Stephen Bates <stephen.bates at microsemi.com>
 M:	Logan Gunthorpe <logang at deltatee.com>
 L:	linux-pci at vger.kernel.org
 S:	Maintained
+F:	Documentation/switchtec.txt
 F:	drivers/pci/switch/switchtec*
 
 PCI DRIVER FOR NVIDIA TEGRA

2.1.4

This patch adds a few read-only sysfs attributes which provide some device information that is exposed from the devices. Primarily component and device names and versions. These are documented in Documentation/ABI/testing/sysfs-class-switchtec.

---
 Documentation/ABI/testing/sysfs-class-switchtec |  96 ++++++++++++++++++++
 MAINTAINERS                                     |   1 +
 drivers/pci/switch/switchtec.c                  | 113 ++++++++++++++++++++++++
 3 files changed, 210 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-switchtec

new file mode 100644
index 0000000..48cb4c1
@@ -0,0 +1,96 @@
+switchtec - Microsemi Switchtec PCI Switch Management Endpoint
+
+For details on this subsystem look at Documentation/switchtec.txt.
+
+What: 		/sys/class/switchtec
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	The switchtec class subsystem folder.
+		Each registered switchtec driver is represented by a switchtecX
+		subfolder (X being an integer >= 0).
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/component_id
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Component identifier as stored in the hardware (eg. PM8543)
+		(read only)
+Values: 	arbitrary string.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/component_revision
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Component revision stored in the hardware (read only)
+Values: 	integer.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/component_vendor
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Component vendor as stored in the hardware (eg. MICROSEM)
+		(read only)
+Values: 	arbitrary string.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/device_version
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Device version as stored in the hardware (read only)
+Values: 	integer.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/fw_version
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Currently running firmware version (read only)
+Values: 	integer (in hexadecimal).
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/partition
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Partition number for this device in the switch (read only)
+Values: 	integer.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/partition_count
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Total number of partitions in the switch (read only)
+Values: 	integer.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/product_id
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Product identifier as stored in the hardware (eg. PSX 48XG3)
+		(read only)
+Values: 	arbitrary string.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/product_revision
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Product revision stored in the hardware (eg. RevB)
+		(read only)
+Values: 	arbitrary string.
+
+
+What:		/sys/class/switchtec/switchtec[0-9]+/product_vendor
+Date:		05-Jan-2017
+KernelVersion:	v4.11
+Contact:	Logan Gunthorpe <logang at deltatee.com>
+Description:	Product vendor as stored in the hardware (eg. MICROSEM)
+		(read only)
+Values: 	arbitrary string.

index aa702b0..6fed938 100644
@@ -9513,6 +9513,7 @@ M:	Logan Gunthorpe <logang at deltatee.com>
 L:	linux-pci at vger.kernel.org
 S:	Maintained
 F:	Documentation/switchtec.txt
+F:	Documentation/ABI/testing/sysfs-class-switchtec
 F:	drivers/pci/switch/switchtec*
 
 PCI DRIVER FOR NVIDIA TEGRA

2.1.4

This patch introduces a couple of special IOCTLs which are provided to:

* Inform userspace of firmware partition locations
* Pass event counts and allow userspace to wait on events
* Translate between PFF numbers used by the switch to port numbers.

---
 Documentation/ioctl/ioctl-number.txt |   1 +
 Documentation/switchtec.txt          |  27 ++
 MAINTAINERS                          |   1 +
 drivers/pci/switch/switchtec.c       | 467 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/switchtec_ioctl.h | 132 ++++++++++
 5 files changed, 628 insertions(+)
 create mode 100644 include/uapi/linux/switchtec_ioctl.h

index 81c7f2b..032b33c 100644
@@ -191,6 +191,7 @@ Code  Seq#(hex)	Include File		Comments
 'W'	00-1F	linux/watchdog.h	conflict!
 'W'	00-1F	linux/wanrouter.h	conflict!		(pre 3.9)
 'W'	00-3F	sound/asound.h		conflict!
+'W'	40-5F   drivers/pci/switch/switchtec.c
 'X'	all	fs/xfs/xfs_fs.h		conflict!
 		and fs/xfs/linux-2.6/xfs_ioctl32.h
 		and include/linux/falloc.h

index 4bced4c..a0a9c7b 100644
@@ -51,3 +51,30 @@ The char device has the following semantics:
 
 * The poll call will also be supported for userspace applications that
   need to do other things while waiting for the command to complete.
+
+The following IOCTLs are also supported by the device:
+
+* SWITCHTEC_IOCTL_FLASH_INFO - Retrieve firmware length and number
+  of partitions in the device.
+
+* SWITCHTEC_IOCTL_FLASH_PART_INFO - Retrieve address and lengeth for
+  any specified partition in flash.
+
+* SWITCHTEC_IOCTL_EVENT_SUMMARY - Read a structure of bitmaps
+  indicating all uncleared events.
+
+* SWITCHTEC_IOCTL_EVENT_CTL - Get the current count, clear and set flags
+  for any event. This ioctl takes in a switchtec_ioctl_event_ctl struct
+  with the event_id, index and flags set (index being the partition or PFF
+  number for non-global events). It returns whether the event has
+  occurred, the number of times and any event specific data. The flags
+  can be used to clear the count or enable and disable actions to
+  happen when the event occurs.
+  By using the SWITCHTEC_IOCTL_EVENT_FLAG_EN_POLL flag,
+  you can set an event to trigger a poll command to return with
+  POLLPRI. In this way, userspace can wait for events to occur.
+
+* SWITCHTEC_IOCTL_PFF_TO_PORT and SWITCHTEC_IOCTL_PORT_TO_PFF convert
+  between PCI Function Framework number (used by the event system)
+  and Switchtec Logic Port ID and Partition number (which is more
+  user friendly).

index 6fed938..c1a9a30 100644
@@ -9515,6 +9515,7 @@ S:	Maintained
 F:	Documentation/switchtec.txt
 F:	Documentation/ABI/testing/sysfs-class-switchtec
 F:	drivers/pci/switch/switchtec*
+F:	include/uapi/linux/switchtec_ioctl.h
 
 PCI DRIVER FOR NVIDIA TEGRA
 M:	Thierry Reding <thierry.reding at gmail.com>

2.1.4

Hi Logan,

This driver doesn't have anything to do with the PCI core, other than using the pci_register_driver() interface (just like all other drivers for PCI-connected devices), so drivers/pci doesn't really feel like the right place for it. Putting it in drivers/pci leads to the sort of confusion you mentioned above ("To make this entirely clear ...").

Would drivers/perf or drivers/misc/switchtec/ be possible places for it?

Bjorn

I made a similar argument when we made the decision of where to put the code. In the end, the device _is_ a PCI Switch and someone going through menuconfig or the source tree would probably look there first.

As for drivers/perf, our device does a fair bit more than performance counters so it doesn't seem like it really fits in there. drivers/misc just seems like a dumping ground which we'd prefer not to contribute to. We also considered drivers/char (seeing it exposes a char device), but that also seems like a dumping ground with stuff that belongs and other stuff that just ended up stuck between the cracks.

If you still feel strongly about this we can move it into misc, but I think from an organizational perspective pci/switch makes a bit more sense.

In any case, I also wish we could have had this discussion 3 months ago when we posted the RFC and not when I have people pushing to get this merged.

Thanks,

Logan

Relax, there is no rush here, no deadlines, etc. Moving the files to a different directly takes all of 5 minutes, max.

mellow out dude,

greg k-h

It's not a perfect fit in drivers/pci because it's not bus infrastructure and I don't want to be the default maintainer of it, but I agree there's not really an alternative that's clearly better, so let's leave it where it is for now.

You and me both! I hope some of those same people are pushing to help avoid the tragedy of the commons by helping review other contributions.

Bjorn

Sounds good, thanks.

It would be nice if it could stay where it is for organization but go through other maintainers trees (though I'm not sure who). I understand why you wouldn't want to take on the maintenance of it. Hopefully, myself and the other Microsemi developers will be able to do the bulk of the maintenance work for the driver.

Thanks,

Logan

This is in free_msi_irqs():

  368    for_each_pci_msi_entry(entry, dev)
  369            if (entry->irq)
  370                    for (i = 0; i < entry->nvec_used; i++)
  371                            BUG_ON(irq_has_action(entry->irq + i));

I don't think this is indicating a bug in the PCI core (although I do think a BUG_ON() here is an excessive response). I think it's an indication that the driver didn't disconnect its ISR. Without more details of the failure it's hard to tell if the BUG_ON is a symptom of a problem in the driver or what.

An "alive" flag feels racy, and I can't tell if it's really the best way to deal with this, or if it's just avoiding the issue. There must be other drivers with the same cleanup issue -- do they handle it the same way?

Yes, my assumption was that when you force an unbind on the PCI core, it's designed to stop using the PCI device right away even if there are users using it. Thus it becomes the drivers responsibility to handle this situation.

I haven't done a comprehensive search, but it's very common for people to use (and this is what I've adopted again in v5):

devm_request_irq(&pdev->dev, ...)

In this way, the IRQs are released with the pci_dev (or often platform) and thus the BUG_ON never hits. However, it means any user space program waiting on an IRQ (like via a cdev call) will hang unless handled with other means. Exactly what those means are seems driver specific and not always obvious. I wouldn't be surprised if a lot of drivers get this aspect wrong.

A couple examples I've looked at:

1) drivers/dax/dax.c uses an alive flag without any mutexes, atomics or anything. So I don't know if it's racy or perhaps correct for other reasons.

2) drivers/char/hw_random has a drop_current_rng that looks like it could easily be racy with the get_current_rng in the userspace flow.

3) A couple of drivers drivers/char/tpm doesn't seem to have any protection at all and appears like they would continue to use io operations even after the they may get unmapped because the char device persists.

So I'm not sure where you'd find a driver that does it correctly and in a simpler way..

Another thing: based on comments in [1], a lot of people don't seem to realize that cdev instances can persist long after cdev_del so it's probably very common for drivers to get this wrong.

Logan

[1] https://lists.01.org/pipermail/linux-nvdimm/2017-February/009001.html

Hey,

Seems to me like an elegant solution would be to implement a 'cdev_kill' function which could kill all the processes using a cdev. Thus, during an unbind, a driver could call it and be sure that there are no users left and it can safely allow the devres unwind to continue. Then no difficult and racy 'alive' flags would be necessary and it would be much easier on drivers.

However, I don't think any such thing exists at the moment and it's not likely to be done in the near term. I'm reasonably confident in the correctness of v5 of my driver (especially when compared to other drivers) and unless someone can describe how it's wrong or a better solution I'd rather see it merged as is. If and when a better approach arrives I'd happily patch it to improve the situation.

Logan

That could help, but this would mean cdev would have to insert a shim to grab locks around the various file ops.

AFAIK TPM is correct and has been robustly tested now. We have a 'vtpm' driver that agressively uses hot-unplug.

Firstly, TPM correctly tracks krefs on the 'chip', so the pointer is always valid.

Next, anytime someone has a 'chip' pointer and wants to execute a TPM operation they have to call tpm_try_get_ops and hold that lock for the duration of their usage. Internally it does this:

        down_read(&chip->ops_sem);
        if (!chip->ops)
                goto out_lock; // FAILURE

In the cdev fops case this will cause the fop to return EPIPE if it fails. cdev holds this read side lock while it is running TPM commands inside the driver.

This meshes with this code called by tpm_chip_unregister():

        down_write(&chip->ops_sem);
        chip->ops = NULL;
        up_write(&chip->ops_sem);

tpm_chip_unregister uses this to provide a strong fence guarentee to the driver. After tpm_chip_unregister return:
- Nothing is running in a TPM ops callback currently
- Nothing will ever call a TPM ops callback in future

This allows the TPM drivers to be trivial: call tpm_chip_unregister(), kill the IRQ, and unmap the io resource, unload the module code.

TPM drivers cannot associate HW resources to the 'chip' struct device, as it is unwound and devm triggered *during* tpm_chip_unregister and does not enjoy the strong fence guarantee.

infiniband has a similar 'strong fence' unregister function. I think it is best-practice for subsystem design to provide that because drivers will never get it right on their own :(

Both infiniband and TPM have the 'detach' flavour of unplug where the user is not forced to close their open cdevs. For simpler cases you can just wait for all cdevs to close with a simple rwsem, much like sysfs does already during device_del.

Jason

Hmm, I was hoping something more along the lines of actually killing the processes instead of just shimming away fops.

Ah, thanks for the explanation of how that works. I didn't notice the semaphore usage.

Switchtec is a bit more tricky because a) there's no upper level driver to handle things and b) userspace may be inside a wait_for_completion (via read or poll) that needs to be completed. If a so called 'cdev_kill' could actually just kill these processes it would be a bit easier.

Currently, in the Switchtec code, there's a timeout if the interrupt doesn't fire (which occurs if the pci device has been torn down) and the code will check an alive bit (under mutex protection) and error out if it's not alive.

Because of how poll works, I don't see how I can just hold a semaphore inside every fops call like the tpm code does.

Logan

Though, after reading your email again, a hard fence on close sounds like a good idea. Tomorrow I'll post a v6 that uses that approach.

Thanks,

Logan

That would probably make most cdev users unhappy, it is not what we want in tpm or infiniband, for instance.

Introducing a light split between 'the upper part that owns the cdev' and 'the lower part that owns the hardware' makes things much easier to understand in a driver and it becomes clearer where, eg, devm actions should be linked (ie probably not to the cdev part)

For TPM, poll could be something like:

static unsigned int tpm_poll(struct file *filp,
struct poll_table_struct *wait)
{
poll_wait(filp, &chip->poll_wait, wait);

   if (tpm_try_get_ops(chip)) {
          mask = chip->ops->driver_do_poll(...);
          tpm_put_ops(chip);
   } else 
          mask = POLLIN | POLLRDHUP | POLLOUT | POLLERR | POLLHUP;

return mask;
}

And we would trigger chip->poll_wait in the unregister.

wait_for_completion is similar, drop the rwsem while sleeping, add 'ops = NULL' to the sleeping condition test, trigger the wait on unregister then reacquire the rwsem and test ops on wake.

Jason

I think this is from using the managed device resource API to request the irq actions. The scope of the resource used to be tied to the pci_dev's dev, but now it's the new switchec class dev, which has a different lifetime while open references exist, so it's not releasing the irq's.

One thing about the BUG_ON that is confusing me is how it's getting to free_msi_irq's BUG in v4 or v5. I don't see any part releasing the allocated ones. Maybe the devres API is harder to use than having the driver manage all the resources...

The scope of the IRQ was originally tied to the pci_dev. Then in v4 I tied it to the switchtec device in order to try and keep using the pci device after unbind. This didn't work, so I switched it back to using the pci_dev. (This seems to be the way most drivers work anyway.)

free_msi_irqs seems to be called via pci_disable_device in pcim_release which devres will call during release of the PCI device and before all the references to the pci_dev are freed (I tried adding an extra get_device which gets put in the child devices release -- this didn't work):

 [ 1079.845616] Call Trace:
 [ 1079.845652]  ? pcim_release+0x35/0x96
 [ 1079.845691]  ? release_nodes+0x15b/0x17c
 [ 1079.845730]  ? device_release_driver_internal+0x12d/0x1cb
 [ 1079.845771]  ? unbind_store+0x59/0x89
 [ 1079.845809]  ? kernfs_fop_write+0xe7/0x129
 [ 1079.845847]  ? __vfs_write+0x1c/0xa2
 [ 1079.845885]  ? kmem_cache_alloc+0xc5/0x131
 [ 1079.845923]  ? fput+0xd/0x7d
 [ 1079.845958]  ? filp_close+0x5a/0x61
 [ 1079.845993]  ? vfs_write+0xa2/0xe4
 [ 1079.846028]  ? SyS_write+0x48/0x73
 [ 1079.846066]  ? entry_SYSCALL_64_fastpath+0x13/0x94

v5 is correct because it registers the irqs against the pci_dev (with devm_request_irq) and thus they get freed in time as part of the devres unwind.

Logan

Okay, I see. Was mistakenliy looking at v4. The v5 looks right.

Great! Thanks for reviewing that for me.

Logan