On Fri, Jun 22, 2018 at 2:53 AM, Federico Vaga <federico.vaga@xxxxxxx> wrote:
Hi Federico,
>> > What is buggy is the function fpga_mgr_get().
>> > That patch has been done to allow multiple FPGA manager instances
>> > to be linked to the same device (PCI it says). But function
>> > fpga_mgr_get() will return only the first found: what about the
>> > others?
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
