fpga: fpga_mgr_get() buggy ?

fpga: fpga_mgr_get() buggy ?

[Federico Vaga]

Hello,

I believe that this patch

fpga: manager: change api, don't use drvdata
7085e2a94f7df5f419e3cfb2fe809ce6564e9629

is incomplete and buggy.

I completely agree that drvdata should not be used by the FPGA manager 
or any other subsystem like that.

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?

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.

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.
Despite this, I believe we still need to increment the module 
reference counter (which is done by fpga_mgr_get()).

We can fix this function by just replacing the argument from 'device' 
to 'fpga_manager' (the one returned by create() ). 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. I'm proposing these alternatives because I'm not sure that 
this is correct:

	if (!try_module_get(dev->parent->driver->owner))

What if the device does not have a driver? Do we consider the 
following a valid use case?


probe(struct device *dev) {
  struct device *mydev;

  mydev->parent = dev;
  device_register(mydev);
  fpga_mrg_create(mydev, ....);
}


thanks :)

Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Thu, Jun 21, 2018 at 8:13 AM, Federico Vaga <federico.vaga@cern.ch> wrote:

Hi Federico,

Thanks for the analysis.  I'll probably not be able to look into this
very much until next week.  A few notes below.

> Hello,
>
> I believe that this patch
>
> fpga: manager: change api, don't use drvdata
> 7085e2a94f7df5f419e3cfb2fe809ce6564e9629
>
> is incomplete and buggy.
>
> I completely agree that drvdata should not be used by the FPGA manager
> or any other subsystem like that.
>
> 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?

I was thinking it was going to be one manager per device which makes
sense if the device corresponds to a single FPGA.  But I could see
that there could be valid use cases that had more than one FPGA such
as on a PCI card.

>
> 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.
>
> 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.
> Despite this, I believe we still need to increment the module
> reference counter (which is done by fpga_mgr_get()).
>
> 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.

> I'm proposing these alternatives because I'm not sure that
> this is correct:
>
>         if (!try_module_get(dev->parent->driver->owner))
>
> What if the device does not have a driver? Do we consider the
> following a valid use case?
>
>
> probe(struct device *dev) {
>   struct device *mydev;
>
>   mydev->parent = dev;
>   device_register(mydev);
>   fpga_mrg_create(mydev, ....);
> }

When would you want to do that?

Alan

>
>
> thanks :)
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: fpga: fpga_mgr_get() buggy ?

[Federico Vaga]

Hi Alan,

inline comments

On Friday, 22 June 2018 04:07:41 CEST Alan Tull wrote:
> On Thu, Jun 21, 2018 at 8:13 AM, Federico Vaga
> <federico.vaga@cern.ch> wrote:
> 
> Hi Federico,
> 
> Thanks for the analysis.  I'll probably not be able to look into
> this very much until next week.  A few notes below.
> 
> > Hello,
> > 
> > I believe that this patch
> > 
> > fpga: manager: change api, don't use drvdata
> > 7085e2a94f7df5f419e3cfb2fe809ce6564e9629
> > 
> > is incomplete and buggy.
> > 
> > I completely agree that drvdata should not be used by the FPGA
> > manager or any other subsystem like that.
> > 
> > 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?
> 
> I was thinking it was going to be one manager per device which makes
> sense if the device corresponds to a single FPGA.  But I could see
> that there could be valid use cases that had more than one FPGA
> such as on a PCI card.

Here a practical example where we have 2 FPGAs on the same card

https://www.ohwr.org/projects/svec/wiki/wiki

> > 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.
> > 
> > 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.
> > Despite this, I believe we still need to increment the module
> > reference counter (which is done by fpga_mgr_get()).
> > 
> > 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? 
 
> > I'm proposing these alternatives because I'm not sure that
> > 
> > this is correct:
> >         if (!try_module_get(dev->parent->driver->owner))
> > 
> > What if the device does not have a driver? Do we consider the
> > following a valid use case?
> > 
> > 
> > probe(struct device *dev) {
> > 
> >   struct device *mydev;
> >   
> >   mydev->parent = dev;
> >   device_register(mydev);
> >   fpga_mrg_create(mydev, ....);
> > 
> > }
>
> When would you want to do that?

Not sure when, I'm in the middle of some other development and I 
stumbled into this issue. But of course I can do it ... at some point 
:)

> Alan
> 
> > thanks :)
> > 
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-fpga" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Fri, Jun 22, 2018 at 2:53 AM, Federico Vaga <federico.vaga@cern.ch> 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?

I've had more time with this, I agree with you.  I didn't intend to
limit us to one manager per parent device.

>> > 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.
>> >
>> > 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.
>> > Despite this, I believe we still need to increment the module
>> > reference counter (which is done by fpga_mgr_get()).
>> >
>> > 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'm proposing these alternatives because I'm not sure that
>> >
>> > this is correct:
>> >         if (!try_module_get(dev->parent->driver->owner))
>> >
>> > What if the device does not have a driver? Do we consider the
>> > following a valid use case?
>> >
>> >
>> > probe(struct device *dev) {
>> >
>> >   struct device *mydev;
>> >
>> >   mydev->parent = dev;
>> >   device_register(mydev);
>> >   fpga_mrg_create(mydev, ....);
>> >
>> > }

Sure

>>
>> When would you want to do that?
>
> Not sure when, I'm in the middle of some other development and I
> stumbled into this issue. But of course I can do it ... at some point
> :)

I was meaning to ask something else.  I don't mind writing this and
would be interested in your review/feedback.  Thanks again for seeing
this and for the thoughtful analysis.

Alan

Re: fpga: fpga_mgr_get() buggy ?

[Federico Vaga]

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@cern.ch> 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?
> 
> I've had more time with this, I agree with you.  I didn't intend to
> limit us to one manager per parent device.
> 
> >> > 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.
> >> > 
> >> > 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.
> >> > Despite this, I believe we still need to increment the module
> >> > reference counter (which is done by fpga_mgr_get()).
> >> > 
> >> > 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().

