Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Wed, Jul 18, 2018 at 4:47 PM, Federico Vaga <federico.vaga@xxxxxxx> wrote:
> Hi Alan,
>
> Thanks for your time, comments below
>
> On Wednesday, July 18, 2018 9:47:24 PM CEST Alan Tull wrote:
>> On Thu, Jun 28, 2018 at 2:50 AM, Federico Vaga <federico.vaga@xxxxxxx>
> wrote:
>> > On Wednesday, 27 June 2018 23:23:07 CEST Alan Tull wrote:
>> >> On Wed, Jun 27, 2018 at 4:25 AM, Federico Vaga
>> >
>> > <federico.vaga@xxxxxxx> wrote:
>> >> > Hi Alan,
>> >> >
>> >> > On Tuesday, 26 June 2018 23:00:46 CEST Alan Tull wrote:
>> >> >> On Fri, Jun 22, 2018 at 2:53 AM, Federico Vaga
>> >> >> <federico.vaga@xxxxxxx> wrote:
>> >> >>
>> >> >> Hi Federico,
>> >> >>
>> >> >> >> > What is buggy is the function fpga_mgr_get().
>> >> >> >> > That patch has been done to allow multiple FPGA manager
>> >> >> >> > instances
>> >> >> >> > to be linked to the same device (PCI it says). But function
>> >> >> >> > fpga_mgr_get() will return only the first found: what about
>> >> >> >> > the
>> >> >> >> > others?
>>
>> Looking at this further, no code change is needed in the FPGA API to
>> support multiple managers.  A FPGA manager driver is a self-contained
>> platform driver.  The PCI driver for a board that contains multiple
>> FPGAs should create a platform device for each manager and save each
>> of those devs in its pdata.
>>
>> >> >> >> > Then, all load kernel-doc comments says:
>> >> >> >> >
>> >> >> >> > "This code assumes the caller got the mgr pointer from
>> >> >> >> > of_fpga_mgr_get() or fpga_mgr_get()"
>> >> >> >> >
>> >> >> >> > but that function does not allow me to get, for instance,
>> >> >> >> > the
>> >> >> >> > second FPGA manager on my card.
>>
>> fpga_mgr_get() will do what you want if your PCI driver creates a
>> platform device per FPGA manager as mentioned above.
>>
>> >> >> >> > Since, thanks to this patch I'm actually the creator of the
>> >> >> >> > fpga_manager structure,  I do not need to use fpga_mgr_get()
>> >> >> >> > to
>> >> >> >> > retrieve that data structure.
>>
>> The creator of the FPGA mgr structure is the low level FPGA manager
>> driver, not the PCIe driver.
>
> In my case it is.
> It's a bit like where SPI driver is the low level FPGA manager driver for
> the xilinx-spi.c. But if I understand what you mean, I should always have a
> platform_driver just for the FPGA manager even if it has a 1:1 relation
> with its carrier? In other words, I write two drivers:
> - one for the FPGA manager
> - one for the PCI device that then register the FPGA manager driver
>
> In my case the FPGA programmed is also the PCIe bridge (GN4124). Probably I
> can do it anyway, because the part dedicated to FPGA programming is quite
> independent from the rest (don't remember all details)
>
>> >> >> >> > Despite this, I believe we still need to increment the
>> >> >> >> > module
>> >> >> >> > reference counter (which is done by fpga_mgr_get()).
>>
>> This is only done in the probe function of a FPGA region driver.
>>
>> >> >> >> > We can fix this function by just replacing the argument from
>> >> >> >> > 'device' to 'fpga_manager' (the one returned by create() ).
>> >> >> >>
>> >> >> >> At first thought, that's what I'd want.
>> >> >> >>
>> >> >> >> > Alternatively, we
>> >> >> >> > can add an 'owner' field in "struct fpga_manager_ops" and
>> >> >> >> > 'get'
>> >> >> >> > it
>> >> >> >> > when we use it. Or again, just an 'owner' argument in the
>> >> >> >> > create()
>> >> >> >> > function.
>> >> >> >>
>> >> >> >> It seems like we shouldn't have to do that.
>> >> >> >
>> >> >> > Why?
>> >> >>
>> >> >> OK yes, I agree; the kernel has a lot of examples of doing this.
>> >> >>
>> >> >> I'll have to play with it, I'll probably add the owner arg to the
>> >> >> create function, add owner to struct fpga_manager, and save.
>> >> >
>> >> > I have two though about this.
>> >> >
>> >> > 1. file_operation like approach. The onwer is associated to the
>> >> > FPGA manager operation. Whenever the FPGA manager wants to use an
>> >> > operation it get/put the module owner of these operations
>> >> > (because this is what we need to protect). With this the user is
>> >> > not directly involved, read it as less code, less things to care
>> >> > about. And probably there is no need for fpga_manager_get().
>> >>
>> >> How does try_module_get fit into this scheme?  I think this proposal
>> >> #1 is missing the point of what the reference count increment is
>> >> meant to do.  Or am I misunderstanding?  How does that keep the
>> >> module from being unloaded 1/3 way through programming a FPGA?
>> >> IIUC you are saying that the reference count would be incremented
>> >> before doing the manager ops .write_init, then decremented again
>> >> afterwards, incremented before each call to .write, decremented
>> >> afterwards, then the same for .write_complete.
>> >
>> > I'm not saying to do module_get/put just around the mops->XXX(): it's
>> > too much. Just where you have this comment:
>> >
>> > "This code assumes the caller got the mgr pointer from
>> > of_fpga_mgr_get() or fpga_mgr_get()"
>> >
>> > Because, currently, it's here where we do module_get()
>>
>> No it's not.
>
> It is not in the code, but the comment says that we should do it before
> calling fpga_mgr_load()
>
>> fpga_mgr_get() or of_fpga_mgr_get() is called when the region is
>> created such as in of-fpga-region's probe.  That way, as long as the
>> region exists, the manager won't be unloaded.  If someone wants to
>> start unloading modules, they need to unload higher level ones first,
>> so they'll have to unload the region before mgr.
>>
>> > Most mops are invoked within a set of static functions which are
>> > called only by few exported functions. I'm suggesting to do
>> > module_get/put in those exported function at the beginning (get) and
>> > and the end (put) because we know that within these functions, here
>> > and there, we will use mops which are owned by a different module.
>> > We do not want the module owner of the mops to disappear while someone
>> > is doing fpga_mgr_load(). For example, inside fpga_mgr_load() we use
>> > most of the mops, so we 'get' the module at the beginning and 'put' it
>> > at the end. The same for fpga_region_program_fpga().
>>
>> If we do it the way you are suggesting, then someone can unload the
>> manager module without unloading the region.  The region code will be
>> in for a nasty surprise when programming is attempted.
>
> Of course, this should be taken into account. I was not suggesting a
> precise implementation, but only the idea to hide get/put. Probably there
> other things as well that I'm not considering (indeed I do not have a
> patch, but just a comment)
>
>> > Like this we do not have to ask users to do fpga_mgr_get()/put().
>>
>> The only user who has to do this is a region's probe function.
>>
>> I'm assuming that only fpga_region is using fpga_mgr_load() and
>> anybody who is creating a region uses fpga_region_program_fpga().
>> That's what I want to encourage anyway.  I should probably move
>> fpga_mgr_load to a private header.
>
> All right, if this is what you want to encourage I do not have anything to
> say because I did exactly what you do not want to encourage :)
>
> For me this change everything because I do not use regions since I do not
> have them. The way the API is exposed to me did not encouraged me to use
> their API. In addition, the documentation guides me directly to the FPGA
> manager.

