Re: fpga: fpga_mgr_free usage

[Federico Vaga]

On Wednesday, July 25, 2018 6:33:44 PM CEST Alan Tull wrote:
> On Wed, Jul 11, 2018 at 10:59 AM, Alan Tull <atull@xxxxxxxxxx> wrote:
> > On Wed, Jul 11, 2018 at 7:38 AM, Federico Vaga <federico.vaga@xxxxxxx>
> > wrote:
> > 
> > Hi Federico,
> > 
> >> Hi Alan,
> >> 
> >> I have another point that I would like to discuss. It is about the
> >> usage of 'fpga_mgr_free()' which does not look like consistent.
> >> 
> >> This function, according to the current implementation, can be used by
> >> an FPGA manager user and it is used by the FPGA manager itself on
> >> device release. This means that the user can only use this function if
> >> fpga_mgr_register() fails (to clean up), otherwise the user must NOT
> >> use this function, otherwise we most likely get an oops (NULL or
> >> invalid pointer). The example here is correct, this is what we should
> >> do:
> >> 
> >> https://www.kernel.org/doc/html/latest/driver-api/fpga/fpga-mgr.html
> >> 
> >> But I suggest to document it better or prevent this type of mistakes
> >> from happening. Following a couple of proposals
> >> 
> >> ------
> >> 1.
> >> Document it better. This is easy, in the fpga_mgr_free() kernel-doc
> >> comment we explain that the use of this function must be limited to
> >> clean up the memory on a registration failure. If an FPGA manager has
> >> been successfully registered then it will be freed by the framework
> >> itself.
> 
> As I was researching this, I remembered why I implemented it this way.
>   See below for that explanation.
> 
> It looks like I'm going to switch to option 1 here and add more
> documentation for both fpga_mgr_free() and fpga_mgr_unregister().
> Note that fpga_mgr_unregister() already says that that it frees the
> manager, and the usage example already does the right thing, but I'll
> add more words to really beat the message in.
> 
> >> But still, this does not prevent an oops from happening.
> >> ------
> >> 2.
> >> Remove the fpga_mgr_free() from fpga_mgr_dev_release() and ask the
> >> user to free the manager when necessary.
> >> 
> >> This makes the usage consistent: the user creates and destroy its own
> >> objects. This is also consistent with our other discussion where we
> >> said, among the other things, that the module that uses the FPGA
> >> manager can the owner of the fpga_manager data structure.
> > 
> > You're not the first to complain about this.  I think I'll err on the
> > side of consistency and implement your option 2 here.
> > 
> > Alan
> 
> If you write a class or create a device, the kernel wants a release
> function and will give a warning if you leave it out.  The warning is
> "Device 'fpga0' does not have a release() function, it is broken and
> must be fixed." and comes from drivers/base/core.c.

True, but that function can be empty (in other words, it does nothing) and 
option 2 can be implemented as well without warnings. I do not think is that 
bad, for example if I allocate everything with devm_* probably I will not have 
much to do in my release() function.
Anyway, I do not have strong technical arguments in favor of option 1 or 2.

> I will add some more documentation to make it clear that once a a
> mgr/bridge/region has been registered, the cleanup will be handled
> automatically when the device goes away.  Until the
> fpga_(mgr|bridge|region)_register succeeds, the caller still needs to
> do cleanup.
> 
> I did find one bug while looking at this.  I'll post some patches.
> 
> Full message was:
> root@cyclone5:~# rmmod socfpga
> [   48.206235] fpga_manager fpga0: fpga_mgr_unregister Altera SOCFPGA
> FPGA Manager
> [   48.213677] ------------[ cut here ]------------
> [   48.218312] WARNING: CPU: 1 PID: 1369 at
> /home/atull/repos/linux-socfpga/drivers/base/core.c:895
> device_release+0x9c/0xa0
> [   48.229293] Device 'fpga0' does not have a release() function, it
> is broken and must be fixed.
> [   48.237904] Modules linked in: socfpga(-) altera_hps2fpga fpga_mgr
> fpga_bridge [last unloaded: fpga_region]
> [   48.247659] CPU: 1 PID: 1369 Comm: rmmod Not tainted
> 4.18.0-rc5-next-20180717-00012-ge5f548e-dirty #3
> [   48.256843] Hardware name: Altera SOCFPGA
> [   48.260858] [<c01137ac>] (unwind_backtrace) from [<c010dc04>]
> (show_stack+0x20/0x24)
> [   48.268582] [<c010dc04>] (show_stack) from [<c07d448c>]
> (dump_stack+0x8c/0xa0)
> [   48.275786] [<c07d448c>] (dump_stack) from [<c0123bc0>]
> (__warn+0x104/0x11c) [   48.282810] [<c0123bc0>] (__warn) from
> [<c0123c2c>]
> (warn_slowpath_fmt+0x54/0x70)
> [   48.290269] [<c0123c2c>] (warn_slowpath_fmt) from [<c052c9cc>]
> (device_release+0x9c/0xa0)
> [   48.298418] [<c052c9cc>] (device_release) from [<c07d904c>]
> (kobject_put+0xa8/0xe0)
> [   48.306047] [<c07d904c>] (kobject_put) from [<c052ec3c>]
> (device_unregister+0x2c/0x30)
> [   48.313939] [<c052ec3c>] (device_unregister) from [<bf01262c>]
> (fpga_mgr_unregister+0x58/0x74 [fpga_mgr])
> [   48.323475] [<bf01262c>] (fpga_mgr_unregister [fpga_mgr]) from
> [<bf02b01c>] (socfpga_fpga_remove+0x1c/0x24 [socfpga])
> [   48.334047] [<bf02b01c>] (socfpga_fpga_remove [socfpga]) from
> [<c0534be8>] (platform_drv_remove+0x34/0x4c)
> [   48.343664] [<c0534be8>] (platform_drv_remove) from [<c0532f64>]
> (device_release_driver_internal+0x180/0x230)
> [   48.353538] [<c0532f64>] (device_release_driver_internal) from
> [<c0533090>] (driver_detach+0x58/0xa0)
> [   48.362720] [<c0533090>] (driver_detach) from [<c0531bf8>]
> (bus_remove_driver+0x5c/0xb4)
> [   48.370781] [<c0531bf8>] (bus_remove_driver) from [<c0533a70>]
> (driver_unregister+0x38/0x58)
> [   48.379186] [<c0533a70>] (driver_unregister) from [<c0534cd0>]
> (platform_driver_unregister+0x1c/0x20)
> [   48.388370] [<c0534cd0>] (platform_driver_unregister) from
> [<bf02b688>] (socfpga_fpga_driver_exit+0x18/0x990 [socfpga])
> [   48.399113] [<bf02b688>] (socfpga_fpga_driver_exit [socfpga]) from
> [<c01ad948>] (sys_delete_module+0x1a0/0x1f0)
> [   48.409164] [<c01ad948>] (sys_delete_module) from [<c0101000>]
> (ret_fast_syscall+0x0/0x54)
> [   48.417391] Exception stack(0xee6dbfa8 to 0xee6dbff0)
> [   48.422424] bfa0:                   0001dce0 beba4be0 0001dd1c
> 00000800 0000000a 80080000
> [   48.430568] bfc0: 0001dce0 beba4be0 00000000 00000081 0001c22c
> 00000000 00000001 beba4dcc
> [   48.438708] bfe0: b6ecdd00 beba4b9c 00012b43 b6ecdd0c
> [   48.443773] ---[ end trace bcf003ed0f464330 ]---
> 
> Alan

Federico Vaga
[BE-CO-HT]