2. fpga_manager onwer, we can still play the game above but 
conceptually, from the user point of view, I see it like the driver 
that creates the fpga_manager instance becomes the owner of it. I 
think that this is not true, the fpga_manager structure is completely 
used by the FPGA manager module and the driver use it as a token to 
access the FPGA manager API. I hope my point is clear :)

I'm more for the solution 1.

> >> > I'm proposing these alternatives because I'm not sure that
> >> > 
> >> > this is correct:
> >> >         if (!try_module_get(dev->parent->driver->owner))
> >> > 
> >> > What if the device does not have a driver? Do we consider the
> >> > following a valid use case?
> >> > 
> >> > 
> >> > probe(struct device *dev) {
> >> > 
> >> >   struct device *mydev;
> >> >   
> >> >   mydev->parent = dev;
> >> >   device_register(mydev);
> >> >   fpga_mrg_create(mydev, ....);
> >> > 
> >> > }
> 
> Sure
> 
> >> When would you want to do that?
> > 
> > Not sure when, I'm in the middle of some other development and I
> > stumbled into this issue. But of course I can do it ... at some
> > point> 
> > :)
> 
> I was meaning to ask something else. 

I see, you meant the example about the "virtual device" without 
driver. I do not have a true example, but this is a possible case I 
think we should support it or not (check this on register())

> I don't mind writing this and would be interested in your review/
> feedback.  Thanks again for seeing this and for the thoughtful
> analysis.

I'm here for any feedback/review :)

Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Wed, Jun 27, 2018 at 4:25 AM, Federico Vaga <federico.vaga@cern.ch> 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@cern.ch> 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?
>>
>> I've had more time with this, I agree with you.  I didn't intend to
>> limit us to one manager per parent device.
>>
>> >> > 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.
>> >> >
>> >> > 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.
>> >> > Despite this, I believe we still need to increment the module
>> >> > reference counter (which is done by fpga_mgr_get()).
>> >> >
>> >> > 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.

>
> 2. fpga_manager onwer, we can still play the game above but
> conceptually, from the user point of view, I see it like the driver
> that creates the fpga_manager instance becomes the owner of it.

I think that's what's happening.  And can continue to happen, adding
the owner parameter to fpga_mgr_create.

> I
> think that this is not true,

How do you figure that?  fpga_mgr_create() is called with pdev->dev
from each of the FPGA manager's probe functions.  It stores dev as the
parent: mgr->dev.parent = dev;

> the fpga_manager structure is completely
> used by the FPGA manager module and the driver use it as a token to
> access the FPGA manager API. I hope my point is clear :)
>
> I'm more for the solution 1.
>
>> >> > I'm proposing these alternatives because I'm not sure that
>> >> >
>> >> > this is correct:
>> >> >         if (!try_module_get(dev->parent->driver->owner))
>> >> >
>> >> > What if the device does not have a driver? Do we consider the
>> >> > following a valid use case?
>> >> >
>> >> >
>> >> > probe(struct device *dev) {
>> >> >
>> >> >   struct device *mydev;
>> >> >
>> >> >   mydev->parent = dev;
>> >> >   device_register(mydev);
>> >> >   fpga_mrg_create(mydev, ....);
>> >> >
>> >> > }
>>
>> Sure
>>
>> >> When would you want to do that?
>> >
>> > Not sure when, I'm in the middle of some other development and I
>> > stumbled into this issue. But of course I can do it ... at some
>> > point>
>> > :)
>>
>> I was meaning to ask something else.
>
> I see, you meant the example about the "virtual device" without
> driver. I do not have a true example, but this is a possible case I
> think we should support it or not (check this on register())

I think we should support this use, yes.

Alan

>
>> I don't mind writing this and would be interested in your review/
>> feedback.  Thanks again for seeing this and for the thoughtful
>> analysis.
>
> I'm here for any feedback/review :)

Re: fpga: fpga_mgr_get() buggy ?

[Federico Vaga]

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@cern.ch> 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@cern.ch> 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?
> >> 
> >> I've had more time with this, I agree with you.  I didn't intend
> >> to
> >> limit us to one manager per parent device.
> >> 
> >> >> > 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.
> >> >> > 
> >> >> > 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.
> >> >> > Despite this, I believe we still need to increment the
> >> >> > module
> >> >> > reference counter (which is done by fpga_mgr_get()).
> >> >> > 
> >> >> > 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()

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().
Like this we do not have to ask users to do fpga_mgr_get()/put().

<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. 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? (and then fpga_region_create can have one argument 
less and close any other weird possibility)
</Parenthesis>

I'm thinking about fpga_mgr_load() because, in principle, this can be 
used by other modules to program the FPGA with what they want.
While functions like fpga_mgr_unregister() are most likely called by 
the module that owns the ops; here we can safely assumes that its the 
same module to call this function and not a third one (if this is the 
case, then it is a bug) and we can forget about module_get()/put() 
(and indeed you do not suggest to call fpga_mgr_get()).

I did my best to make it clear but I understand that it is not always 
easy to read someone else mind and I'm not a best-seller writer :D
 
> > 2. fpga_manager onwer, we can still play the game above but
> > conceptually, from the user point of view, I see it like the
> > driver
> > that creates the fpga_manager instance becomes the owner of it.
> 
> I think that's what's happening.  And can continue to happen, adding
> the owner parameter to fpga_mgr_create.
> 
> > I
> > think that this is not true,
> 
> How do you figure that?  fpga_mgr_create() is called with pdev->dev
> from each of the FPGA manager's probe functions.  It stores dev as
> the parent: mgr->dev.parent = dev;

Let me say that I'm not advocating for this or that, I'm expressing my 
point of view (taste if you want). I do not see a true technical 
difference because mops are in a 1:1 relation with the fpga_manager 
structure. So, whatever is easier to implement is good :)

Just to try to clarify my way of seeing things ----------------------
In the reletionship between the driver and the FPGA manager, if we say 
that the driver owns the fpga_manager instance, then we do not need to 
do any module_get/put, because it is the driver that does everything 
with its own instance. If the driver does something wrong with the 
FPGA manager, it's its fault.
 
