fpga: altera-cvp: fix probing for multiple FPGAs on the bus

PATCH Superseded Anatolij Gustschin 2 participants 6 messages Nov 6, 2018 – Nov 7, 2018 FPGA manager framework

Share & tools

Anatolij Gustschin

author

2018-11-07 06:02 UTC #0 lore raw

PATCH 1/1 fpga: altera-cvp: fix probing for multiple FPGAs on the bus

Currently registering CvP managers works only for first probed CvP device, for all other devices it is refused due to duplicated chkcfg sysfs entry:

fpga_manager fpga3: Altera CvP FPGA Manager @0000:0c:00.0 registered
sysfs: cannot create duplicate filename '/bus/pci/drivers/altera-cvp/chkcfg'

CPU: 0 PID: 3808 Comm: bash Tainted: G           O      4.19.0-custom+ #5
Call Trace:
  dump_stack+0x46/0x5b
  sysfs_warn_dup+0x53/0x60
  sysfs_add_file_mode_ns+0x16d/0x180
  sysfs_create_file_ns+0x51/0x60
  altera_cvp_probe+0x16f/0x2a0 [altera_cvp]
  local_pci_probe+0x3f/0xa0
  ? pci_match_device+0xb1/0xf0
  pci_device_probe+0x116/0x170
  really_probe+0x21b/0x2c0
  driver_probe_device+0x4b/0xe0
  bind_store+0xcb/0x130
  kernfs_fop_write+0xfd/0x180
  __vfs_write+0x21/0x150
  ? selinux_file_permission+0xdc/0x130
  vfs_write+0xa8/0x1a0
  ? find_vma+0xd/0x60
  ksys_write+0x3d/0x90
  do_syscall_64+0x44/0xf0
  entry_SYSCALL_64_after_hwframe+0x44/0xa9
  ...
 altera-cvp 0000:0c:00.0: Can't create sysfs chkcfg file
 fpga_manager fpga3: fpga_mgr_unregister Altera CvP FPGA Manager @0000:0c:00.0

Skip chkcfg creation for all but first probed device. Remove chkcfg interface when last potential user has gone.

Signed-off-by Anatolij Gustschin <[email protected]>

---
 drivers/fpga/altera-cvp.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

drivers/fpga/altera-cvp.c +15 -7

index 144fa2a4d4cc..381d0c42450f 100644
@@ -60,6 +60,7 @@
 
 /* Optional CvP config error status check for debugging */
 static bool altera_cvp_chkcfg;
