Hi Alan,
On Tue, 6 Nov 2018 15:54:17 -0600
Alan Tull atull@xxxxxxxxxx wrote:
...
>> 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.
...
>> - 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);
>
>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