If we allow a third module to use fpga_mgr_load() (which I think is 
the case) then I think it's different. The fpga_manager is an instance 
owned by the FPGA manager module; fpga_manager is a token to access 
some services (like a middleware), when you use these services we need 
support (mops) from another module that actually does the loading and 
owns the operations.
And I stop here with this argument ----------------------------------

> > the fpga_manager structure is completely
> > used by the FPGA manager module and the driver use it as a token
> > to
> > access the FPGA manager API. I hope my point is clear :)
> > 
> > I'm more for the solution 1.
> > 
> >> >> > I'm proposing these alternatives because I'm not sure that
> >> >> > 
> >> >> > this is correct:
> >> >> >         if (!try_module_get(dev->parent->driver->owner))
> >> >> > 
> >> >> > What if the device does not have a driver? Do we consider
> >> >> > the
> >> >> > following a valid use case?
> >> >> > 
> >> >> > 
> >> >> > probe(struct device *dev) {
> >> >> > 
> >> >> >   struct device *mydev;
> >> >> >   
> >> >> >   mydev->parent = dev;
> >> >> >   device_register(mydev);
> >> >> >   fpga_mrg_create(mydev, ....);
> >> >> > 
> >> >> > }
> >> 
> >> Sure
> >> 
> >> >> When would you want to do that?
> >> > 
> >> > Not sure when, I'm in the middle of some other development and
> >> > I
> >> > stumbled into this issue. But of course I can do it ... at some
> >> > point>
> >> > 
> >> > :)
> >> 
> >> I was meaning to ask something else.
> > 
> > I see, you meant the example about the "virtual device" without
> > driver. I do not have a true example, but this is a possible case
> > I
> > think we should support it or not (check this on register())
> 
> I think we should support this use, yes.
> 
> Alan
> 
> >> I don't mind writing this and would be interested in your review/
> >> feedback.  Thanks again for seeing this and for the thoughtful
> >> analysis.
> > 
> > I'm here for any feedback/review :)

Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Thu, Jun 28, 2018 at 2:50 AM, Federico Vaga <federico.vaga@cern.ch> 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@cern.ch> 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@cern.ch> 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.

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

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.

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

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

> 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

Re: fpga: fpga_mgr_get() buggy ?

[Federico Vaga]

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@cern.ch> 
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@cern.ch> 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@cern.ch> 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.

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

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

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

Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Wed, Jul 18, 2018 at 4:47 PM, Federico Vaga <federico.vaga@cern.ch> 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@cern.ch>
> 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@cern.ch> 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@cern.ch> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: fpga: fpga_mgr_get() buggy ?

[Federico Vaga]

Hi alan,

inline comments

On Wednesday, August 15, 2018 11:02:12 PM CEST Alan Tull wrote:
> On Wed, Jul 18, 2018 at 4:47 PM, Federico Vaga <federico.vaga@cern.ch> 
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@cern.ch>
> > 
> > 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@cern.ch> 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@cern.ch> 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."

I would change this in something like:

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

What I understand from the current statement is:
"if you care about re-usability, add your interface on top of an FPGA region" 
Since in my special case, I do not care about re-usability, I skipped the FPGA 
region.

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

unfortunately yes

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

Yes, like that more or less.

I did a chardevice in /dev/ because in our environment debugfs is not mounted 
automatically and we need this feature also operationally. But yes, that's the 
idea.

One comment. The code on github [1] looks like it is assuming that on 
userspace people write the full image in one shot and it is smaller that 4MiB. 
What I did is to build a scatterlist in order to support the following case:

cat image.bin > /sys/kernel/debug/.../image

where `cat` write 4K at time. And it can be bigger than 4M