+static unsigned int altera_cvp_cnt;
 
 struct altera_cvp_conf {
 	struct fpga_manager	*mgr;

@@ -466,12 +467,15 @@ static int altera_cvp_probe(struct pci_dev *pdev,
 	if (ret)
 		goto err_unmap;
 
-	ret = driver_create_file(&altera_cvp_driver.driver,
-				 &driver_attr_chkcfg);
-	if (ret) {
-		dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
-		fpga_mgr_unregister(mgr);
-		goto err_unmap;
+	if (!altera_cvp_cnt++) {
+		ret = driver_create_file(&altera_cvp_driver.driver,
+					 &driver_attr_chkcfg);
+		if (ret) {
+			dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
+			fpga_mgr_unregister(mgr);
+			altera_cvp_cnt--;
+			goto err_unmap;
+		}
 	}
 
 	return 0;

@@ -492,7 +496,11 @@ static void altera_cvp_remove(struct pci_dev *pdev)
 	struct altera_cvp_conf *conf = mgr->priv;
 	u16 cmd;
 
-	driver_remove_file(&altera_cvp_driver.driver, &driver_attr_chkcfg);
+	if (!--altera_cvp_cnt) {
+		driver_remove_file(&altera_cvp_driver.driver,
+				   &driver_attr_chkcfg);
+	}
+
 	fpga_mgr_unregister(mgr);
 	if (conf->map)
 		pci_iounmap(pdev, conf->map);

Signature · 1 line

2.17.1

Anatolij Gustschin

author

2018-11-06 23:56 UTC #2 lore raw

Hi Alan,

On Tue, 6 Nov 2018 15:54:17 -0600
Alan Tull [email protected] wrote:
...

Quoted 5 lines

Quoted 2 lines
fpga_manager fpga3: Altera CvP FPGA Manager @0000:0c:00.0 registered
sysfs: cannot create duplicate filename '/bus/pci/drivers/altera-cvp/chkcfg'
Shouldn't this have been a file under /sys/class/fpga_manager/fpga0/ instead? This is a control per-device not per driver.

I thought /sys/class/fpga_manager/fpgaN/ interface is generic for fpga-mgr and low-level manager specific stuff does not belong there. At least this is my filling when reading Documentation/ABI/testing/sysfs-class-fpga-manager.

chkcfg is a debugging option and will be rarely used while development, it is off by default. And when enabling it globally at debugging time, it won't hurt other devices I think. If it were some device specific behaviour control, then it surely would make sense to turn it on/off pre-device.

...

Quoted 10 lines

drivers/fpga/altera-cvp.c static int altera_cvp_probe(struct pci_dev *pdev, · line 472

Quoted 9 lines

drivers/fpga/altera-cvp.c +3 -6

-       ret = driver_create_file(&altera_cvp_driver.driver,
-                                &driver_attr_chkcfg);
-       if (ret) {
-               dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
-               fpga_mgr_unregister(mgr);
-               goto err_unmap;
+       if (!altera_cvp_cnt++) {
+               ret = driver_create_file(&altera_cvp_driver.driver,
+                                        &driver_attr_chkcfg);

Quoted 2 lines

In the review, I didn't catch that this was adding a driver file instead of a device file.

I was too focused on tons of comments to the driver patches when mainlining this driver and didn't have hardware with multiple PCIe cards to test it, so this bug crept in.

When adding it as a device file, it will be in the directory with many PCI device specific files, e.g:

# ls /sys/bus/pci/devices/0000\:0c\:00.0/
ari_enabled           config                    current_link_width  dma_mask_bits    enable        local_cpulist   max_link_width  numa_node  rescan    resource0  resource4     subsystem         uevent
broken_parity_status  consistent_dma_mask_bits  d3cold_allowed      driver           fpga_manager  local_cpus      modalias        power      reset     resource1  resource4_wc  subsystem_device  vendor
class                 current_link_speed        device              driver_override  irq           max_link_speed  msi_bus         remove     resource  resource2  revision      subsystem_vendor

I don't think that it is a good place for such driver specific control file. If we really must implement it per-device, then it would make more sense to use the current path and use something like

echo "1 0000:0c:00.0" > /sys/bus/pci/drivers/altera-cvp/chkcfg

for enabling the option for a particular device. For disabling:

echo "0 0000:0c:00.0" > /sys/bus/pci/drivers/altera-cvp/chkcfg

But it is harder to implement. Is it worth the effort when it is hardly used?

Thanks,
Anatolij

Alan Tull

2018-11-07 15:53 UTC #3 lore raw

Quoted 9 lines · Anatolij Gustschin

Hi Alan,
On Tue, 6 Nov 2018 15:54:17 -0600
Alan Tull [email protected] wrote:
...
Quoted 3 lines
Quoted 2 lines
fpga_manager fpga3: Altera CvP FPGA Manager @0000:0c:00.0 registered
sysfs: cannot create duplicate filename '/bus/pci/drivers/altera-cvp/chkcfg'

Quoted 6 lines

Quoted 2 lines
Shouldn't this have been a file under /sys/class/fpga_manager/fpga0/ instead? This is a control per-device not per driver.
I thought /sys/class/fpga_manager/fpgaN/ interface is generic for fpga-mgr and low-level manager specific stuff does not belong there. At least this is my filling when reading Documentation/ABI/testing/sysfs-class-fpga-manager.

Yes

Quoted 4 lines

chkcfg is a debugging option and will be rarely used while development, it is off by default. And when enabling it globally at debugging time, it won't hurt other devices I think.

OK that's good to know.

Quoted 33 lines

If it were some device specific
behaviour control, then it surely would make sense to turn it on/off pre-device.
...
Quoted 10 lines
drivers/fpga/altera-cvp.c static int altera_cvp_probe(struct pci_dev *pdev, · line 472
Quoted 9 lines
drivers/fpga/altera-cvp.c +3 -6
-       ret = driver_create_file(&altera_cvp_driver.driver,
-                                &driver_attr_chkcfg);
-       if (ret) {
-               dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
-               fpga_mgr_unregister(mgr);
-               goto err_unmap;
+       if (!altera_cvp_cnt++) {
+               ret = driver_create_file(&altera_cvp_driver.driver,
+                                        &driver_attr_chkcfg);
Quoted 2 lines
In the review, I didn't catch that this was adding a driver file instead of a device file.
I was too focused on tons of comments to the driver patches when mainlining this driver and didn't have hardware with multiple PCIe cards to test it, so this bug crept in.
When adding it as a device file, it will be in the directory with many PCI device specific files, e.g:
# ls /sys/bus/pci/devices/0000\:0c\:00.0/
ari_enabled           config                    current_link_width  dma_mask_bits    enable        local_cpulist   max_link_width  numa_node  rescan    resource0  resource4     subsystem         uevent
broken_parity_status  consistent_dma_mask_bits  d3cold_allowed      driver           fpga_manager  local_cpus      modalias        power      reset     resource1  resource4_wc  subsystem_device  vendor
class                 current_link_speed        device              driver_override  irq           max_link_speed  msi_bus         remove     resource  resource2  revision      subsystem_vendor
I don't think that it is a good place for such driver specific control file. If we really must implement it per-device, then it would make more sense to use the current path and use something like

Quoted 8 lines

   echo "1 0000:0c:00.0" > /sys/bus/pci/drivers/altera-cvp/chkcfg
for enabling the option for a particular device. For disabling:
echo "0 0000:0c:00.0" > /sys/bus/pci/drivers/altera-cvp/chkcfg
But it is harder to implement. Is it worth the effort when it is hardly used?

I agree, yes let's keep it as is then and just fix it.

Thanks,
Alan

Quoted 3 lines

Thanks,
Anatolij

Alan Tull

2018-11-07 16:05 UTC #4 lore raw

Quoted 18 lines · Alan Tull

Quoted 9 lines · Anatolij Gustschin
Hi Alan,
On Tue, 6 Nov 2018 15:54:17 -0600
Alan Tull [email protected] wrote:
...
Quoted 3 lines
Quoted 2 lines
fpga_manager fpga3: Altera CvP FPGA Manager @0000:0c:00.0 registered
sysfs: cannot create duplicate filename '/bus/pci/drivers/altera-cvp/chkcfg'
Quoted 6 lines
Quoted 2 lines
Shouldn't this have been a file under /sys/class/fpga_manager/fpga0/ instead? This is a control per-device not per driver.
I thought /sys/class/fpga_manager/fpgaN/ interface is generic for fpga-mgr and low-level manager specific stuff does not belong there. At least this is my filling when reading Documentation/ABI/testing/sysfs-class-fpga-manager.

Quoted 23 lines

Yes
Quoted 4 lines
chkcfg is a debugging option and will be rarely used while development, it is off by default. And when enabling it globally at debugging time, it won't hurt other devices I think.
OK that's good to know.
Quoted 14 lines
If it were some device specific
behaviour control, then it surely would make sense to turn it on/off pre-device.
...
Quoted 9 lines
Quoted 9 lines
drivers/fpga/altera-cvp.c +3 -6
-       ret = driver_create_file(&altera_cvp_driver.driver,
-                                &driver_attr_chkcfg);
-       if (ret) {
-               dev_err(&pdev->dev, "Can't create sysfs chkcfg file\n");
-               fpga_mgr_unregister(mgr);
-               goto err_unmap;
+       if (!altera_cvp_cnt++) {
+               ret = driver_create_file(&altera_cvp_driver.driver,
+                                        &driver_attr_chkcfg);

Please consider whether moving this to the module init would solve your problem without having to add a counter.

Quoted 36 lines

Quoted 19 lines
Quoted 3 lines
In the review, I didn't catch that this was adding a driver file instead of a device file.
I was too focused on tons of comments to the driver patches when mainlining this driver and didn't have hardware with multiple PCIe cards to test it, so this bug crept in.
When adding it as a device file, it will be in the directory with many PCI device specific files, e.g:
# ls /sys/bus/pci/devices/0000\:0c\:00.0/
ari_enabled           config                    current_link_width  dma_mask_bits    enable        local_cpulist   max_link_width  numa_node  rescan    resource0  resource4     subsystem         uevent
broken_parity_status  consistent_dma_mask_bits  d3cold_allowed      driver           fpga_manager  local_cpus      modalias        power      reset     resource1  resource4_wc  subsystem_device  vendor
class                 current_link_speed        device              driver_override  irq           max_link_speed  msi_bus         remove     resource  resource2  revision      subsystem_vendor
I don't think that it is a good place for such driver specific control file. If we really must implement it per-device, then it would make more sense to use the current path and use something like
Quoted 8 lines
   echo "1 0000:0c:00.0" > /sys/bus/pci/drivers/altera-cvp/chkcfg
for enabling the option for a particular device. For disabling:
echo "0 0000:0c:00.0" > /sys/bus/pci/drivers/altera-cvp/chkcfg
But it is harder to implement. Is it worth the effort when it is hardly used?
I agree, yes let's keep it as is then and just fix it.
Thanks,
Alan
Quoted 3 lines
Thanks,
Anatolij

Anatolij Gustschin

author

2018-11-07 22:11 UTC #5 lore raw

Hi Alan,

On Wed, 7 Nov 2018 10:05:21 -0600
Alan Tull [email protected] wrote:
...

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/fpga/altera-cvp.c static int altera_cvp_probe(struct pci_dev *pdev, · line 472
Quoted 3 lines
drivers/fpga/altera-cvp.c +3 -0
+       if (!altera_cvp_cnt++) {
+               ret = driver_create_file(&altera_cvp_driver.driver,
+                                        &driver_attr_chkcfg);  
Please consider whether moving this to the module init would solve your problem without having to add a counter.

This should work and it is a better idea, thanks.

Anatolij