[RFC,v3,2/2] fpga: set owner of fpga_manager_ops for existing low-level modules
Commit Message
This patch tentatively set the owner field of fpga_manager_ops to
THIS_MODULE for existing fpga manager low-level control modules.
Signed-off-by: Marco Pagani <marpagan@redhat.com>
---
drivers/fpga/altera-cvp.c | 1 +
drivers/fpga/altera-pr-ip-core.c | 1 +
drivers/fpga/altera-ps-spi.c | 1 +
drivers/fpga/dfl-fme-mgr.c | 1 +
drivers/fpga/ice40-spi.c | 1 +
drivers/fpga/lattice-sysconfig.c | 1 +
drivers/fpga/machxo2-spi.c | 1 +
drivers/fpga/microchip-spi.c | 1 +
drivers/fpga/socfpga-a10.c | 1 +
drivers/fpga/socfpga.c | 1 +
drivers/fpga/stratix10-soc.c | 1 +
drivers/fpga/tests/fpga-mgr-test.c | 1 +
drivers/fpga/tests/fpga-region-test.c | 1 +
drivers/fpga/ts73xx-fpga.c | 1 +
drivers/fpga/versal-fpga.c | 1 +
drivers/fpga/xilinx-spi.c | 1 +
drivers/fpga/zynq-fpga.c | 1 +
drivers/fpga/zynqmp-fpga.c | 1 +
18 files changed, 18 insertions(+)
Comments
On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
> This patch tentatively set the owner field of fpga_manager_ops to
> THIS_MODULE for existing fpga manager low-level control modules.
>
> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> ---
> drivers/fpga/altera-cvp.c | 1 +
> drivers/fpga/altera-pr-ip-core.c | 1 +
> drivers/fpga/altera-ps-spi.c | 1 +
> drivers/fpga/dfl-fme-mgr.c | 1 +
> drivers/fpga/ice40-spi.c | 1 +
> drivers/fpga/lattice-sysconfig.c | 1 +
> drivers/fpga/machxo2-spi.c | 1 +
> drivers/fpga/microchip-spi.c | 1 +
> drivers/fpga/socfpga-a10.c | 1 +
> drivers/fpga/socfpga.c | 1 +
> drivers/fpga/stratix10-soc.c | 1 +
> drivers/fpga/tests/fpga-mgr-test.c | 1 +
> drivers/fpga/tests/fpga-region-test.c | 1 +
> drivers/fpga/ts73xx-fpga.c | 1 +
> drivers/fpga/versal-fpga.c | 1 +
> drivers/fpga/xilinx-spi.c | 1 +
> drivers/fpga/zynq-fpga.c | 1 +
> drivers/fpga/zynqmp-fpga.c | 1 +
> 18 files changed, 18 insertions(+)
>
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 4ffb9da537d8..aeb913547dd8 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
> .write_init = altera_cvp_write_init,
> .write = altera_cvp_write,
> .write_complete = altera_cvp_write_complete,
> + .owner = THIS_MODULE,
Note, this is not how to do this, force the compiler to set this for you
automatically, otherwise everyone will always forget to do it. Look at
how functions like usb_register_driver() works.
Also, are you _sure_ that you need a module owner in this structure? I
still don't know why...
thanks,
greg k-h
On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
>> This patch tentatively set the owner field of fpga_manager_ops to
>> THIS_MODULE for existing fpga manager low-level control modules.
>>
>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>> ---
>> drivers/fpga/altera-cvp.c | 1 +
>> drivers/fpga/altera-pr-ip-core.c | 1 +
>> drivers/fpga/altera-ps-spi.c | 1 +
>> drivers/fpga/dfl-fme-mgr.c | 1 +
>> drivers/fpga/ice40-spi.c | 1 +
>> drivers/fpga/lattice-sysconfig.c | 1 +
>> drivers/fpga/machxo2-spi.c | 1 +
>> drivers/fpga/microchip-spi.c | 1 +
>> drivers/fpga/socfpga-a10.c | 1 +
>> drivers/fpga/socfpga.c | 1 +
>> drivers/fpga/stratix10-soc.c | 1 +
>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
>> drivers/fpga/tests/fpga-region-test.c | 1 +
>> drivers/fpga/ts73xx-fpga.c | 1 +
>> drivers/fpga/versal-fpga.c | 1 +
>> drivers/fpga/xilinx-spi.c | 1 +
>> drivers/fpga/zynq-fpga.c | 1 +
>> drivers/fpga/zynqmp-fpga.c | 1 +
>> 18 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>> index 4ffb9da537d8..aeb913547dd8 100644
>> --- a/drivers/fpga/altera-cvp.c
>> +++ b/drivers/fpga/altera-cvp.c
>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
>> .write_init = altera_cvp_write_init,
>> .write = altera_cvp_write,
>> .write_complete = altera_cvp_write_complete,
>> + .owner = THIS_MODULE,
>
> Note, this is not how to do this, force the compiler to set this for you
> automatically, otherwise everyone will always forget to do it. Look at
> how functions like usb_register_driver() works.
>
> Also, are you _sure_ that you need a module owner in this structure? I
> still don't know why...
>
Do you mean moving the module owner field to the manager context and setting
it during registration with a helper macro?
Something like:
struct fpga_manager {
...
struct module *owner;
};
#define fpga_mgr_register(parent, ...) \
__fpga_mgr_register(parent,..., THIS_MODULE)
struct fpga_manager *
__fpga_mgr_register(struct device *parent, ..., struct module *owner)
{
...
mgr->owner = owner;
}
Thanks,
Marco
On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
>
>
> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
> > On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
> >> This patch tentatively set the owner field of fpga_manager_ops to
> >> THIS_MODULE for existing fpga manager low-level control modules.
> >>
> >> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >> ---
> >> drivers/fpga/altera-cvp.c | 1 +
> >> drivers/fpga/altera-pr-ip-core.c | 1 +
> >> drivers/fpga/altera-ps-spi.c | 1 +
> >> drivers/fpga/dfl-fme-mgr.c | 1 +
> >> drivers/fpga/ice40-spi.c | 1 +
> >> drivers/fpga/lattice-sysconfig.c | 1 +
> >> drivers/fpga/machxo2-spi.c | 1 +
> >> drivers/fpga/microchip-spi.c | 1 +
> >> drivers/fpga/socfpga-a10.c | 1 +
> >> drivers/fpga/socfpga.c | 1 +
> >> drivers/fpga/stratix10-soc.c | 1 +
> >> drivers/fpga/tests/fpga-mgr-test.c | 1 +
> >> drivers/fpga/tests/fpga-region-test.c | 1 +
> >> drivers/fpga/ts73xx-fpga.c | 1 +
> >> drivers/fpga/versal-fpga.c | 1 +
> >> drivers/fpga/xilinx-spi.c | 1 +
> >> drivers/fpga/zynq-fpga.c | 1 +
> >> drivers/fpga/zynqmp-fpga.c | 1 +
> >> 18 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> >> index 4ffb9da537d8..aeb913547dd8 100644
> >> --- a/drivers/fpga/altera-cvp.c
> >> +++ b/drivers/fpga/altera-cvp.c
> >> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
> >> .write_init = altera_cvp_write_init,
> >> .write = altera_cvp_write,
> >> .write_complete = altera_cvp_write_complete,
> >> + .owner = THIS_MODULE,
> >
> > Note, this is not how to do this, force the compiler to set this for you
> > automatically, otherwise everyone will always forget to do it. Look at
> > how functions like usb_register_driver() works.
> >
> > Also, are you _sure_ that you need a module owner in this structure? I
> > still don't know why...
> >
>
> Do you mean moving the module owner field to the manager context and setting
> it during registration with a helper macro?
I mean set it during registration with a helper macro.
> Something like:
>
> struct fpga_manager {
> ...
> struct module *owner;
> };
>
> #define fpga_mgr_register(parent, ...) \
> __fpga_mgr_register(parent,..., THIS_MODULE)
>
> struct fpga_manager *
> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
> {
> ...
> mgr->owner = owner;
> }
Yes.
But again, is a module owner even needed? I don't think you all have
proven that yet...
thanks,
greg k-h
On 2023-12-19 16:10, Greg Kroah-Hartman wrote:
> On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
>>
>>
>> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
>>> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
>>>> This patch tentatively set the owner field of fpga_manager_ops to
>>>> THIS_MODULE for existing fpga manager low-level control modules.
>>>>
>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>> ---
>>>> drivers/fpga/altera-cvp.c | 1 +
>>>> drivers/fpga/altera-pr-ip-core.c | 1 +
>>>> drivers/fpga/altera-ps-spi.c | 1 +
>>>> drivers/fpga/dfl-fme-mgr.c | 1 +
>>>> drivers/fpga/ice40-spi.c | 1 +
>>>> drivers/fpga/lattice-sysconfig.c | 1 +
>>>> drivers/fpga/machxo2-spi.c | 1 +
>>>> drivers/fpga/microchip-spi.c | 1 +
>>>> drivers/fpga/socfpga-a10.c | 1 +
>>>> drivers/fpga/socfpga.c | 1 +
>>>> drivers/fpga/stratix10-soc.c | 1 +
>>>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
>>>> drivers/fpga/tests/fpga-region-test.c | 1 +
>>>> drivers/fpga/ts73xx-fpga.c | 1 +
>>>> drivers/fpga/versal-fpga.c | 1 +
>>>> drivers/fpga/xilinx-spi.c | 1 +
>>>> drivers/fpga/zynq-fpga.c | 1 +
>>>> drivers/fpga/zynqmp-fpga.c | 1 +
>>>> 18 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>>>> index 4ffb9da537d8..aeb913547dd8 100644
>>>> --- a/drivers/fpga/altera-cvp.c
>>>> +++ b/drivers/fpga/altera-cvp.c
>>>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
>>>> .write_init = altera_cvp_write_init,
>>>> .write = altera_cvp_write,
>>>> .write_complete = altera_cvp_write_complete,
>>>> + .owner = THIS_MODULE,
>>>
>>> Note, this is not how to do this, force the compiler to set this for you
>>> automatically, otherwise everyone will always forget to do it. Look at
>>> how functions like usb_register_driver() works.
>>>
>>> Also, are you _sure_ that you need a module owner in this structure? I
>>> still don't know why...
>>>
>>
>> Do you mean moving the module owner field to the manager context and setting
>> it during registration with a helper macro?
>
> I mean set it during registration with a helper macro.
>
>> Something like:
>>
>> struct fpga_manager {
>> ...
>> struct module *owner;
>> };
>>
>> #define fpga_mgr_register(parent, ...) \
>> __fpga_mgr_register(parent,..., THIS_MODULE)
>>
>> struct fpga_manager *
>> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
>> {
>> ...
>> mgr->owner = owner;
>> }
>
> Yes.
>
> But again, is a module owner even needed? I don't think you all have
> proven that yet...
Programming an FPGA involves a potentially lengthy sequence of interactions
with the reconfiguration engine. The manager conceptually organizes these
interactions as a sequence of ops. Low-level modules implement these ops/steps
for a specific device. If we don't protect the low-level module, someone might
unload it right when we are in the middle of a low-level op programming the
FPGA. As far as I know, the kernel would crash in that case.
Thanks,
Marco
On Tue, Dec 19, 2023 at 06:17:20PM +0100, Marco Pagani wrote:
>
> On 2023-12-19 16:10, Greg Kroah-Hartman wrote:
> > On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
> >>
> >>
> >> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
> >>> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
> >>>> This patch tentatively set the owner field of fpga_manager_ops to
> >>>> THIS_MODULE for existing fpga manager low-level control modules.
> >>>>
> >>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >>>> ---
> >>>> drivers/fpga/altera-cvp.c | 1 +
> >>>> drivers/fpga/altera-pr-ip-core.c | 1 +
> >>>> drivers/fpga/altera-ps-spi.c | 1 +
> >>>> drivers/fpga/dfl-fme-mgr.c | 1 +
> >>>> drivers/fpga/ice40-spi.c | 1 +
> >>>> drivers/fpga/lattice-sysconfig.c | 1 +
> >>>> drivers/fpga/machxo2-spi.c | 1 +
> >>>> drivers/fpga/microchip-spi.c | 1 +
> >>>> drivers/fpga/socfpga-a10.c | 1 +
> >>>> drivers/fpga/socfpga.c | 1 +
> >>>> drivers/fpga/stratix10-soc.c | 1 +
> >>>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
> >>>> drivers/fpga/tests/fpga-region-test.c | 1 +
> >>>> drivers/fpga/ts73xx-fpga.c | 1 +
> >>>> drivers/fpga/versal-fpga.c | 1 +
> >>>> drivers/fpga/xilinx-spi.c | 1 +
> >>>> drivers/fpga/zynq-fpga.c | 1 +
> >>>> drivers/fpga/zynqmp-fpga.c | 1 +
> >>>> 18 files changed, 18 insertions(+)
> >>>>
> >>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> >>>> index 4ffb9da537d8..aeb913547dd8 100644
> >>>> --- a/drivers/fpga/altera-cvp.c
> >>>> +++ b/drivers/fpga/altera-cvp.c
> >>>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
> >>>> .write_init = altera_cvp_write_init,
> >>>> .write = altera_cvp_write,
> >>>> .write_complete = altera_cvp_write_complete,
> >>>> + .owner = THIS_MODULE,
> >>>
> >>> Note, this is not how to do this, force the compiler to set this for you
> >>> automatically, otherwise everyone will always forget to do it. Look at
> >>> how functions like usb_register_driver() works.
> >>>
> >>> Also, are you _sure_ that you need a module owner in this structure? I
> >>> still don't know why...
> >>>
> >>
> >> Do you mean moving the module owner field to the manager context and setting
> >> it during registration with a helper macro?
> >
> > I mean set it during registration with a helper macro.
> >
> >> Something like:
> >>
> >> struct fpga_manager {
> >> ...
> >> struct module *owner;
> >> };
> >>
> >> #define fpga_mgr_register(parent, ...) \
> >> __fpga_mgr_register(parent,..., THIS_MODULE)
> >>
> >> struct fpga_manager *
> >> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
> >> {
> >> ...
> >> mgr->owner = owner;
> >> }
> >
> > Yes.
> >
> > But again, is a module owner even needed? I don't think you all have
> > proven that yet...
>
> Programming an FPGA involves a potentially lengthy sequence of interactions
> with the reconfiguration engine. The manager conceptually organizes these
> interactions as a sequence of ops. Low-level modules implement these ops/steps
> for a specific device. If we don't protect the low-level module, someone might
> unload it right when we are in the middle of a low-level op programming the
> FPGA. As far as I know, the kernel would crash in that case.
The only way an unload of a module can happen is if a user explicitly
asks for it to be unloaded. So they get what they ask for, right?
How do you "know" it is active? And why doesn't the normal
"driver/device" bindings prevent unloading from being a problem? When
you unload a module, you stop all ops on the driver, and then unregister
it, which causes any future ones to fail.
Or am I missing something here?
thanks,
greg k-h
On 19/12/23 19:11, Greg Kroah-Hartman wrote:
> On Tue, Dec 19, 2023 at 06:17:20PM +0100, Marco Pagani wrote:
>>
>> On 2023-12-19 16:10, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
>>>>
>>>>
>>>> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
>>>>> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
>>>>>> This patch tentatively set the owner field of fpga_manager_ops to
>>>>>> THIS_MODULE for existing fpga manager low-level control modules.
>>>>>>
>>>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>>>> ---
>>>>>> drivers/fpga/altera-cvp.c | 1 +
>>>>>> drivers/fpga/altera-pr-ip-core.c | 1 +
>>>>>> drivers/fpga/altera-ps-spi.c | 1 +
>>>>>> drivers/fpga/dfl-fme-mgr.c | 1 +
>>>>>> drivers/fpga/ice40-spi.c | 1 +
>>>>>> drivers/fpga/lattice-sysconfig.c | 1 +
>>>>>> drivers/fpga/machxo2-spi.c | 1 +
>>>>>> drivers/fpga/microchip-spi.c | 1 +
>>>>>> drivers/fpga/socfpga-a10.c | 1 +
>>>>>> drivers/fpga/socfpga.c | 1 +
>>>>>> drivers/fpga/stratix10-soc.c | 1 +
>>>>>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
>>>>>> drivers/fpga/tests/fpga-region-test.c | 1 +
>>>>>> drivers/fpga/ts73xx-fpga.c | 1 +
>>>>>> drivers/fpga/versal-fpga.c | 1 +
>>>>>> drivers/fpga/xilinx-spi.c | 1 +
>>>>>> drivers/fpga/zynq-fpga.c | 1 +
>>>>>> drivers/fpga/zynqmp-fpga.c | 1 +
>>>>>> 18 files changed, 18 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>>>>>> index 4ffb9da537d8..aeb913547dd8 100644
>>>>>> --- a/drivers/fpga/altera-cvp.c
>>>>>> +++ b/drivers/fpga/altera-cvp.c
>>>>>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
>>>>>> .write_init = altera_cvp_write_init,
>>>>>> .write = altera_cvp_write,
>>>>>> .write_complete = altera_cvp_write_complete,
>>>>>> + .owner = THIS_MODULE,
>>>>>
>>>>> Note, this is not how to do this, force the compiler to set this for you
>>>>> automatically, otherwise everyone will always forget to do it. Look at
>>>>> how functions like usb_register_driver() works.
>>>>>
>>>>> Also, are you _sure_ that you need a module owner in this structure? I
>>>>> still don't know why...
>>>>>
>>>>
>>>> Do you mean moving the module owner field to the manager context and setting
>>>> it during registration with a helper macro?
>>>
>>> I mean set it during registration with a helper macro.
>>>
>>>> Something like:
>>>>
>>>> struct fpga_manager {
>>>> ...
>>>> struct module *owner;
>>>> };
>>>>
>>>> #define fpga_mgr_register(parent, ...) \
>>>> __fpga_mgr_register(parent,..., THIS_MODULE)
>>>>
>>>> struct fpga_manager *
>>>> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
>>>> {
>>>> ...
>>>> mgr->owner = owner;
>>>> }
>>>
>>> Yes.
>>>
>>> But again, is a module owner even needed? I don't think you all have
>>> proven that yet...
>>
>> Programming an FPGA involves a potentially lengthy sequence of interactions
>> with the reconfiguration engine. The manager conceptually organizes these
>> interactions as a sequence of ops. Low-level modules implement these ops/steps
>> for a specific device. If we don't protect the low-level module, someone might
>> unload it right when we are in the middle of a low-level op programming the
>> FPGA. As far as I know, the kernel would crash in that case.
>
> The only way an unload of a module can happen is if a user explicitly
> asks for it to be unloaded. So they get what they ask for, right?
>
Right, the user should get what he asked for, including hanging the
hardware. My only concern is that the kernel should not crash.
> How do you "know" it is active? And why doesn't the normal
> "driver/device" bindings prevent unloading from being a problem? When
> you unload a module, you stop all ops on the driver, and then unregister
> it, which causes any future ones to fail.
>
> Or am I missing something here?
>
I think the problem is that the ops are not directly tied to the driver
of the manager's parent device. It is not even required to have a driver
to register a manager. The only way to know if the fpga manager is
active (i.e., someone is running one op) is by poking manager->state.
One possibility that comes into my mind, excluding a major reworking,
is waiting in fpga_mgr_unregister() until the manager reaches a steady
state (no ops are running) before unregistering the device. However, it
feels questionable because if one of the ops hangs, the module removal
will also hang.
Thanks,
Marco
On Wed, Dec 20, 2023 at 11:24:20PM +0100, Marco Pagani wrote:
>
>
> On 19/12/23 19:11, Greg Kroah-Hartman wrote:
> > On Tue, Dec 19, 2023 at 06:17:20PM +0100, Marco Pagani wrote:
> >>
> >> On 2023-12-19 16:10, Greg Kroah-Hartman wrote:
> >>> On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
> >>>>
> >>>>
> >>>> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
> >>>>> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
> >>>>>> This patch tentatively set the owner field of fpga_manager_ops to
> >>>>>> THIS_MODULE for existing fpga manager low-level control modules.
> >>>>>>
> >>>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> >>>>>> ---
> >>>>>> drivers/fpga/altera-cvp.c | 1 +
> >>>>>> drivers/fpga/altera-pr-ip-core.c | 1 +
> >>>>>> drivers/fpga/altera-ps-spi.c | 1 +
> >>>>>> drivers/fpga/dfl-fme-mgr.c | 1 +
> >>>>>> drivers/fpga/ice40-spi.c | 1 +
> >>>>>> drivers/fpga/lattice-sysconfig.c | 1 +
> >>>>>> drivers/fpga/machxo2-spi.c | 1 +
> >>>>>> drivers/fpga/microchip-spi.c | 1 +
> >>>>>> drivers/fpga/socfpga-a10.c | 1 +
> >>>>>> drivers/fpga/socfpga.c | 1 +
> >>>>>> drivers/fpga/stratix10-soc.c | 1 +
> >>>>>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
> >>>>>> drivers/fpga/tests/fpga-region-test.c | 1 +
> >>>>>> drivers/fpga/ts73xx-fpga.c | 1 +
> >>>>>> drivers/fpga/versal-fpga.c | 1 +
> >>>>>> drivers/fpga/xilinx-spi.c | 1 +
> >>>>>> drivers/fpga/zynq-fpga.c | 1 +
> >>>>>> drivers/fpga/zynqmp-fpga.c | 1 +
> >>>>>> 18 files changed, 18 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> >>>>>> index 4ffb9da537d8..aeb913547dd8 100644
> >>>>>> --- a/drivers/fpga/altera-cvp.c
> >>>>>> +++ b/drivers/fpga/altera-cvp.c
> >>>>>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
> >>>>>> .write_init = altera_cvp_write_init,
> >>>>>> .write = altera_cvp_write,
> >>>>>> .write_complete = altera_cvp_write_complete,
> >>>>>> + .owner = THIS_MODULE,
> >>>>>
> >>>>> Note, this is not how to do this, force the compiler to set this for you
> >>>>> automatically, otherwise everyone will always forget to do it. Look at
> >>>>> how functions like usb_register_driver() works.
> >>>>>
> >>>>> Also, are you _sure_ that you need a module owner in this structure? I
> >>>>> still don't know why...
> >>>>>
> >>>>
> >>>> Do you mean moving the module owner field to the manager context and setting
> >>>> it during registration with a helper macro?
> >>>
> >>> I mean set it during registration with a helper macro.
> >>>
> >>>> Something like:
> >>>>
> >>>> struct fpga_manager {
> >>>> ...
> >>>> struct module *owner;
> >>>> };
> >>>>
> >>>> #define fpga_mgr_register(parent, ...) \
> >>>> __fpga_mgr_register(parent,..., THIS_MODULE)
> >>>>
> >>>> struct fpga_manager *
> >>>> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
> >>>> {
> >>>> ...
> >>>> mgr->owner = owner;
> >>>> }
> >>>
> >>> Yes.
> >>>
> >>> But again, is a module owner even needed? I don't think you all have
> >>> proven that yet...
> >>
> >> Programming an FPGA involves a potentially lengthy sequence of interactions
> >> with the reconfiguration engine. The manager conceptually organizes these
> >> interactions as a sequence of ops. Low-level modules implement these ops/steps
> >> for a specific device. If we don't protect the low-level module, someone might
> >> unload it right when we are in the middle of a low-level op programming the
> >> FPGA. As far as I know, the kernel would crash in that case.
> >
> > The only way an unload of a module can happen is if a user explicitly
> > asks for it to be unloaded. So they get what they ask for, right?
> >
>
> Right, the user should get what he asked for, including hanging the
> hardware. My only concern is that the kernel should not crash.
>
> > How do you "know" it is active? And why doesn't the normal
> > "driver/device" bindings prevent unloading from being a problem? When
> > you unload a module, you stop all ops on the driver, and then unregister
> > it, which causes any future ones to fail.
> >
> > Or am I missing something here?
> >
>
> I think the problem is that the ops are not directly tied to the driver
> of the manager's parent device.
Then that needs to be fixed right there, as that is obviously not using
the driver model properly.
Why aren't the "ops" a driver that is bound to this device? If it is
the one responsible for controlling it, then it should be a driver and
as such, the driver model logic will handle things if/when a module is
unloaded to tear things down better.
> It is not even required to have a driver
> to register a manager. The only way to know if the fpga manager is
> active (i.e., someone is running one op) is by poking manager->state.
That too seems wrong, why is this?
> One possibility that comes into my mind, excluding a major reworking,
> is waiting in fpga_mgr_unregister() until the manager reaches a steady
> state (no ops are running) before unregistering the device. However, it
> feels questionable because if one of the ops hangs, the module removal
> will also hang.
You never know when a new operand will come in, so there's no way to
know "all is quiet", sorry.
Try fixing this properly, buy using the driver model correctly, that
should help resolve these issues automatically instead of hacked up
module reference count attempts.
Remember, this is the whole reason why the driver model was created all
those 20+ years ago, to move away from these module reference count
issues, let's not forget history please.
thanks,
greg k-h
On Tue, Dec 19, 2023 at 07:11:09PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Dec 19, 2023 at 06:17:20PM +0100, Marco Pagani wrote:
> >
> > On 2023-12-19 16:10, Greg Kroah-Hartman wrote:
> > > On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
> > >>
> > >>
> > >> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
> > >>> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
> > >>>> This patch tentatively set the owner field of fpga_manager_ops to
> > >>>> THIS_MODULE for existing fpga manager low-level control modules.
> > >>>>
> > >>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
> > >>>> ---
> > >>>> drivers/fpga/altera-cvp.c | 1 +
> > >>>> drivers/fpga/altera-pr-ip-core.c | 1 +
> > >>>> drivers/fpga/altera-ps-spi.c | 1 +
> > >>>> drivers/fpga/dfl-fme-mgr.c | 1 +
> > >>>> drivers/fpga/ice40-spi.c | 1 +
> > >>>> drivers/fpga/lattice-sysconfig.c | 1 +
> > >>>> drivers/fpga/machxo2-spi.c | 1 +
> > >>>> drivers/fpga/microchip-spi.c | 1 +
> > >>>> drivers/fpga/socfpga-a10.c | 1 +
> > >>>> drivers/fpga/socfpga.c | 1 +
> > >>>> drivers/fpga/stratix10-soc.c | 1 +
> > >>>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
> > >>>> drivers/fpga/tests/fpga-region-test.c | 1 +
> > >>>> drivers/fpga/ts73xx-fpga.c | 1 +
> > >>>> drivers/fpga/versal-fpga.c | 1 +
> > >>>> drivers/fpga/xilinx-spi.c | 1 +
> > >>>> drivers/fpga/zynq-fpga.c | 1 +
> > >>>> drivers/fpga/zynqmp-fpga.c | 1 +
> > >>>> 18 files changed, 18 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> > >>>> index 4ffb9da537d8..aeb913547dd8 100644
> > >>>> --- a/drivers/fpga/altera-cvp.c
> > >>>> +++ b/drivers/fpga/altera-cvp.c
> > >>>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
> > >>>> .write_init = altera_cvp_write_init,
> > >>>> .write = altera_cvp_write,
> > >>>> .write_complete = altera_cvp_write_complete,
> > >>>> + .owner = THIS_MODULE,
> > >>>
> > >>> Note, this is not how to do this, force the compiler to set this for you
> > >>> automatically, otherwise everyone will always forget to do it. Look at
> > >>> how functions like usb_register_driver() works.
> > >>>
> > >>> Also, are you _sure_ that you need a module owner in this structure? I
> > >>> still don't know why...
> > >>>
> > >>
> > >> Do you mean moving the module owner field to the manager context and setting
> > >> it during registration with a helper macro?
> > >
> > > I mean set it during registration with a helper macro.
> > >
> > >> Something like:
> > >>
> > >> struct fpga_manager {
> > >> ...
> > >> struct module *owner;
> > >> };
> > >>
> > >> #define fpga_mgr_register(parent, ...) \
> > >> __fpga_mgr_register(parent,..., THIS_MODULE)
> > >>
> > >> struct fpga_manager *
> > >> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
> > >> {
> > >> ...
> > >> mgr->owner = owner;
> > >> }
> > >
> > > Yes.
> > >
> > > But again, is a module owner even needed? I don't think you all have
> > > proven that yet...
> >
> > Programming an FPGA involves a potentially lengthy sequence of interactions
> > with the reconfiguration engine. The manager conceptually organizes these
> > interactions as a sequence of ops. Low-level modules implement these ops/steps
> > for a specific device. If we don't protect the low-level module, someone might
> > unload it right when we are in the middle of a low-level op programming the
> > FPGA. As far as I know, the kernel would crash in that case.
>
> The only way an unload of a module can happen is if a user explicitly
> asks for it to be unloaded. So they get what they ask for, right?
We have discussed this before [1]. The conclusion is kernel developers can
prevent user module unloading, as long as it doesn't make things
complex. Actually some fundamental subsystems do care about module
unloading. And I assume this patch doesn't make a complex fix.
[1] https://lore.kernel.org/linux-fpga/2023110922-lurk-subdivide-4962@gregkh/
>
> How do you "know" it is active? And why doesn't the normal
The FPGA core manages the reprogramming flow and knows if device is
active. FPGA core will grab the low-level driver module until
reprograming is finished.
> "driver/device" bindings prevent unloading from being a problem? When
> you unload a module, you stop all ops on the driver, and then unregister
> it, which causes any future ones to fail.
This is also discussed [2]. It is still about user explicit module
unloading. The low-level driver module on its own has no way to
garantee its own code page of callbacks not accessed. It *is*
accessing its code page when it tries (to release) any protection. [3]
Core code which calls into it must help, and something like
file_operations.owner is an effective way. This is why a struct
module *owner is introduced for core code to provide help.
[2] https://lore.kernel.org/linux-fpga/2023110918-showroom-choosy-ad14@gregkh/
[3] https://lore.kernel.org/all/20231010003746.GN800259@ZenIV/
Thanks,
Yilun
>
> Or am I missing something here?
>
> thanks,
>
> greg k-h
>
On 2023-12-21 09:22, Greg Kroah-Hartman wrote:
> On Wed, Dec 20, 2023 at 11:24:20PM +0100, Marco Pagani wrote:
>>
>>
>> On 19/12/23 19:11, Greg Kroah-Hartman wrote:
>>> On Tue, Dec 19, 2023 at 06:17:20PM +0100, Marco Pagani wrote:
>>>>
>>>> On 2023-12-19 16:10, Greg Kroah-Hartman wrote:
>>>>> On Tue, Dec 19, 2023 at 03:54:25PM +0100, Marco Pagani wrote:
>>>>>>
>>>>>>
>>>>>> On 2023-12-18 21:33, Greg Kroah-Hartman wrote:
>>>>>>> On Mon, Dec 18, 2023 at 09:28:09PM +0100, Marco Pagani wrote:
>>>>>>>> This patch tentatively set the owner field of fpga_manager_ops to
>>>>>>>> THIS_MODULE for existing fpga manager low-level control modules.
>>>>>>>>
>>>>>>>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>>>>>>>> ---
>>>>>>>> drivers/fpga/altera-cvp.c | 1 +
>>>>>>>> drivers/fpga/altera-pr-ip-core.c | 1 +
>>>>>>>> drivers/fpga/altera-ps-spi.c | 1 +
>>>>>>>> drivers/fpga/dfl-fme-mgr.c | 1 +
>>>>>>>> drivers/fpga/ice40-spi.c | 1 +
>>>>>>>> drivers/fpga/lattice-sysconfig.c | 1 +
>>>>>>>> drivers/fpga/machxo2-spi.c | 1 +
>>>>>>>> drivers/fpga/microchip-spi.c | 1 +
>>>>>>>> drivers/fpga/socfpga-a10.c | 1 +
>>>>>>>> drivers/fpga/socfpga.c | 1 +
>>>>>>>> drivers/fpga/stratix10-soc.c | 1 +
>>>>>>>> drivers/fpga/tests/fpga-mgr-test.c | 1 +
>>>>>>>> drivers/fpga/tests/fpga-region-test.c | 1 +
>>>>>>>> drivers/fpga/ts73xx-fpga.c | 1 +
>>>>>>>> drivers/fpga/versal-fpga.c | 1 +
>>>>>>>> drivers/fpga/xilinx-spi.c | 1 +
>>>>>>>> drivers/fpga/zynq-fpga.c | 1 +
>>>>>>>> drivers/fpga/zynqmp-fpga.c | 1 +
>>>>>>>> 18 files changed, 18 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>>>>>>>> index 4ffb9da537d8..aeb913547dd8 100644
>>>>>>>> --- a/drivers/fpga/altera-cvp.c
>>>>>>>> +++ b/drivers/fpga/altera-cvp.c
>>>>>>>> @@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
>>>>>>>> .write_init = altera_cvp_write_init,
>>>>>>>> .write = altera_cvp_write,
>>>>>>>> .write_complete = altera_cvp_write_complete,
>>>>>>>> + .owner = THIS_MODULE,
>>>>>>>
>>>>>>> Note, this is not how to do this, force the compiler to set this for you
>>>>>>> automatically, otherwise everyone will always forget to do it. Look at
>>>>>>> how functions like usb_register_driver() works.
>>>>>>>
>>>>>>> Also, are you _sure_ that you need a module owner in this structure? I
>>>>>>> still don't know why...
>>>>>>>
>>>>>>
>>>>>> Do you mean moving the module owner field to the manager context and setting
>>>>>> it during registration with a helper macro?
>>>>>
>>>>> I mean set it during registration with a helper macro.
>>>>>
>>>>>> Something like:
>>>>>>
>>>>>> struct fpga_manager {
>>>>>> ...
>>>>>> struct module *owner;
>>>>>> };
>>>>>>
>>>>>> #define fpga_mgr_register(parent, ...) \
>>>>>> __fpga_mgr_register(parent,..., THIS_MODULE)
>>>>>>
>>>>>> struct fpga_manager *
>>>>>> __fpga_mgr_register(struct device *parent, ..., struct module *owner)
>>>>>> {
>>>>>> ...
>>>>>> mgr->owner = owner;
>>>>>> }
>>>>>
>>>>> Yes.
>>>>>
>>>>> But again, is a module owner even needed? I don't think you all have
>>>>> proven that yet...
>>>>
>>>> Programming an FPGA involves a potentially lengthy sequence of interactions
>>>> with the reconfiguration engine. The manager conceptually organizes these
>>>> interactions as a sequence of ops. Low-level modules implement these ops/steps
>>>> for a specific device. If we don't protect the low-level module, someone might
>>>> unload it right when we are in the middle of a low-level op programming the
>>>> FPGA. As far as I know, the kernel would crash in that case.
>>>
>>> The only way an unload of a module can happen is if a user explicitly
>>> asks for it to be unloaded. So they get what they ask for, right?
>>>
>>
>> Right, the user should get what he asked for, including hanging the
>> hardware. My only concern is that the kernel should not crash.
>>
>>> How do you "know" it is active? And why doesn't the normal
>>> "driver/device" bindings prevent unloading from being a problem? When
>>> you unload a module, you stop all ops on the driver, and then unregister
>>> it, which causes any future ones to fail.
>>>
>>> Or am I missing something here?
>>>
>>
>> I think the problem is that the ops are not directly tied to the driver
>> of the manager's parent device.
>
> Then that needs to be fixed right there, as that is obviously not using
> the driver model properly.
>
> Why aren't the "ops" a driver that is bound to this device? If it is
> the one responsible for controlling it, then it should be a driver and
> as such, the driver model logic will handle things if/when a module is
> unloaded to tear things down better.
>
>> It is not even required to have a driver
>> to register a manager. The only way to know if the fpga manager is
>> active (i.e., someone is running one op) is by poking manager->state.
>
> That too seems wrong, why is this?
I don't know. I was not around when the fpga subsystem was laid down.
>
>> One possibility that comes into my mind, excluding a major reworking,
>> is waiting in fpga_mgr_unregister() until the manager reaches a steady
>> state (no ops are running) before unregistering the device. However, it
>> feels questionable because if one of the ops hangs, the module removal
>> will also hang.
>
> You never know when a new operand will come in, so there's no way to
> know "all is quiet", sorry.
>
> Try fixing this properly, buy using the driver model correctly, that
> should help resolve these issues automatically instead of hacked up
> module reference count attempts.
>
> Remember, this is the whole reason why the driver model was created all
> those 20+ years ago, to move away from these module reference count
> issues, let's not forget history please.
>
I do not entirely understand this part. The subsystem only provides an
in-kernel API for programming the fpga that in-kernel consumers can use.
The ops that the low-level module implements are used only internally by
the manager in a predefined order.
There is no standard interface for programming the fpga exposed to
userspace using file_operations or attributes exported via sysfs.
The manager only exports read-only attributes for status. On top
of that, there is only the support for device tree overlays.
Would it be correct to assume that the responsibility of keeping
the low-level module in while programming the fpga is on the kernel
component that consumes the subsystem's in-kernel API and (eventually)
exports a programming interface to userspace?
If we consider the case where the programming is done through a
userspace interface exported by the same module that implements the ops,
then we should be good even without taking the low-level module in the
manager.
However, I guess the decision to take the low-level module in the
manager was meant to address the case where the module implementing the
ops and the consumer of the in-kernel API (that may optionally export a
userspace interface for programming) are two separate entities.
Thanks,
Marco
@@ -520,6 +520,7 @@ static const struct fpga_manager_ops altera_cvp_ops = {
.write_init = altera_cvp_write_init,
.write = altera_cvp_write,
.write_complete = altera_cvp_write_complete,
+ .owner = THIS_MODULE,
};
static const struct cvp_priv cvp_priv_v1 = {
@@ -171,6 +171,7 @@ static const struct fpga_manager_ops alt_pr_ops = {
.write_init = alt_pr_fpga_write_init,
.write = alt_pr_fpga_write,
.write_complete = alt_pr_fpga_write_complete,
+ .owner = THIS_MODULE,
};
int alt_pr_register(struct device *dev, void __iomem *reg_base)
@@ -228,6 +228,7 @@ static const struct fpga_manager_ops altera_ps_ops = {
.write_init = altera_ps_write_init,
.write = altera_ps_write,
.write_complete = altera_ps_write_complete,
+ .owner = THIS_MODULE,
};
static int altera_ps_probe(struct spi_device *spi)
@@ -264,6 +264,7 @@ static const struct fpga_manager_ops fme_mgr_ops = {
.write = fme_mgr_write,
.write_complete = fme_mgr_write_complete,
.status = fme_mgr_status,
+ .owner = THIS_MODULE,
};
static void fme_mgr_get_compat_id(void __iomem *fme_pr,
@@ -130,6 +130,7 @@ static const struct fpga_manager_ops ice40_fpga_ops = {
.write_init = ice40_fpga_ops_write_init,
.write = ice40_fpga_ops_write,
.write_complete = ice40_fpga_ops_write_complete,
+ .owner = THIS_MODULE,
};
static int ice40_fpga_probe(struct spi_device *spi)
@@ -348,6 +348,7 @@ static const struct fpga_manager_ops sysconfig_fpga_mgr_ops = {
.write_init = sysconfig_ops_write_init,
.write = sysconfig_ops_write,
.write_complete = sysconfig_ops_write_complete,
+ .owner = THIS_MODULE,
};
int sysconfig_probe(struct sysconfig_priv *priv)
@@ -358,6 +358,7 @@ static const struct fpga_manager_ops machxo2_ops = {
.write_init = machxo2_write_init,
.write = machxo2_write,
.write_complete = machxo2_write_complete,
+ .owner = THIS_MODULE,
};
static int machxo2_spi_probe(struct spi_device *spi)
@@ -362,6 +362,7 @@ static const struct fpga_manager_ops mpf_ops = {
.write_init = mpf_ops_write_init,
.write = mpf_ops_write,
.write_complete = mpf_ops_write_complete,
+ .owner = THIS_MODULE,
};
static int mpf_probe(struct spi_device *spi)
@@ -463,6 +463,7 @@ static const struct fpga_manager_ops socfpga_a10_fpga_mgr_ops = {
.write_init = socfpga_a10_fpga_write_init,
.write = socfpga_a10_fpga_write,
.write_complete = socfpga_a10_fpga_write_complete,
+ .owner = THIS_MODULE,
};
static int socfpga_a10_fpga_probe(struct platform_device *pdev)
@@ -538,6 +538,7 @@ static const struct fpga_manager_ops socfpga_fpga_ops = {
.write_init = socfpga_fpga_ops_configure_init,
.write = socfpga_fpga_ops_configure_write,
.write_complete = socfpga_fpga_ops_configure_complete,
+ .owner = THIS_MODULE,
};
static int socfpga_fpga_probe(struct platform_device *pdev)
@@ -393,6 +393,7 @@ static const struct fpga_manager_ops s10_ops = {
.write_init = s10_ops_write_init,
.write = s10_ops_write,
.write_complete = s10_ops_write_complete,
+ .owner = THIS_MODULE,
};
static int s10_probe(struct platform_device *pdev)
@@ -187,6 +187,7 @@ static const struct fpga_manager_ops fake_mgr_ops = {
.write = op_write,
.write_sg = op_write_sg,
.write_complete = op_write_complete,
+ .owner = THIS_MODULE,
};
static void fpga_mgr_test_get(struct kunit *test)
@@ -52,6 +52,7 @@ static int op_write(struct fpga_manager *mgr, const char *buf, size_t count)
*/
static const struct fpga_manager_ops fake_mgr_ops = {
.write = op_write,
+ .owner = THIS_MODULE,
};
static int op_enable_set(struct fpga_bridge *bridge, bool enable)
@@ -96,6 +96,7 @@ static const struct fpga_manager_ops ts73xx_fpga_ops = {
.write_init = ts73xx_fpga_write_init,
.write = ts73xx_fpga_write,
.write_complete = ts73xx_fpga_write_complete,
+ .owner = THIS_MODULE,
};
static int ts73xx_fpga_probe(struct platform_device *pdev)
@@ -40,6 +40,7 @@ static int versal_fpga_ops_write(struct fpga_manager *mgr,
static const struct fpga_manager_ops versal_fpga_ops = {
.write_init = versal_fpga_ops_write_init,
.write = versal_fpga_ops_write,
+ .owner = THIS_MODULE,
};
static int versal_fpga_probe(struct platform_device *pdev)
@@ -218,6 +218,7 @@ static const struct fpga_manager_ops xilinx_spi_ops = {
.write_init = xilinx_spi_write_init,
.write = xilinx_spi_write,
.write_complete = xilinx_spi_write_complete,
+ .owner = THIS_MODULE,
};
static int xilinx_spi_probe(struct spi_device *spi)
@@ -548,6 +548,7 @@ static const struct fpga_manager_ops zynq_fpga_ops = {
.write_init = zynq_fpga_ops_write_init,
.write_sg = zynq_fpga_ops_write,
.write_complete = zynq_fpga_ops_write_complete,
+ .owner = THIS_MODULE,
};
static int zynq_fpga_probe(struct platform_device *pdev)
@@ -101,6 +101,7 @@ static const struct fpga_manager_ops zynqmp_fpga_ops = {
.state = zynqmp_fpga_ops_state,
.write_init = zynqmp_fpga_ops_write_init,
.write = zynqmp_fpga_ops_write,
+ .owner = THIS_MODULE,
};
static int zynqmp_fpga_probe(struct platform_device *pdev)