[1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/
drivers/fpga/fpga-mgr-debugfs.c

Below (end of this email) the patch I quickly did last year. Unfortunately I 
do not have the time to clean this solution, so it is the very first thing I 
implemented without many thoughts (that's why I never pushed this patch).


> > 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@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


------
From d6a6df9515c5e92cc74b8948c3c10ac9cbeec6d2 Mon Sep 17 00:00:00 2001
From: Federico Vaga <federico.vaga@vaga.pv.it>
Date: Mon, 20 Nov 2017 14:40:26 +0100
Subject: [PATCH] fpga: program from user-space

Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
---
 Documentation/fpga/fpga-mgr.txt |   3 +
 drivers/fpga/fpga-mgr.c         | 261 ++++++++++++++++++++++++++++++++++++++
+-
 include/linux/fpga/fpga-mgr.h   |  19 +++
 3 files changed, 281 insertions(+), 2 deletions(-)

diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
index 78f197f..397dae9 100644
--- a/Documentation/fpga/fpga-mgr.txt
+++ b/Documentation/fpga/fpga-mgr.txt
@@ -197,3 +197,6 @@ to put the FPGA into operating mode.
 The ops include a .state function which will read the hardware FPGA manager 
and
 return a code of type enum fpga_mgr_states.  It doesn't result in a change in
 hardware state.
+
+Configuring the FPGA from user-space
+====================================
diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 6441f91..964b7e4 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -27,10 +27,56 @@
 #include <linux/slab.h>
 #include <linux/scatterlist.h>
 #include <linux/highmem.h>
+#include <linux/spinlock.h>
 #include <linux/version.h>
+#include <linux/cdev.h>
+#include <linux/list.h>
 
 static DEFINE_IDA(fpga_mgr_ida);
 static struct class *fpga_mgr_class;
+static dev_t fpga_mgr_devt;
+
+/**
+ * struct fpga_image_chunck - FPGA configuration chunck
+ * @data: chunk data
+ * @size: chunk data size
+ * @list: for the linked list of chunks
+ */
+struct fpga_image_chunk {
+	void *data;
+	size_t size;
+	struct list_head list;
+};
+#define CHUNK_MAX_SIZE (PAGE_SIZE)
+
+/**
+ * struct fpga_user_load - structure to handle configuration from user-space
+ * @mgr: pointer to the FPGA manager
+ * @chunk_list: HEAD point of a linked list of FPGA chunks
+ * @n_chunks: number of chunks in the list
+ * @lock: it protects: chunk_list, n_chunks
+ */
+struct fpga_user_load {
+	struct fpga_manager *mgr;
+	struct list_head chunk_list;
+	unsigned int n_chunks;
+	struct spinlock lock;
+};
+
+
+/**
+ * It sets by default a huge timeout for configuration coming from user-space
+ * just to play safe.
+ *
+ * FIXME what about sysfs parameters to adjust it? The flag bit in particular
+ */
+struct fpga_image_info default_user_info = {
+	.flags = 0,
+	.enable_timeout_us = 10000000, /* 10s */
+	.disable_timeout_us = 10000000, /* 10s */
+	.config_complete_timeout_us = 10000000, /* 10s */
+};
+
 
 /*
  * Call the low level driver's write_init function.  This will do the
@@ -310,6 +356,158 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
 }
 EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
 
+
+static int fpga_mgr_open(struct inode *inode, struct file *file)
+{
+	struct fpga_manager *mgr = container_of(inode->i_cdev,
+						struct fpga_manager,
+						cdev);
+	struct fpga_user_load *usr;
+	int ret;
+
+	/* Allow the user-space programming only if user unlocked the FPGA */
+	if (!test_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags)) {
+		dev_info(&mgr->dev, "The FPGA programming is locked\n");
+		return -EPERM;
+	}
+
+	ret = mutex_trylock(&mgr->usr_mutex);
+	if (ret == 0)
+		return -EBUSY;
+
+	usr = kzalloc(sizeof(struct fpga_user_load), GFP_KERNEL);
+	if (!usr) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
+
+	usr->mgr = mgr;
+	spin_lock_init(&usr->lock);
+	INIT_LIST_HEAD(&usr->chunk_list);
+	file->private_data = usr;
+
+	return 0;
+
+err_alloc:
+	spin_lock(&mgr->lock);
+	clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+	spin_unlock(&mgr->lock);
+	mutex_unlock(&mgr->usr_mutex);
+	return ret;
+}
+
+
+static int fpga_mgr_flush(struct file *file, fl_owner_t id)
+{
+	struct fpga_user_load *usr = file->private_data;
+	struct fpga_image_chunk *chunk, *tmp;
+	struct sg_table sgt;
+	struct scatterlist *sg;
+	int err = 0;
+
+	if (!usr->n_chunks)
+		return 0;
+
+	err = sg_alloc_table(&sgt, usr->n_chunks, GFP_KERNEL);
+	if (err)
+		goto out;
+
+	sg = sgt.sgl;
+	list_for_each_entry(chunk, &usr->chunk_list, list) {
+		sg_set_buf(sg, chunk->data, chunk->size);
+		sg = sg_next(sg);
+		if (!sg)
+			break;
+	}
+
+	err = fpga_mgr_buf_load_sg(usr->mgr,
+				   &default_user_info,
+				   &sgt);
+	sg_free_table(&sgt);
+
+out:
+	/* Remove all chunks */
+	spin_lock(&usr->lock);
+	list_for_each_entry_safe(chunk, tmp, &usr->chunk_list, list) {
+		list_del(&chunk->list);
+		kfree(chunk->data);
+		kfree(chunk);
+		usr->n_chunks--;
+	}
+	spin_unlock(&usr->lock);
+
+	return err;
+}
+
+
+static int fpga_mgr_close(struct inode *inode, struct file *file)
+{
+	struct fpga_user_load *usr = file->private_data;
+	struct fpga_manager *mgr = usr->mgr;
+
+	kfree(usr);
+
+	spin_lock(&mgr->lock);
+	clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+	spin_unlock(&mgr->lock);
+
+	mutex_unlock(&mgr->usr_mutex);
+
+	return 0;
+}
+
+
+static ssize_t fpga_mgr_write(struct file *file, const char __user *buf,
+			      size_t count, loff_t *offp)
+{
+	struct fpga_user_load *usr = file->private_data;
+	struct fpga_image_chunk *chunk;
+	int err;
+
+	chunk = kmalloc(sizeof(struct fpga_image_chunk), GFP_KERNEL);
+	if (!chunk)
+		return -ENOMEM;
+
+	chunk->size = count > CHUNK_MAX_SIZE ? CHUNK_MAX_SIZE : count;
+	chunk->data = kmalloc(chunk->size, GFP_KERNEL);
+	if (!chunk->data) {
+		err = -ENOMEM;
+		goto err_buf_alloc;
+	}
+
+	err = copy_from_user(chunk->data, buf, chunk->size);
+	if(err)
+		goto err_buf_copy;
+
+	spin_lock(&usr->lock);
+	list_add_tail(&chunk->list, &usr->chunk_list);
+	usr->n_chunks++;
+	spin_unlock(&usr->lock);
+
+	*offp += count;
+
+	return chunk->size;
+
+err_buf_copy:
+	kfree(chunk->data);
+err_buf_alloc:
+	kfree(chunk);
+	return err;
+}
+
+
+/**
+ * Char device operation
+ */
+static const struct file_operations fpga_mgr_fops = {
+	.owner = THIS_MODULE,
+	.open = fpga_mgr_open,
+	.flush = fpga_mgr_flush,
+	.release = fpga_mgr_close,
+	.write  = fpga_mgr_write,
+};
+
+
 static const char * const state_str[] = {
 	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
 	[FPGA_MGR_STATE_POWER_OFF] =		"power off",
@@ -352,13 +550,43 @@ static ssize_t state_show(struct device *dev,
 	return sprintf(buf, "%s\n", state_str[mgr->state]);
 }
 
+static ssize_t config_lock_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	if (test_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags))
+		return sprintf(buf, "unlock\n");
+	return sprintf(buf, "lock\n");
+}
+
+static ssize_t config_lock_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	spin_lock(&mgr->lock);
+	if (strncmp(buf, "lock" , min(4, (int)count)) == 0)
+		clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+	else if (strncmp(buf, "unlock" , min(6, (int)count)) == 0)
+		set_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
+	else
+		count = -EINVAL;
+	spin_unlock(&mgr->lock);
+
+	return count;
+}
+
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
 static DEVICE_ATTR_RO(name);
 static DEVICE_ATTR_RO(state);
