qat: rework driver binding

The new_id based driver binding is failing on kernels 5.11+ when the
QAT VF is not bound to any driver: attempts to write to new_id with
the same device ID repeatedly error with "file exists".

Move the new_id initialization to the beginning of the startup and
write the enabled device IDs only once.

This commit also fixes an issue where VF devices where not correctly detected
in virtual machines where the VF was not bound any driver.

Signed-off-by: Mikko Ylinen <mikko.ylinen@intel.com>

cmd/qat_plugin/dpdkdrv/dpdkdrv.go

@@ -41,13 +41,14 @@ const (
 	pciDriverDirectory = "/sys/bus/pci/drivers"
 	uioSuffix          = "uio"
 	iommuGroupSuffix   = "iommu_group"
-	newIDSuffix        = "new_id"
-	driverUnbindSuffix = "driver/unbind"
 	vendorPrefix       = "8086 "
 	envVarPrefix       = "QAT"
 
 	igbUio  = "igb_uio"
 	vfioPci = "vfio-pci"
+
+	// Period of device scans.
+	scanPeriod = 5 * time.Second
 )
 
 // QAT PCI VF Device ID -> kernel QAT VF device driver mappings.
@@ -63,6 +64,9 @@ var qatDeviceDriver = map[string]string{
 
 // DevicePlugin represents vfio based QAT plugin.
 type DevicePlugin struct {
+	scanTicker *time.Ticker
+	scanDone   chan bool
+
 	pciDriverDir    string
 	pciDeviceDir    string
 	dpdkDriver      string
@@ -93,11 +97,36 @@ func newDevicePlugin(pciDriverDir, pciDeviceDir string, maxDevices int, kernelVf
 		pciDeviceDir:    pciDeviceDir,
 		kernelVfDrivers: kernelVfDrivers,
 		dpdkDriver:      dpdkDriver,
+		scanTicker:      time.NewTicker(scanPeriod),
+		scanDone:        make(chan bool, 1),
 	}
 }
 
+func (dp *DevicePlugin) setupDeviceIDs() error {
+	for devID, driver := range qatDeviceDriver {
+		for _, enabledDriver := range dp.kernelVfDrivers {
+			if driver != enabledDriver {
+				continue
+			}
+
+			err := writeToDriver(filepath.Join(dp.pciDriverDir, dp.dpdkDriver, "new_id"), vendorPrefix+devID)
+			if err != nil && !errors.Is(err, os.ErrExist) {
+				return errors.WithMessagef(err, "failed to set device ID %s for %s. Driver module not loaded?", devID, dp.dpdkDriver)
+			}
+		}
+	}
+
+	return nil
+}
+
 // Scan implements Scanner interface for vfio based QAT plugin.
 func (dp *DevicePlugin) Scan(notifier dpapi.Notifier) error {
+	defer dp.scanTicker.Stop()
+
+	if err := dp.setupDeviceIDs(); err != nil {
+		return err
+	}
+
 	for {
 		devTree, err := dp.scan()
 		if err != nil {
@@ -106,7 +135,11 @@ func (dp *DevicePlugin) Scan(notifier dpapi.Notifier) error {
 
 		notifier.Notify(devTree)
 
-		time.Sleep(5 * time.Second)
+		select {
+		case <-dp.scanDone:
+			return nil
+		case <-dp.scanTicker.C:
+		}
 	}
 }
 
@@ -197,34 +230,18 @@ func (dp *DevicePlugin) getDpdkMounts(dpdkDeviceName string) []pluginapi.Mount {
 	}
 }
 
-func (dp *DevicePlugin) getDeviceID(pciAddr string) (string, error) {
-	devID, err := os.ReadFile(filepath.Join(dp.pciDeviceDir, filepath.Clean(pciAddr), "device"))
+func getDeviceID(device string) (string, error) {
+	devID, err := os.ReadFile(filepath.Join(device, "device"))
 	if err != nil {
-		return "", errors.Wrapf(err, "Cannot obtain ID for the device %s", pciAddr)
+		return "", errors.Wrapf(err, "failed to read device ID")
 	}
 
 	return strings.TrimPrefix(string(bytes.TrimSpace(devID)), "0x"), nil
 }
 
-// bindDevice unbinds given device from kernel driver and binds to DPDK driver.
-func (dp *DevicePlugin) bindDevice(vfBdf string) error {
-	unbindDevicePath := filepath.Join(dp.pciDeviceDir, vfBdf, driverUnbindSuffix)
-
-	// Unbind from the kernel driver. IsNotExist means the device is not bound to any driver.
-	if err := os.WriteFile(unbindDevicePath, []byte(vfBdf), 0600); !os.IsNotExist(err) {
-		return errors.Wrapf(err, "Unbinding from kernel driver failed for the device %s", vfBdf)
-	}
-
-	vfdevID, err := dp.getDeviceID(vfBdf)
-	if err != nil {
-		return err
-	}
-
-	bindDevicePath := filepath.Join(dp.pciDriverDir, dp.dpdkDriver, newIDSuffix)
-	//Bind to the the dpdk driver
-	err = os.WriteFile(bindDevicePath, []byte(vendorPrefix+vfdevID), 0600)
-	if err != nil {
-		return errors.Wrapf(err, "Binding to the DPDK driver failed for the device %s", vfBdf)
+func writeToDriver(path, value string) error {
+	if err := os.WriteFile(path, []byte(value), 0600); err != nil {
+		return errors.Wrapf(err, "write to driver failed: %s", value)
 	}
 
 	return nil
@@ -307,27 +324,43 @@ func (dp *DevicePlugin) getVfDevices() []string {
 	qatPfDevices := make([]string, 0)
 	qatVfDevices := make([]string, 0)
 
-	// Get PF BDFs bound to a PF driver
+	// Get PF BDFs bound to a known QAT PF driver
 	for _, vfDriver := range dp.kernelVfDrivers {
 		pfDriver := strings.TrimSuffix(vfDriver, "vf")
 		pattern := filepath.Join(dp.pciDriverDir, pfDriver, "????:??:??.?")
 		qatPfDevices = append(qatPfDevices, getPciDevicesWithPattern(pattern)...)
 	}
 
-	// Get VF devices belonging to a PF device
+	// Get VF devices belonging to a valid QAT PF device
 	for _, qatPfDevice := range qatPfDevices {
 		pattern := filepath.Join(qatPfDevice, "virtfn*")
 		qatVfDevices = append(qatVfDevices, getPciDevicesWithPattern(pattern)...)
 	}
 
-	if len(qatVfDevices) > 0 {
+	if len(qatPfDevices) > 0 {
+		if len(qatVfDevices) >= dp.maxDevices {
+			return qatVfDevices[:dp.maxDevices]
+		}
+
 		return qatVfDevices
 	}
 
-	// No PF devices found, running in a VM?
-	for _, vfDriver := range append([]string{dp.dpdkDriver}, dp.kernelVfDrivers...) {
-		pattern := filepath.Join(dp.pciDriverDir, vfDriver, "????:??:??.?")
-		qatVfDevices = append(qatVfDevices, getPciDevicesWithPattern(pattern)...)
+	// No PF devices with a QAT driver found, running in a VM?
+	pattern := filepath.Join(dp.pciDeviceDir, "????:??:??.?")
+	for _, pciDev := range getPciDevicesWithPattern(pattern) {
+		devID, err := getDeviceID(pciDev)
+		if err != nil {
+			klog.Warningf("unable to read device id for device %s: %q", filepath.Base(pciDev), err)
+			continue
+		}
+
+		if dp.isValidVfDeviceID(devID) {
+			qatVfDevices = append(qatVfDevices, pciDev)
+		}
+	}
+
+	if len(qatVfDevices) >= dp.maxDevices {
+		return qatVfDevices[:dp.maxDevices]
 	}
 
 	return qatVfDevices
@@ -352,22 +385,15 @@ func (dp *DevicePlugin) scan() (dpapi.DeviceTree, error) {
 	for _, vfDevice := range dp.getVfDevices() {
 		vfBdf := filepath.Base(vfDevice)
 
-		vfdevID, err := dp.getDeviceID(vfBdf)
+		if drv := getCurrentDriver(vfDevice); drv != dp.dpdkDriver {
+			if drv != "" {
+				err := writeToDriver(filepath.Join(dp.pciDriverDir, drv, "unbind"), vfBdf)
 				if err != nil {
 					return nil, err
 				}
-
-		if !dp.isValidVfDeviceID(vfdevID) {
-			continue
 			}
 
-		n = n + 1
-		if n > dp.maxDevices {
-			break
-		}
-
-		if getCurrentDriver(vfDevice) != dp.dpdkDriver {
-			err = dp.bindDevice(vfBdf)
+			err := writeToDriver(filepath.Join(dp.pciDriverDir, dp.dpdkDriver, "bind"), vfBdf)
 			if err != nil {
 				return nil, err
 			}
@@ -380,6 +406,7 @@ func (dp *DevicePlugin) scan() (dpapi.DeviceTree, error) {
 
 		klog.V(1).Infof("Device %s found", vfBdf)
 
+		n = n + 1
 		envs := map[string]string{
 			fmt.Sprintf("%s%d", envVarPrefix, n): vfBdf,
 		}

cmd/qat_plugin/dpdkdrv/dpdkdrv_test.go

@@ -23,6 +23,8 @@ import (
 	"github.com/pkg/errors"
 
 	pluginapi "k8s.io/kubelet/pkg/apis/deviceplugin/v1beta1"
+
+	dpapi "github.com/intel/intel-device-plugins-for-kubernetes/pkg/deviceplugin"
 )
 
 func init() {
@@ -105,7 +107,19 @@ func TestNewDevicePlugin(t *testing.T) {
 	}
 }
 
-func TestScanPrivate(t *testing.T) {
+// fakeNotifier implements Notifier interface.
+type fakeNotifier struct {
+	scanDone chan bool
+	tree     dpapi.DeviceTree
+}
+
+// Notify stops plugin Scan.
+func (n *fakeNotifier) Notify(newDeviceTree dpapi.DeviceTree) {
+	n.tree = newDeviceTree
+	n.scanDone <- true
+}
+
+func TestScan(t *testing.T) {
 	tcases := []struct {
 		name            string
 		dpdkDriver      string
@@ -124,33 +138,26 @@ func TestScanPrivate(t *testing.T) {
 			name:       "igb_uio DPDKdriver with one DPDK bound device where vfdevID cannot be determined",
 			dpdkDriver: "igb_uio",
 			dirs: []string{
-				"sys/bus/pci/drivers/c6xx",
 				"sys/bus/pci/drivers/igb_uio/0000:02:01.0",
 				"sys/bus/pci/devices/0000:02:01.0/uio/sometestfile",
 				"sys/bus/pci/devices/0000:02:00.0",
 			},
-			symlinks: map[string]string{
-				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
-				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
-			},
 			maxDevNum:      1,
-			expectedErr: true,
+			expectedDevNum: 0,
 		},
 		{
-			name:            "igb_uio DPDKdriver with one kernel bound device, but unbindable",
+			name:            "igb_uio DPDKdriver with one kernel bound device, but binding fails",
 			dpdkDriver:      "igb_uio",
 			kernelVfDrivers: []string{"c6xxvf"},
 			dirs: []string{
-				"sys/bus/pci/drivers/c6xx",
-				"sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/devices/0000:02:01.0",
 			},
 			files: map[string][]byte{
 				"sys/bus/pci/devices/0000:02:01.0/device": []byte("0x37c9"),
 			},
 			symlinks: map[string]string{
-				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
-				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver": "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:   1,
 			expectedErr: true,
@@ -161,17 +168,18 @@ func TestScanPrivate(t *testing.T) {
 			kernelVfDrivers: []string{"c6xxvf"},
 			dirs: []string{
 				"sys/bus/pci/drivers/c6xx",
+				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/drivers/igb_uio",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
 				"sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/devices/0000:02:01.0",
 			},
 			files: map[string][]byte{
-				"sys/bus/pci/devices/0000:02:01.0/driver/unbind": []byte("some junk"),
 				"sys/bus/pci/devices/0000:02:01.0/device": []byte("0x37c9"),
 			},
 			symlinks: map[string]string{
 				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver":  "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:   1,
 			expectedErr: true,
@@ -182,9 +190,9 @@ func TestScanPrivate(t *testing.T) {
 			kernelVfDrivers: []string{"c6xxvf"},
 			dirs: []string{
 				"sys/bus/pci/drivers/c6xx",
+				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/drivers/igb_uio",
 				"sys/bus/pci/devices/0000:02:01.0/uio",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
 				"sys/bus/pci/devices/0000:02:00.0",
 			},
 			files: map[string][]byte{
@@ -193,6 +201,7 @@ func TestScanPrivate(t *testing.T) {
 			symlinks: map[string]string{
 				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver":  "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:   1,
 			expectedErr: true,
@@ -203,11 +212,10 @@ func TestScanPrivate(t *testing.T) {
 			kernelVfDrivers: []string{"c6xxvf"},
 			dirs: []string{
 				"sys/bus/pci/drivers/c6xx",
+				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/drivers/igb_uio",
 				"sys/bus/pci/devices/0000:02:01.0/uio/sometestfile",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
 				"sys/bus/pci/devices/0000:02:01.1/uio/sometestfile",
-				"sys/bus/pci/devices/0000:02:01.1/driver",
 				"sys/bus/pci/devices/0000:02:00.0",
 			},
 			files: map[string][]byte{
@@ -218,19 +226,48 @@ func TestScanPrivate(t *testing.T) {
 				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn1": "sys/bus/pci/devices/0000:02:01.1",
+				"sys/bus/pci/devices/0000:02:01.0/driver":  "sys/bus/pci/drivers/c6xxvf",
+				"sys/bus/pci/devices/0000:02:01.1/driver":  "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:      1,
 			expectedDevNum: 1,
 		},
+		{
+			name:            "igb_uio DPDKdriver with one kernel bound device (QAT device) where vfdevID is equal to qatDevId (37c9) where the available devices on the system are 2 but maxNumDevices=4] ",
+			dpdkDriver:      "igb_uio",
+			kernelVfDrivers: []string{"c6xxvf"},
+			dirs: []string{
+				"sys/bus/pci/drivers/c6xx",
+				"sys/bus/pci/drivers/c6xxvf",
+				"sys/bus/pci/drivers/igb_uio",
+				"sys/bus/pci/devices/0000:02:01.0/uio/sometestfile",
+				"sys/bus/pci/devices/0000:02:01.1/uio/sometestfile",
+				"sys/bus/pci/devices/0000:02:00.0",
+			},
+			files: map[string][]byte{
+				"sys/bus/pci/devices/0000:02:01.0/device": []byte("0x37c9"),
+				"sys/bus/pci/devices/0000:02:01.1/device": []byte("0x37c9"),
+			},
+			symlinks: map[string]string{
+				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:00.0/virtfn1": "sys/bus/pci/devices/0000:02:01.1",
+				"sys/bus/pci/devices/0000:02:01.0/driver":  "sys/bus/pci/drivers/c6xxvf",
+				"sys/bus/pci/devices/0000:02:01.1/driver":  "sys/bus/pci/drivers/c6xxvf",
+			},
+			maxDevNum:      4,
+			expectedDevNum: 2,
+		},
 		{
 			name:            "vfio-pci DPDKdriver with one kernel bound device (QAT device) where vfdevID is equal to qatDevId (37c9)",
 			dpdkDriver:      "vfio-pci",
 			kernelVfDrivers: []string{"c6xxvf"},
 			dirs: []string{
 				"sys/bus/pci/drivers/c6xx",
+				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/drivers/vfio-pci",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
 				"sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/devices/0000:02:01.0",
 			},
 			files: map[string][]byte{
 				"sys/bus/pci/devices/0000:02:01.0/device": []byte("0x37c9"),
@@ -239,6 +276,7 @@ func TestScanPrivate(t *testing.T) {
 				"sys/bus/pci/devices/0000:02:01.0/iommu_group": "sys/kernel/iommu_groups/vfiotestfile",
 				"sys/bus/pci/drivers/c6xx/0000:02:00.0":        "sys/bus/pci/devices/0000:02:00.0",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn0":     "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver":      "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:      1,
 			expectedDevNum: 1,
@@ -269,17 +307,19 @@ func TestScanPrivate(t *testing.T) {
 			dpdkDriver:      "vfio-pci",
 			kernelVfDrivers: []string{"c6xxvf"},
 			dirs: []string{
-				"sys/bus/pci/drivers/c6xx",
+				"sys/bus/pci/drivers/d15xx",
+				"sys/bus/pci/drivers/d15xxvf",
 				"sys/bus/pci/drivers/vfio-pci",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
 				"sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/devices/0000:02:01.0",
 			},
 			files: map[string][]byte{
 				"sys/bus/pci/devices/0000:02:01.0/device": []byte("0x6f55"),
 			},
 			symlinks: map[string]string{
 				"sys/bus/pci/devices/0000:02:01.0/iommu_group": "sys/kernel/iommu_groups/vfiotestfile",
-				"sys/bus/pci/drivers/c6xx/0000:02:00.0":        "sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/drivers/d15xx/0000:02:00.0":       "sys/bus/pci/devices/0000:02:00.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver":      "sys/bus/pci/drivers/d15xxvf",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn0":     "sys/bus/pci/devices/0000:02:01.0",
 			},
 			maxDevNum:      1,
@@ -292,7 +332,7 @@ func TestScanPrivate(t *testing.T) {
 			dirs: []string{
 				"sys/bus/pci/drivers/vfio-pci",
 				"sys/bus/pci/drivers/c6xx",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
+				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/devices/0000:02:00.0",
 				"sys/bus/pci/devices/0000:02:01.0/vfio-pci/vfiotestfile",
 			},
@@ -302,6 +342,7 @@ func TestScanPrivate(t *testing.T) {
 			symlinks: map[string]string{
 				"sys/bus/pci/drivers/c6xx/0000:02:00.0":    "sys/bus/pci/devices/0000:02:00.0",
 				"sys/bus/pci/devices/0000:02:00.0/virtfn0": "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver":  "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:   1,
 			expectedErr: true,
@@ -313,14 +354,14 @@ func TestScanPrivate(t *testing.T) {
 			dirs: []string{
 				"sys/bus/pci/drivers/c6xxvf",
 				"sys/bus/pci/drivers/vfio-pci",
-				"sys/bus/pci/devices/0000:02:01.0/driver",
+				"sys/bus/pci/devices/0000:02:01.0",
 			},
 			files: map[string][]byte{
 				"sys/bus/pci/devices/0000:02:01.0/device": []byte("0x37c9"),
 			},
 			symlinks: map[string]string{
 				"sys/bus/pci/devices/0000:02:01.0/iommu_group": "sys/kernel/iommu_groups/vfiotestfile",
-				"sys/bus/pci/drivers/c6xxvf/0000:02:01.0":      "sys/bus/pci/devices/0000:02:01.0",
+				"sys/bus/pci/devices/0000:02:01.0/driver":      "sys/bus/pci/drivers/c6xxvf",
 			},
 			maxDevNum:      1,
 			expectedDevNum: 1,
@@ -337,23 +378,28 @@ func TestScanPrivate(t *testing.T) {
 				t.Fatalf("%+v", err)
 			}
 
-			dp := &DevicePlugin{
-				maxDevices:      tt.maxDevNum,
-				pciDriverDir:    path.Join(tmpdir, "sys/bus/pci/drivers"),
-				pciDeviceDir:    path.Join(tmpdir, "sys/bus/pci/devices"),
-				dpdkDriver:      tt.dpdkDriver,
-				kernelVfDrivers: tt.kernelVfDrivers,
+			dp := newDevicePlugin(
+				path.Join(tmpdir, "sys/bus/pci/drivers"),
+				path.Join(tmpdir, "sys/bus/pci/devices"),
+				tt.maxDevNum,
+				tt.kernelVfDrivers,
+				tt.dpdkDriver,
+			)
+
+			fN := fakeNotifier{
+				scanDone: dp.scanDone,
 			}
 
-			tree, err := dp.scan()
+			err = dp.Scan(&fN)
+
 			if tt.expectedErr && err == nil {
 				t.Errorf("expected error, but got success")
 			}
 			if !tt.expectedErr && err != nil {
 				t.Errorf("got unexpected error: %+v", err)
 			}
-			if len(tree) != tt.expectedDevNum {
-				t.Errorf("expected %d, but got %d devices", tt.expectedDevNum, len(tree))
+			if len(fN.tree["generic"]) != tt.expectedDevNum {
+				t.Errorf("expected %d, but got %d devices", tt.expectedDevNum, len(fN.tree["generic"]))
 			}
 
 			if err = os.RemoveAll(tmpdir); err != nil {