Date: Thu, 21 Dec 2023 17:26:59 +0800
From: Xu Yilun <yilun.xu@...ux.intel.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Marco Pagani <marpagan@...hat.com>, Moritz Fischer <mdf@...nel.org>,
	Wu Hao <hao.wu@...el.com>, Xu Yilun <yilun.xu@...el.com>,
	Tom Rix <trix@...hat.com>, linux-kernel@...r.kernel.org,
	linux-fpga@...r.kernel.org
Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for
 existing low-level modules

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

Powered by blists - more mailing lists