Re: fpga: fpga_mgr_get() buggy ?

[Alan Tull <atull@kernel.org>]

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