diff mbox series

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

Message ID 20181106203512.4509-1-agust@denx.de
State Changes Requested
Headers show
Series fpga: altera-cvp: fix probing for multiple FPGAs on the bus | expand

Commit Message

Anatolij Gustschin Nov. 6, 2018, 8:35 p.m. UTC
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 <agust@denx.de>
---
 drivers/fpga/altera-cvp.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

Comments

Alan Tull Nov. 6, 2018, 9:54 p.m. UTC | #1
On Tue, Nov 6, 2018 at 2:35 PM Anatolij Gustschin <agust@denx.de> wrote:

Hi Anatolij,

>
> 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'

Shouldn't this have been a file under /sys/class/fpga_manager/fpga0/
instead?  This is a control per-device not per driver.

> 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 <agust@denx.de>
> ---
>  drivers/fpga/altera-cvp.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 144fa2a4d4cc..381d0c42450f 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -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);

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

> +               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);
> --
> 2.17.1
>

Thanks,
Alan
Anatolij Gustschin Nov. 6, 2018, 11:56 p.m. UTC | #2
Hi Alan,

On Tue, 6 Nov 2018 15:54:17 -0600
Alan Tull atull@kernel.org 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
Alan Tull Nov. 7, 2018, 3:53 p.m. UTC | #3
On Tue, Nov 6, 2018 at 5:56 PM Anatolij Gustschin <agust@denx.de> wrote:
>
> Hi Alan,
>
> On Tue, 6 Nov 2018 15:54:17 -0600
> Alan Tull atull@kernel.org 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.

Yes

>
> 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.

> 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?

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

Thanks,
Alan

>
> Thanks,
> Anatolij
Alan Tull Nov. 7, 2018, 4:05 p.m. UTC | #4
On Wed, Nov 7, 2018 at 9:53 AM Alan Tull <atull@kernel.org> wrote:
>
> On Tue, Nov 6, 2018 at 5:56 PM Anatolij Gustschin <agust@denx.de> wrote:
> >
> > Hi Alan,
> >
> > On Tue, 6 Nov 2018 15:54:17 -0600
> > Alan Tull atull@kernel.org 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.
>
> Yes
>
> >
> > 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.
>
> > 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);

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

> > >
> > >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?
>
> I agree, yes let's keep it as is then and just fix it.
>
> Thanks,
> Alan
>
> >
> > Thanks,
> > Anatolij
Anatolij Gustschin Nov. 7, 2018, 10:11 p.m. UTC | #5
Hi Alan,

On Wed, 7 Nov 2018 10:05:21 -0600
Alan Tull atull@kernel.org wrote:
...
>> > >> +       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
diff mbox series

Patch

diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
index 144fa2a4d4cc..381d0c42450f 100644
--- a/drivers/fpga/altera-cvp.c
+++ b/drivers/fpga/altera-cvp.c
@@ -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);