| Message ID | 20231218202809.84253-3-marpagan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: improve protection against low-level control module unloading | expand |
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
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, }; static const struct cvp_priv cvp_priv_v1 = { diff --git a/drivers/fpga/altera-pr-ip-core.c b/drivers/fpga/altera-pr-ip-core.c index df8671af4a92..354221c609e6 100644 --- a/drivers/fpga/altera-pr-ip-core.c +++ b/drivers/fpga/altera-pr-ip-core.c @@ -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) diff --git a/drivers/fpga/altera-ps-spi.c b/drivers/fpga/altera-ps-spi.c index 740980e7cef8..3be05796a6fc 100644 --- a/drivers/fpga/altera-ps-spi.c +++ b/drivers/fpga/altera-ps-spi.c @@ -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) diff --git a/drivers/fpga/dfl-fme-mgr.c b/drivers/fpga/dfl-fme-mgr.c index ab228d8837a0..740ce82e3ac9 100644 --- a/drivers/fpga/dfl-fme-mgr.c +++ b/drivers/fpga/dfl-fme-mgr.c @@ -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, diff --git a/drivers/fpga/ice40-spi.c b/drivers/fpga/ice40-spi.c index 7cbb3558b844..97afa6dc5d76 100644 --- a/drivers/fpga/ice40-spi.c +++ b/drivers/fpga/ice40-spi.c @@ -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) diff --git a/drivers/fpga/lattice-sysconfig.c b/drivers/fpga/lattice-sysconfig.c index ba51a60f672f..1393cdd11e49 100644 --- a/drivers/fpga/lattice-sysconfig.c +++ b/drivers/fpga/lattice-sysconfig.c @@ -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) diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c index 905607992a12..46193a47f863 100644 --- a/drivers/fpga/machxo2-spi.c +++ b/drivers/fpga/machxo2-spi.c @@ -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) diff --git a/drivers/fpga/microchip-spi.c b/drivers/fpga/microchip-spi.c index 2a82c726d6e5..023ccdf2d5da 100644 --- a/drivers/fpga/microchip-spi.c +++ b/drivers/fpga/microchip-spi.c @@ -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) diff --git a/drivers/fpga/socfpga-a10.c b/drivers/fpga/socfpga-a10.c index cc4861e345c9..a8ab74b30006 100644 --- a/drivers/fpga/socfpga-a10.c +++ b/drivers/fpga/socfpga-a10.c @@ -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) diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c index 723ea0ad3f09..87f3f4a367d0 100644 --- a/drivers/fpga/socfpga.c +++ b/drivers/fpga/socfpga.c @@ -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) diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c index cacb9cc5757e..63a5a2fe4911 100644 --- a/drivers/fpga/stratix10-soc.c +++ b/drivers/fpga/stratix10-soc.c @@ -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) diff --git a/drivers/fpga/tests/fpga-mgr-test.c b/drivers/fpga/tests/fpga-mgr-test.c index 6acec55b60ce..4c2a3e98f8ad 100644 --- a/drivers/fpga/tests/fpga-mgr-test.c +++ b/drivers/fpga/tests/fpga-mgr-test.c @@ -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) diff --git a/drivers/fpga/tests/fpga-region-test.c b/drivers/fpga/tests/fpga-region-test.c index baab07e3fc59..2705c1b33d09 100644 --- a/drivers/fpga/tests/fpga-region-test.c +++ b/drivers/fpga/tests/fpga-region-test.c @@ -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) diff --git a/drivers/fpga/ts73xx-fpga.c b/drivers/fpga/ts73xx-fpga.c index 4e1d2a4d3df4..20b8db0d150a 100644 --- a/drivers/fpga/ts73xx-fpga.c +++ b/drivers/fpga/ts73xx-fpga.c @@ -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) diff --git a/drivers/fpga/versal-fpga.c b/drivers/fpga/versal-fpga.c index 3710e8f01be2..02fd8ed36ff0 100644 --- a/drivers/fpga/versal-fpga.c +++ b/drivers/fpga/versal-fpga.c @@ -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) diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c index e1a227e7ff2a..d58cf0ccbd41 100644 --- a/drivers/fpga/xilinx-spi.c +++ b/drivers/fpga/xilinx-spi.c @@ -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) diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c index 96611d424a10..241e1fe48a13 100644 --- a/drivers/fpga/zynq-fpga.c +++ b/drivers/fpga/zynq-fpga.c @@ -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) diff --git a/drivers/fpga/zynqmp-fpga.c b/drivers/fpga/zynqmp-fpga.c index f3434e2c487b..2f66400d2330 100644 --- a/drivers/fpga/zynqmp-fpga.c +++ b/drivers/fpga/zynqmp-fpga.c @@ -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)
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(+)