The documentation says:

"If you are adding a new interface to the FPGA framework, add it on
top of a FPGA region to allow the most reuse of your interface."

https://www.kernel.org/doc/html/latest/driver-api/fpga/intro.html

But the stuff that I submitted yesterday goes back through the docs to
clear out anything that is not clear about this.

>
> To be honest I did not have much time to look at the region code. My
> understanding, after a quick look, is that it works great with device-tree.

It is used by the DFL framework which doesn't use device tree.

> But what if I do not have it? Actually, I cannot use device-tree because of
> environment limitations and Linux version in use. Oops, this implies that I
> back-ported the FPGA manager to an older Linux version? Yes, guilty :)

I separated region code from its device-tree dependencies.  But if you
can't use device-tree, then you end up having to implement some of the
things DT gives you for free.

>
> Anyway, I'm using the API exposed, and if part of the assumptions behind
> this API is that I should use device-tree, then I'm out of scope.
>
> <chatting>
> Just for chatting. One addition I made for the FPGA manager is to support
> dynamic loading of FPGA code using char devices. Something like:
>
>    dd if=binary.bin of=/dev/fpga0
>    cat binary.bin > /dev/fpga0

Since it's not handling the bridge, there's some risk involved.

>
> We do not have device tree, and we do not have binaries in /lib/firmware.
> It's quite handy to quickly load a binary just synthesized, especially for
> debugging purpose.

Like a debugfs?

https://lkml.org/lkml/2018/8/2/125

But for a debugging/developing, I have a debugfs that was a downstream
patchset that I'm cleaning for submission.

> If you are interested I can try to clean it up (at some
> point, probably in autumn), or I can show you the code in private for a
> quick look.
> </chatting>
>
>
>> > <Parenthesis>
>> > fpga_region_program_fpga() does not do (today) fpga_mgr_get() because
>> > it assumes, but it is not enforced, that both fpga_region and fpga_mgr
>> > are child of the same device.
>>
>> fpga_region_program_fpga() doesn't do fpga_mgr_get() becuase
>> fpga_mgr_get() happens when the fpga_region probes.  The assumption I
>> am making is that nobody other than a region is calling
>> fpga_manager_load().  I should create a fpga_private.h and move
>> fpga_manager_load() out of fpga-mgr.h to make that official.
>
> Yes, I agree. If what I'm doing is not supposed to happen I should not be
> allowed to do it :)
>
> <suggestion>
> If you want to encourage people to use regions rather than the manager
> directly, have you though about changing the API and merge in a single
> module fpga-mgr and fpga-region?
>
> Brainstorming. Perhaps, it is possible to have a `struct fpga_region_info`
> and when we register and FPGA manager we use something like:
>
> struct fpga_manager *fpga_mgr_create(struct device *dev, const char *name,
>                                      const struct fpga_manager_ops *mops,
>                      struct fpga_region_info *info, int n_regions,
>                                      void *priv)
>
> So those regions will be created directly and the interface will be smaller
> and easier.
>
> Don't waste much time on this suggestion, as I said before I did not study
> much the fpga-region.c code and perhaps this is not possible and I'm just
> speaking rubbish :)
> </suggestion>
>
>
>> > Probably this is true 99.99% of the
>> > time.
>> > Let me open in parallel another point: why fpga_region is not a child
>> > of fpga_manager?
>>
>> FPGA regions are children of FPGA bridges.
>>
>> Alan
>
>
> --
> Federico Vaga
> [BE-CO-HT]
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html