+static DEVICE_ATTR_RW(config_lock);
 
 static struct attribute *fpga_mgr_attrs[] = {
 	&dev_attr_name.attr,
 	&dev_attr_state.attr,
+	&dev_attr_lock.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(fpga_mgr);
@@ -367,6 +595,9 @@ ATTRIBUTE_GROUPS(fpga_mgr);
 static struct device_attribute fpga_mgr_attrs[] = {
 	__ATTR(name, S_IRUGO, name_show, NULL),
 	__ATTR(state, S_IRUGO, state_show, NULL),
+	__ATTR(config_lock, S_IRUGO | S_IWUSR | S_IRGRP | S_IWGRP,
+	       config_lock_show, config_lock_store),
+	__ATTR_NULL,
 };
 #endif
 
@@ -507,6 +738,8 @@ int fpga_mgr_register(struct device *dev, const char 
*name,
 	}
 
 	mutex_init(&mgr->ref_mutex);
+	mutex_init(&mgr->usr_mutex);
+	spin_lock_init(&mgr->lock);
 
 	mgr->name = name;
 	mgr->mops = mops;
@@ -524,12 +757,19 @@ int fpga_mgr_register(struct device *dev, const char 
*name,
 	mgr->dev.parent = dev;
 	mgr->dev.of_node = dev->of_node;
 	mgr->dev.id = id;
+	mgr->dev.devt = MKDEV(MAJOR(fpga_mgr_devt), id);
 	dev_set_drvdata(dev, mgr);
 
 	ret = dev_set_name(&mgr->dev, "fpga%d", id);
 	if (ret)
 		goto error_device;
 
+	cdev_init(&mgr->cdev, &fpga_mgr_fops);
+	mgr->cdev.owner = THIS_MODULE;
+	ret = cdev_add(&mgr->cdev, mgr->dev.devt, 1);
+	if (ret)
+		goto err_cdev;
+
 	ret = device_add(&mgr->dev);
 	if (ret)
 		goto error_device;
@@ -539,6 +779,8 @@ int fpga_mgr_register(struct device *dev, const char 
*name,
 	return 0;
 
 error_device:
+	cdev_del(&mgr->cdev);
+err_cdev:
 	ida_simple_remove(&fpga_mgr_ida, id);
 error_kfree:
 	kfree(mgr);
@@ -572,17 +814,27 @@ static void fpga_mgr_dev_release(struct device *dev)
 {
 	struct fpga_manager *mgr = to_fpga_manager(dev);
 
+	cdev_del(&mgr->cdev);
 	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
 	kfree(mgr);
 }
 
 static int __init fpga_mgr_class_init(void)
 {
+	int err;
+
 	pr_info("FPGA manager framework\n");
 
+	err = alloc_chrdev_region(&fpga_mgr_devt, 0, FPGA_MGR_MAX_DEV,
+				  "fpga_mgr");
+	if (err)
+		return err;
+
 	fpga_mgr_class = class_create(THIS_MODULE, "fpga_manager");
-	if (IS_ERR(fpga_mgr_class))
-		return PTR_ERR(fpga_mgr_class);
+	if (IS_ERR(fpga_mgr_class)) {
+		err = PTR_ERR(fpga_mgr_class);
+		goto err_class;
+	}
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
 	fpga_mgr_class->dev_groups = fpga_mgr_groups;
 #else
@@ -591,12 +843,17 @@ static int __init fpga_mgr_class_init(void)
 	fpga_mgr_class->dev_release = fpga_mgr_dev_release;
 
 	return 0;
+
+err_class:
+	unregister_chrdev_region(fpga_mgr_devt, FPGA_MGR_MAX_DEV);
+	return err;
 }
 
 static void __exit fpga_mgr_class_exit(void)
 {
 	class_destroy(fpga_mgr_class);
 	ida_destroy(&fpga_mgr_ida);
+	unregister_chrdev_region(fpga_mgr_devt, FPGA_MGR_MAX_DEV);
 }
 
 MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index bfa14bc..ae38e48 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -15,8 +15,10 @@
  * You should have received a copy of the GNU General Public License along 
with
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
+#include <linux/cdev.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 
 #ifndef _LINUX_FPGA_MGR_H
 #define _LINUX_FPGA_MGR_H
@@ -24,6 +26,8 @@
 struct fpga_manager;
 struct sg_table;
 
+#define FPGA_MGR_MAX_DEV (256)
+
 /**
  * enum fpga_mgr_states - fpga framework states
  * @FPGA_MGR_STATE_UNKNOWN: can't determine state
@@ -118,22 +122,37 @@ struct fpga_manager_ops {
 	void (*fpga_remove)(struct fpga_manager *mgr);
 };
 
+/*
+ * List of status FLAGS for the FPGA manager
+ */
+#define FPGA_MGR_FLAG_BITS (32)
+#define FPGA_MGR_FLAG_UNLOCK BIT(0)
+
 /**
  * struct fpga_manager - fpga manager structure
  * @name: name of low level fpga manager
+ * @cdev: char device interface
  * @dev: fpga manager device
  * @ref_mutex: only allows one reference to fpga manager
  * @state: state of fpga manager
  * @mops: pointer to struct of fpga manager ops
  * @priv: low level driver private date
+ * @flags: manager status bits
+ * @lock: it protects: flags
+ * @usr_mutex: only allows one user to program the FPGA
  */
 struct fpga_manager {
 	const char *name;
+	struct cdev cdev;
 	struct device dev;
 	struct mutex ref_mutex;
 	enum fpga_mgr_states state;
 	const struct fpga_manager_ops *mops;
 	void *priv;
+
+	DECLARE_BITMAP(flags, FPGA_MGR_FLAG_BITS);
+	struct spinlock lock;
+	struct mutex usr_mutex;
 };
 
 #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
-- 
2.15.0

Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull]

On Thu, Aug 16, 2018 at 2:18 AM, Federico Vaga <federico.vaga@cern.ch> wrote:
> Hi alan,
>
> inline comments
>
> On Wednesday, August 15, 2018 11:02:12 PM CEST Alan Tull wrote:
>> On Wed, Jul 18, 2018 at 4:47 PM, Federico Vaga <federico.vaga@cern.ch>
> 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@cern.ch>
>> >
>> > 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@cern.ch> 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@cern.ch> 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."
>
> I would change this in something like:
>
> "If you are adding a new interface to the FPGA framework, add it on
> top of a FPGA region."

Sure, will do in v2.

>
> What I understand from the current statement is:
> "if you care about re-usability, add your interface on top of an FPGA region"
> Since in my special case, I do not care about re-usability, I skipped the FPGA
> region.

OK yeah, not what I'm trying to communicate.  Good feedback.

>
>> 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.
>
> unfortunately yes
>
>> > 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.
>
> Yes, like that more or less.
>
> I did a chardevice in /dev/ because in our environment debugfs is not mounted
> automatically and we need this feature also operationally. But yes, that's the
> idea.
>
> One comment. The code on github [1] looks like it is assuming that on
> userspace people write the full image in one shot and it is smaller that 4MiB.
> What I did is to build a scatterlist in order to support the following case:
>
> cat image.bin > /sys/kernel/debug/.../image
>
> where `cat` write 4K at time. And it can be bigger than 4M
>
> [1] https://github.com/altera-opensource/linux-socfpga/blob/socfpga-4.17/
> drivers/fpga/fpga-mgr-debugfs.c

I squashed these patches, cleaned up a bit and posted them yesterday.

> Below (end of this email) the patch I quickly did last year. Unfortunately I
> do not have the time to clean this solution, so it is the very first thing I
> implemented without many thoughts (that's why I never pushed this patch).

Your scatter-gather code is much nicer that my debugfs code that needs
the image to be written in one big chunk to be copied.

>
>
>> > 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@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> ------
> From d6a6df9515c5e92cc74b8948c3c10ac9cbeec6d2 Mon Sep 17 00:00:00 2001
> From: Federico Vaga <federico.vaga@vaga.pv.it>
> Date: Mon, 20 Nov 2017 14:40:26 +0100
> Subject: [PATCH] fpga: program from user-space
>
> Signed-off-by: Federico Vaga <federico.vaga@vaga.pv.it>
> ---
>  Documentation/fpga/fpga-mgr.txt |   3 +
>  drivers/fpga/fpga-mgr.c         | 261 ++++++++++++++++++++++++++++++++++++++
> +-
>  include/linux/fpga/fpga-mgr.h   |  19 +++
>  3 files changed, 281 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/fpga/fpga-mgr.txt b/Documentation/fpga/fpga-mgr.txt
> index 78f197f..397dae9 100644
> --- a/Documentation/fpga/fpga-mgr.txt
> +++ b/Documentation/fpga/fpga-mgr.txt
> @@ -197,3 +197,6 @@ to put the FPGA into operating mode.
>  The ops include a .state function which will read the hardware FPGA manager
> and
>  return a code of type enum fpga_mgr_states.  It doesn't result in a change in
>  hardware state.
> +
> +Configuring the FPGA from user-space
> +====================================
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 6441f91..964b7e4 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -27,10 +27,56 @@
>  #include <linux/slab.h>
>  #include <linux/scatterlist.h>
>  #include <linux/highmem.h>
> +#include <linux/spinlock.h>
>  #include <linux/version.h>
> +#include <linux/cdev.h>
> +#include <linux/list.h>
>
>  static DEFINE_IDA(fpga_mgr_ida);
>  static struct class *fpga_mgr_class;
> +static dev_t fpga_mgr_devt;
> +
> +/**
> + * struct fpga_image_chunck - FPGA configuration chunck
> + * @data: chunk data
> + * @size: chunk data size
> + * @list: for the linked list of chunks
> + */
> +struct fpga_image_chunk {
> +       void *data;
> +       size_t size;
> +       struct list_head list;
> +};
> +#define CHUNK_MAX_SIZE (PAGE_SIZE)
> +
> +/**
> + * struct fpga_user_load - structure to handle configuration from user-space
> + * @mgr: pointer to the FPGA manager
> + * @chunk_list: HEAD point of a linked list of FPGA chunks
> + * @n_chunks: number of chunks in the list
> + * @lock: it protects: chunk_list, n_chunks
> + */
> +struct fpga_user_load {
> +       struct fpga_manager *mgr;
> +       struct list_head chunk_list;
> +       unsigned int n_chunks;
> +       struct spinlock lock;
> +};
> +
> +
> +/**
> + * It sets by default a huge timeout for configuration coming from user-space
> + * just to play safe.
> + *
> + * FIXME what about sysfs parameters to adjust it? The flag bit in particular
> + */
> +struct fpga_image_info default_user_info = {
> +       .flags = 0,
> +       .enable_timeout_us = 10000000, /* 10s */
> +       .disable_timeout_us = 10000000, /* 10s */
> +       .config_complete_timeout_us = 10000000, /* 10s */
> +};
> +
>
>  /*
>   * Call the low level driver's write_init function.  This will do the
> @@ -310,6 +356,158 @@ int fpga_mgr_firmware_load(struct fpga_manager *mgr,
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
>
> +
> +static int fpga_mgr_open(struct inode *inode, struct file *file)
> +{
> +       struct fpga_manager *mgr = container_of(inode->i_cdev,
> +                                               struct fpga_manager,
> +                                               cdev);
> +       struct fpga_user_load *usr;
> +       int ret;
> +
> +       /* Allow the user-space programming only if user unlocked the FPGA */
> +       if (!test_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags)) {
> +               dev_info(&mgr->dev, "The FPGA programming is locked\n");
> +               return -EPERM;
> +       }
> +
> +       ret = mutex_trylock(&mgr->usr_mutex);
> +       if (ret == 0)
> +               return -EBUSY;
> +
> +       usr = kzalloc(sizeof(struct fpga_user_load), GFP_KERNEL);
> +       if (!usr) {
> +               ret = -ENOMEM;
> +               goto err_alloc;
> +       }
> +
> +       usr->mgr = mgr;
> +       spin_lock_init(&usr->lock);
> +       INIT_LIST_HEAD(&usr->chunk_list);
> +       file->private_data = usr;
> +
> +       return 0;
> +
> +err_alloc:
> +       spin_lock(&mgr->lock);
> +       clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
> +       spin_unlock(&mgr->lock);
> +       mutex_unlock(&mgr->usr_mutex);
> +       return ret;
> +}
> +
> +
> +static int fpga_mgr_flush(struct file *file, fl_owner_t id)
> +{
> +       struct fpga_user_load *usr = file->private_data;
> +       struct fpga_image_chunk *chunk, *tmp;
> +       struct sg_table sgt;
> +       struct scatterlist *sg;
> +       int err = 0;
> +
> +       if (!usr->n_chunks)
> +               return 0;
> +
> +       err = sg_alloc_table(&sgt, usr->n_chunks, GFP_KERNEL);
> +       if (err)
> +               goto out;
> +
> +       sg = sgt.sgl;
> +       list_for_each_entry(chunk, &usr->chunk_list, list) {
> +               sg_set_buf(sg, chunk->data, chunk->size);
> +               sg = sg_next(sg);
> +               if (!sg)
> +                       break;
> +       }
> +
> +       err = fpga_mgr_buf_load_sg(usr->mgr,
> +                                  &default_user_info,
> +                                  &sgt);
> +       sg_free_table(&sgt);
> +
> +out:
> +       /* Remove all chunks */
> +       spin_lock(&usr->lock);
> +       list_for_each_entry_safe(chunk, tmp, &usr->chunk_list, list) {
> +               list_del(&chunk->list);
> +               kfree(chunk->data);
> +               kfree(chunk);
> +               usr->n_chunks--;
> +       }
> +       spin_unlock(&usr->lock);
> +
> +       return err;
> +}
> +
> +
> +static int fpga_mgr_close(struct inode *inode, struct file *file)
> +{
> +       struct fpga_user_load *usr = file->private_data;
> +       struct fpga_manager *mgr = usr->mgr;
> +
> +       kfree(usr);
> +
> +       spin_lock(&mgr->lock);
> +       clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
> +       spin_unlock(&mgr->lock);
> +
> +       mutex_unlock(&mgr->usr_mutex);
> +
> +       return 0;
> +}
> +
> +
> +static ssize_t fpga_mgr_write(struct file *file, const char __user *buf,
> +                             size_t count, loff_t *offp)
> +{
> +       struct fpga_user_load *usr = file->private_data;
> +       struct fpga_image_chunk *chunk;
> +       int err;
> +
> +       chunk = kmalloc(sizeof(struct fpga_image_chunk), GFP_KERNEL);
> +       if (!chunk)
> +               return -ENOMEM;
> +
> +       chunk->size = count > CHUNK_MAX_SIZE ? CHUNK_MAX_SIZE : count;
> +       chunk->data = kmalloc(chunk->size, GFP_KERNEL);
> +       if (!chunk->data) {
> +               err = -ENOMEM;
> +               goto err_buf_alloc;
> +       }
> +
> +       err = copy_from_user(chunk->data, buf, chunk->size);
> +       if(err)
> +               goto err_buf_copy;
> +
> +       spin_lock(&usr->lock);
> +       list_add_tail(&chunk->list, &usr->chunk_list);
> +       usr->n_chunks++;
> +       spin_unlock(&usr->lock);
> +
> +       *offp += count;
> +
> +       return chunk->size;
> +
> +err_buf_copy:
> +       kfree(chunk->data);
> +err_buf_alloc:
> +       kfree(chunk);
> +       return err;
> +}
> +
> +
> +/**
> + * Char device operation
> + */
> +static const struct file_operations fpga_mgr_fops = {
> +       .owner = THIS_MODULE,
> +       .open = fpga_mgr_open,
> +       .flush = fpga_mgr_flush,
> +       .release = fpga_mgr_close,
> +       .write  = fpga_mgr_write,
> +};
> +
> +
>  static const char * const state_str[] = {
>         [FPGA_MGR_STATE_UNKNOWN] =              "unknown",
>         [FPGA_MGR_STATE_POWER_OFF] =            "power off",
> @@ -352,13 +550,43 @@ static ssize_t state_show(struct device *dev,
>         return sprintf(buf, "%s\n", state_str[mgr->state]);
>  }
>
> +static ssize_t config_lock_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +       if (test_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags))
> +               return sprintf(buf, "unlock\n");
> +       return sprintf(buf, "lock\n");
> +}
> +
> +static ssize_t config_lock_store(struct device *dev,
> +                                struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +       struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +       spin_lock(&mgr->lock);
> +       if (strncmp(buf, "lock" , min(4, (int)count)) == 0)
> +               clear_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
> +       else if (strncmp(buf, "unlock" , min(6, (int)count)) == 0)
> +               set_bit(FPGA_MGR_FLAG_UNLOCK, mgr->flags);
> +       else
> +               count = -EINVAL;
> +       spin_unlock(&mgr->lock);
> +
> +       return count;
> +}
> +
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
>  static DEVICE_ATTR_RO(name);
>  static DEVICE_ATTR_RO(state);
> +static DEVICE_ATTR_RW(config_lock);
>
>  static struct attribute *fpga_mgr_attrs[] = {
>         &dev_attr_name.attr,
>         &dev_attr_state.attr,
> +       &dev_attr_lock.attr,
>         NULL,
>  };
>  ATTRIBUTE_GROUPS(fpga_mgr);
> @@ -367,6 +595,9 @@ ATTRIBUTE_GROUPS(fpga_mgr);
>  static struct device_attribute fpga_mgr_attrs[] = {
>         __ATTR(name, S_IRUGO, name_show, NULL),
>         __ATTR(state, S_IRUGO, state_show, NULL),
> +       __ATTR(config_lock, S_IRUGO | S_IWUSR | S_IRGRP | S_IWGRP,
> +              config_lock_show, config_lock_store),
> +       __ATTR_NULL,
>  };
>  #endif
>
> @@ -507,6 +738,8 @@ int fpga_mgr_register(struct device *dev, const char
> *name,
>         }
>
>         mutex_init(&mgr->ref_mutex);
> +       mutex_init(&mgr->usr_mutex);
> +       spin_lock_init(&mgr->lock);
>
>         mgr->name = name;
>         mgr->mops = mops;
> @@ -524,12 +757,19 @@ int fpga_mgr_register(struct device *dev, const char
> *name,
>         mgr->dev.parent = dev;
>         mgr->dev.of_node = dev->of_node;
>         mgr->dev.id = id;
> +       mgr->dev.devt = MKDEV(MAJOR(fpga_mgr_devt), id);
>         dev_set_drvdata(dev, mgr);
>
>         ret = dev_set_name(&mgr->dev, "fpga%d", id);
>         if (ret)
>                 goto error_device;
>
> +       cdev_init(&mgr->cdev, &fpga_mgr_fops);
> +       mgr->cdev.owner = THIS_MODULE;
> +       ret = cdev_add(&mgr->cdev, mgr->dev.devt, 1);
> +       if (ret)
> +               goto err_cdev;
> +
>         ret = device_add(&mgr->dev);
>         if (ret)
>                 goto error_device;
> @@ -539,6 +779,8 @@ int fpga_mgr_register(struct device *dev, const char
> *name,
>         return 0;
>
>  error_device:
> +       cdev_del(&mgr->cdev);
> +err_cdev:
>         ida_simple_remove(&fpga_mgr_ida, id);
>  error_kfree:
>         kfree(mgr);
> @@ -572,17 +814,27 @@ static void fpga_mgr_dev_release(struct device *dev)
>  {
>         struct fpga_manager *mgr = to_fpga_manager(dev);
>
> +       cdev_del(&mgr->cdev);
>         ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
>         kfree(mgr);
>  }
>
>  static int __init fpga_mgr_class_init(void)
>  {
> +       int err;
> +
>         pr_info("FPGA manager framework\n");
>
> +       err = alloc_chrdev_region(&fpga_mgr_devt, 0, FPGA_MGR_MAX_DEV,
> +                                 "fpga_mgr");
> +       if (err)
> +               return err;
> +
>         fpga_mgr_class = class_create(THIS_MODULE, "fpga_manager");
> -       if (IS_ERR(fpga_mgr_class))
> -               return PTR_ERR(fpga_mgr_class);
> +       if (IS_ERR(fpga_mgr_class)) {
> +               err = PTR_ERR(fpga_mgr_class);
> +               goto err_class;
> +       }
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(3,11,0)
>         fpga_mgr_class->dev_groups = fpga_mgr_groups;
>  #else
> @@ -591,12 +843,17 @@ static int __init fpga_mgr_class_init(void)
>         fpga_mgr_class->dev_release = fpga_mgr_dev_release;
>
>         return 0;
> +
> +err_class:
> +       unregister_chrdev_region(fpga_mgr_devt, FPGA_MGR_MAX_DEV);
> +       return err;
>  }
>
>  static void __exit fpga_mgr_class_exit(void)
>  {
>         class_destroy(fpga_mgr_class);
>         ida_destroy(&fpga_mgr_ida);
> +       unregister_chrdev_region(fpga_mgr_devt, FPGA_MGR_MAX_DEV);
>  }
>
>  MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index bfa14bc..ae38e48 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -15,8 +15,10 @@
>   * You should have received a copy of the GNU General Public License along
> with
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
> +#include <linux/cdev.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/spinlock.h>
>
>  #ifndef _LINUX_FPGA_MGR_H
>  #define _LINUX_FPGA_MGR_H
> @@ -24,6 +26,8 @@
>  struct fpga_manager;
>  struct sg_table;
>
> +#define FPGA_MGR_MAX_DEV (256)
> +
>  /**
>   * enum fpga_mgr_states - fpga framework states
>   * @FPGA_MGR_STATE_UNKNOWN: can't determine state
> @@ -118,22 +122,37 @@ struct fpga_manager_ops {
>         void (*fpga_remove)(struct fpga_manager *mgr);
>  };
>
> +/*
> + * List of status FLAGS for the FPGA manager
> + */
> +#define FPGA_MGR_FLAG_BITS (32)
> +#define FPGA_MGR_FLAG_UNLOCK BIT(0)
> +
>  /**
>   * struct fpga_manager - fpga manager structure
>   * @name: name of low level fpga manager
> + * @cdev: char device interface
>   * @dev: fpga manager device
>   * @ref_mutex: only allows one reference to fpga manager
>   * @state: state of fpga manager
>   * @mops: pointer to struct of fpga manager ops
>   * @priv: low level driver private date
> + * @flags: manager status bits
> + * @lock: it protects: flags
> + * @usr_mutex: only allows one user to program the FPGA
>   */
>  struct fpga_manager {
>         const char *name;
> +       struct cdev cdev;
>         struct device dev;
>         struct mutex ref_mutex;
>         enum fpga_mgr_states state;
>         const struct fpga_manager_ops *mops;
>         void *priv;
> +
> +       DECLARE_BITMAP(flags, FPGA_MGR_FLAG_BITS);
> +       struct spinlock lock;
> +       struct mutex usr_mutex;
>  };
>
>  #define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> --
> 2.15.0
>
>
>
>
>