2023-12-18 20:28:26

by Marco Pagani

Subject: [RFC PATCH v3 0/2] fpga: improve protection against low-level control module unloading

This RFC proposes a solution to keep protecting the fpga manager against
the unloading of the low-level control modules while addressing the
limitations of the current implementation. Currently, the code assumes
that the low-level module registers a platform driver for the parent
device that is later used by the fpga manager (through the parent
device's driver pointer) to take the low-level module's refcount. This
proposal removes this limitation by adding a module owner field to the
fpga_manager_ops struct. Low-level control modules can statically set
the owner field to THIS_MODULE, and the fpga manager can later use it to
take the low-level module's refcount.

For more context, please refer to these threads:
https://lore.kernel.org/linux-fpga/ZS6hhlvjUcqyv8zL@yilunxu-OptiPlex-7050
https://lore.kernel.org/linux-fpga/ZT9qENE9fE3Z0KCW@yilunxu-OptiPlex-7050

v3:
- Improved locking using Yilun's proposal
v2:
- Fixed protection against races during module removal

Marco Pagani (2):
fpga: add an owner field and use it to take the low-level module's
refcount
fpga: set owner of fpga_manager_ops for existing low-level modules

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/fpga-mgr.c | 50 ++++++++++++++++++---------
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 +
include/linux/fpga/fpga-mgr.h | 4 +++
20 files changed, 56 insertions(+), 16 deletions(-)


base-commit: ceb6a6f023fd3e8b07761ed900352ef574010bcb
--
2.43.0

2023-12-18 20:28:39

by Marco Pagani

Subject: [RFC PATCH v3 1/2] fpga: add an owner field and use it to take the low-level module's refcount

Add a module owner field to the fpga_manager_ops struct and use it to
take the low-level control module's refcount instead of relying on the
parent device's driver pointer. Low-level control modules should
statically set the owner field to THIS_MODULE. To detect when the owner
module pointer becomes stale, set the mops pointer to null during
fpga_mgr_unregister() (called by the low-level module exit function) and
test it before taking the module's refcount. Use a mutex to avoid a
crash that can happen if __fpga_mgr_get() gets suspended between testing
the mops pointer and taking the low-level refcount and then resumes when
the low-level module has already been freed.

Thanks to Xu Yilun for suggesting the locking pattern.

Other changes: move put_device() from __fpga_mgr_get() to fpga_mgr_get()
and of_fpga_mgr_get() to improve code clarity.

Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get")
Signed-off-by: Marco Pagani <[email protected]>
---
drivers/fpga/fpga-mgr.c | 50 ++++++++++++++++++++++++-----------
include/linux/fpga/fpga-mgr.h | 4 +++
2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
index 06651389c592..a32b7d40080d 100644
--- a/drivers/fpga/fpga-mgr.c
+++ b/drivers/fpga/fpga-mgr.c
@@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = {
};
ATTRIBUTE_GROUPS(fpga_mgr);

-static struct fpga_manager *__fpga_mgr_get(struct device *dev)
+static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev)
{
struct fpga_manager *mgr;

- mgr = to_fpga_manager(dev);
+ mgr = to_fpga_manager(mgr_dev);

- if (!try_module_get(dev->parent->driver->owner))
- goto err_dev;
+ mutex_lock(&mgr->mops_mutex);

- return mgr;
+ if (!mgr->mops || !try_module_get(mgr->mops->owner))
+ mgr = ERR_PTR(-ENODEV);

-err_dev:
- put_device(dev);
- return ERR_PTR(-ENODEV);
+ mutex_unlock(&mgr->mops_mutex);
+
+ return mgr;
}

static int fpga_mgr_dev_match(struct device *dev, const void *data)
@@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
*/
struct fpga_manager *fpga_mgr_get(struct device *dev)
{
- struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev,
- fpga_mgr_dev_match);
+ struct fpga_manager *mgr;
+ struct device *mgr_dev;
+
+ mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match);
if (!mgr_dev)
return ERR_PTR(-ENODEV);

- return __fpga_mgr_get(mgr_dev);
+ mgr = __fpga_mgr_get(mgr_dev);
+ if (IS_ERR(mgr))
+ put_device(mgr_dev);
+
+ return mgr;
}
EXPORT_SYMBOL_GPL(fpga_mgr_get);

@@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get);
*/
struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
{
- struct device *dev;
+ struct fpga_manager *mgr;
+ struct device *mgr_dev;

- dev = class_find_device_by_of_node(&fpga_mgr_class, node);
- if (!dev)
+ mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node);
+ if (!mgr_dev)
return ERR_PTR(-ENODEV);

- return __fpga_mgr_get(dev);
+ mgr = __fpga_mgr_get(mgr_dev);
+ if (IS_ERR(mgr))
+ put_device(mgr_dev);
+
+ return mgr;
}
EXPORT_SYMBOL_GPL(of_fpga_mgr_get);

@@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
*/
void fpga_mgr_put(struct fpga_manager *mgr)
{
- module_put(mgr->dev.parent->driver->owner);
+ module_put(mgr->mops->owner);
put_device(&mgr->dev);
}
EXPORT_SYMBOL_GPL(fpga_mgr_put);
@@ -803,6 +814,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
}

mutex_init(&mgr->ref_mutex);
+ mutex_init(&mgr->mops_mutex);

mgr->name = info->name;
mgr->mops = info->mops;
@@ -888,6 +900,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
*/
fpga_mgr_fpga_remove(mgr);

+ mutex_lock(&mgr->mops_mutex);
+
+ mgr->mops = NULL;
+
+ mutex_unlock(&mgr->mops_mutex);
+
device_unregister(&mgr->dev);
}
EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
index 54f63459efd6..b4d9413cb444 100644
--- a/include/linux/fpga/fpga-mgr.h
+++ b/include/linux/fpga/fpga-mgr.h
@@ -162,6 +162,7 @@ struct fpga_manager_info {
* @write_complete: set FPGA to operating state after writing is done
* @fpga_remove: optional: Set FPGA into a specific state during driver remove
* @groups: optional attribute groups.
+ * @owner: owner module containing the ops.
*
* fpga_manager_ops are the low level functions implemented by a specific
* fpga manager driver. The optional ones are tested for NULL before being
@@ -184,6 +185,7 @@ struct fpga_manager_ops {
struct fpga_image_info *info);
void (*fpga_remove)(struct fpga_manager *mgr);
const struct attribute_group **groups;
+ struct module *owner;
};

/* FPGA manager status: Partial/Full Reconfiguration errors */
@@ -201,6 +203,7 @@ struct fpga_manager_ops {
* @state: state of fpga manager
* @compat_id: FPGA manager id for compatibility check.
* @mops: pointer to struct of fpga manager ops
+ * @mops_mutex: protects mops from low-level module removal
* @priv: low level driver private date
*/
struct fpga_manager {
@@ -209,6 +212,7 @@ struct fpga_manager {
struct mutex ref_mutex;
enum fpga_mgr_states state;
struct fpga_compat_id *compat_id;
+ struct mutex mops_mutex;
const struct fpga_manager_ops *mops;
void *priv;
};
--
2.43.0

2023-12-18 20:29:04

by Marco Pagani

Subject: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
---
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,
};

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)
--
2.43.0

2023-12-18 20:33:54

by Greg KH

Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
> ---
> 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

2023-12-19 14:55:57

by Marco Pagani

Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
>> ---
>> 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

2023-12-19 15:10:12

by Greg KH

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 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 <[email protected]>
> >> ---
> >> 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

2023-12-19 17:17:45

by Marco Pagani

Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
>>>> ---
>>>> 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

2023-12-19 18:27:04

by Greg KH

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 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 <[email protected]>
> >>>> ---
> >>>> 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

2023-12-20 22:24:44

by Marco Pagani

Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
>>>>>> ---
>>>>>> 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

2023-12-21 08:22:11

by Greg KH

Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
> >>>>>> ---
> >>>>>> 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

2023-12-21 09:33:32

by Xu Yilun

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 <[email protected]>
> > >>>> ---
> > >>>> 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
>

2023-12-22 20:52:44

by Marco Pagani

Subject: Re: [RFC PATCH v3 2/2] fpga: set owner of fpga_manager_ops for existing low-level modules

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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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

2023-12-25 07:01:32

by Xu Yilun

Subject: Re: [RFC PATCH v3 1/2] fpga: add an owner field and use it to take the low-level module's refcount

On Mon, Dec 18, 2023 at 09:28:08PM +0100, Marco Pagani wrote:
> Add a module owner field to the fpga_manager_ops struct and use it to
> take the low-level control module's refcount instead of relying on the
> parent device's driver pointer. Low-level control modules should
> statically set the owner field to THIS_MODULE. To detect when the owner

Don't be so strict, people could pass in any owner module they think it
is correct.

> module pointer becomes stale, set the mops pointer to null during
> fpga_mgr_unregister() (called by the low-level module exit function) and

No need the side note, people could call fpga_mgr_unregister() at any
time they think it is correct.

> test it before taking the module's refcount. Use a mutex to avoid a
> crash that can happen if __fpga_mgr_get() gets suspended between testing
> the mops pointer and taking the low-level refcount and then resumes when
> the low-level module has already been freed.
>
> Thanks to Xu Yilun for suggesting the locking pattern.

I appreciate that but don't put it in changelog. A Suggested-by is
appropriate.

>
> Other changes: move put_device() from __fpga_mgr_get() to fpga_mgr_get()

Opportunistically move put_device() ...

The point is, try to imply the *Other* changes are simple and relative to
the main change, or we should split the patch.

Sorry but seeing the actual change, please split. The whole change is
small and the put_device() part contributes 40% code ...

Thanks,
Yilun

> and of_fpga_mgr_get() to improve code clarity.
>
> Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get")
> Signed-off-by: Marco Pagani <[email protected]>
> ---
> drivers/fpga/fpga-mgr.c | 50 ++++++++++++++++++++++++-----------
> include/linux/fpga/fpga-mgr.h | 4 +++
> 2 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 06651389c592..a32b7d40080d 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = {
> };
> ATTRIBUTE_GROUPS(fpga_mgr);
>
> -static struct fpga_manager *__fpga_mgr_get(struct device *dev)
> +static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev)
> {
> struct fpga_manager *mgr;
>
> - mgr = to_fpga_manager(dev);
> + mgr = to_fpga_manager(mgr_dev);
>
> - if (!try_module_get(dev->parent->driver->owner))
> - goto err_dev;
> + mutex_lock(&mgr->mops_mutex);
>
> - return mgr;
> + if (!mgr->mops || !try_module_get(mgr->mops->owner))
> + mgr = ERR_PTR(-ENODEV);
>
> -err_dev:
> - put_device(dev);
> - return ERR_PTR(-ENODEV);
> + mutex_unlock(&mgr->mops_mutex);
> +
> + return mgr;
> }
>
> static int fpga_mgr_dev_match(struct device *dev, const void *data)
> @@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
> */
> struct fpga_manager *fpga_mgr_get(struct device *dev)
> {
> - struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev,
> - fpga_mgr_dev_match);
> + struct fpga_manager *mgr;
> + struct device *mgr_dev;
> +
> + mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match);
> if (!mgr_dev)
> return ERR_PTR(-ENODEV);
>
> - return __fpga_mgr_get(mgr_dev);
> + mgr = __fpga_mgr_get(mgr_dev);
> + if (IS_ERR(mgr))
> + put_device(mgr_dev);
> +
> + return mgr;
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_get);
>
> @@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get);
> */
> struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> {
> - struct device *dev;
> + struct fpga_manager *mgr;
> + struct device *mgr_dev;
>
> - dev = class_find_device_by_of_node(&fpga_mgr_class, node);
> - if (!dev)
> + mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node);
> + if (!mgr_dev)
> return ERR_PTR(-ENODEV);
>
> - return __fpga_mgr_get(dev);
> + mgr = __fpga_mgr_get(mgr_dev);
> + if (IS_ERR(mgr))
> + put_device(mgr_dev);
> +
> + return mgr;
> }
> EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>
> @@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
> */
> void fpga_mgr_put(struct fpga_manager *mgr)
> {
> - module_put(mgr->dev.parent->driver->owner);
> + module_put(mgr->mops->owner);
> put_device(&mgr->dev);
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_put);
> @@ -803,6 +814,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
> }
>
> mutex_init(&mgr->ref_mutex);
> + mutex_init(&mgr->mops_mutex);
>
> mgr->name = info->name;
> mgr->mops = info->mops;
> @@ -888,6 +900,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
> */
> fpga_mgr_fpga_remove(mgr);
>
> + mutex_lock(&mgr->mops_mutex);
> +
> + mgr->mops = NULL;
> +
> + mutex_unlock(&mgr->mops_mutex);
> +
> device_unregister(&mgr->dev);
> }
> EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
> index 54f63459efd6..b4d9413cb444 100644
> --- a/include/linux/fpga/fpga-mgr.h
> +++ b/include/linux/fpga/fpga-mgr.h
> @@ -162,6 +162,7 @@ struct fpga_manager_info {
> * @write_complete: set FPGA to operating state after writing is done
> * @fpga_remove: optional: Set FPGA into a specific state during driver remove
> * @groups: optional attribute groups.
> + * @owner: owner module containing the ops.
> *
> * fpga_manager_ops are the low level functions implemented by a specific
> * fpga manager driver. The optional ones are tested for NULL before being
> @@ -184,6 +185,7 @@ struct fpga_manager_ops {
> struct fpga_image_info *info);
> void (*fpga_remove)(struct fpga_manager *mgr);
> const struct attribute_group **groups;
> + struct module *owner;
> };
>
> /* FPGA manager status: Partial/Full Reconfiguration errors */
> @@ -201,6 +203,7 @@ struct fpga_manager_ops {
> * @state: state of fpga manager
> * @compat_id: FPGA manager id for compatibility check.
> * @mops: pointer to struct of fpga manager ops
> + * @mops_mutex: protects mops from low-level module removal
> * @priv: low level driver private date
> */
> struct fpga_manager {
> @@ -209,6 +212,7 @@ struct fpga_manager {
> struct mutex ref_mutex;
> enum fpga_mgr_states state;
> struct fpga_compat_id *compat_id;
> + struct mutex mops_mutex;
> const struct fpga_manager_ops *mops;
> void *priv;
> };
> --
> 2.43.0
>
>

2024-01-03 15:02:53

by Marco Pagani

Subject: Re: [RFC PATCH v3 1/2] fpga: add an owner field and use it to take the low-level module's refcount

On 2023-12-25 07:58, Xu Yilun wrote:
> On Mon, Dec 18, 2023 at 09:28:08PM +0100, Marco Pagani wrote:
>> Add a module owner field to the fpga_manager_ops struct and use it to
>> take the low-level control module's refcount instead of relying on the
>> parent device's driver pointer. Low-level control modules should
>> statically set the owner field to THIS_MODULE. To detect when the owner
>
> Don't be so strict, people could pass in any owner module they think it
> is correct.
>

I'm planning to use a helper macro to set the owner field at registration,
as suggested by Greg K-H. So, I would change this sentence anyway.

>> module pointer becomes stale, set the mops pointer to null during
>> fpga_mgr_unregister() (called by the low-level module exit function) and
>
> No need the side note, people could call fpga_mgr_unregister() at any
> time they think it is correct.
>

Okay, I'll drop this line.

>> test it before taking the module's refcount. Use a mutex to avoid a
>> crash that can happen if __fpga_mgr_get() gets suspended between testing
>> the mops pointer and taking the low-level refcount and then resumes when
>> the low-level module has already been freed.
>>
>> Thanks to Xu Yilun for suggesting the locking pattern.
>
> I appreciate that but don't put it in changelog. A Suggested-by is
> appropriate.
>

Okay.

>>
>> Other changes: move put_device() from __fpga_mgr_get() to fpga_mgr_get()
>
> Opportunistically move put_device() ...
>
> The point is, try to imply the *Other* changes are simple and relative to
> the main change, or we should split the patch.
>
> Sorry but seeing the actual change, please split. The whole change is
> small and the put_device() part contributes 40% code ...
>

Make sense. I would prefer to rephrase the message and do everything in a
single patch since moving put_device() is a small change related to the
mechanism for getting the manager.

Thanks,
Marco

>> and of_fpga_mgr_get() to improve code clarity.
>>
>> Fixes: 654ba4cc0f3e ("fpga manager: ensure lifetime with of_fpga_mgr_get")
>> Signed-off-by: Marco Pagani <[email protected]>
>> ---
>> drivers/fpga/fpga-mgr.c | 50 ++++++++++++++++++++++++-----------
>> include/linux/fpga/fpga-mgr.h | 4 +++
>> 2 files changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> index 06651389c592..a32b7d40080d 100644
>> --- a/drivers/fpga/fpga-mgr.c
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -664,20 +664,20 @@ static struct attribute *fpga_mgr_attrs[] = {
>> };
>> ATTRIBUTE_GROUPS(fpga_mgr);
>>
>> -static struct fpga_manager *__fpga_mgr_get(struct device *dev)
>> +static struct fpga_manager *__fpga_mgr_get(struct device *mgr_dev)
>> {
>> struct fpga_manager *mgr;
>>
>> - mgr = to_fpga_manager(dev);
>> + mgr = to_fpga_manager(mgr_dev);
>>
>> - if (!try_module_get(dev->parent->driver->owner))
>> - goto err_dev;
>> + mutex_lock(&mgr->mops_mutex);
>>
>> - return mgr;
>> + if (!mgr->mops || !try_module_get(mgr->mops->owner))
>> + mgr = ERR_PTR(-ENODEV);
>>
>> -err_dev:
>> - put_device(dev);
>> - return ERR_PTR(-ENODEV);
>> + mutex_unlock(&mgr->mops_mutex);
>> +
>> + return mgr;
>> }
>>
>> static int fpga_mgr_dev_match(struct device *dev, const void *data)
>> @@ -693,12 +693,18 @@ static int fpga_mgr_dev_match(struct device *dev, const void *data)
>> */
>> struct fpga_manager *fpga_mgr_get(struct device *dev)
>> {
>> - struct device *mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev,
>> - fpga_mgr_dev_match);
>> + struct fpga_manager *mgr;
>> + struct device *mgr_dev;
>> +
>> + mgr_dev = class_find_device(&fpga_mgr_class, NULL, dev, fpga_mgr_dev_match);
>> if (!mgr_dev)
>> return ERR_PTR(-ENODEV);
>>
>> - return __fpga_mgr_get(mgr_dev);
>> + mgr = __fpga_mgr_get(mgr_dev);
>> + if (IS_ERR(mgr))
>> + put_device(mgr_dev);
>> +
>> + return mgr;
>> }
>> EXPORT_SYMBOL_GPL(fpga_mgr_get);
>>
>> @@ -711,13 +717,18 @@ EXPORT_SYMBOL_GPL(fpga_mgr_get);
>> */
>> struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
>> {
>> - struct device *dev;
>> + struct fpga_manager *mgr;
>> + struct device *mgr_dev;
>>
>> - dev = class_find_device_by_of_node(&fpga_mgr_class, node);
>> - if (!dev)
>> + mgr_dev = class_find_device_by_of_node(&fpga_mgr_class, node);
>> + if (!mgr_dev)
>> return ERR_PTR(-ENODEV);
>>
>> - return __fpga_mgr_get(dev);
>> + mgr = __fpga_mgr_get(mgr_dev);
>> + if (IS_ERR(mgr))
>> + put_device(mgr_dev);
>> +
>> + return mgr;
>> }
>> EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>>
>> @@ -727,7 +738,7 @@ EXPORT_SYMBOL_GPL(of_fpga_mgr_get);
>> */
>> void fpga_mgr_put(struct fpga_manager *mgr)
>> {
>> - module_put(mgr->dev.parent->driver->owner);
>> + module_put(mgr->mops->owner);
>> put_device(&mgr->dev);
>> }
>> EXPORT_SYMBOL_GPL(fpga_mgr_put);
>> @@ -803,6 +814,7 @@ fpga_mgr_register_full(struct device *parent, const struct fpga_manager_info *in
>> }
>>
>> mutex_init(&mgr->ref_mutex);
>> + mutex_init(&mgr->mops_mutex);
>>
>> mgr->name = info->name;
>> mgr->mops = info->mops;
>> @@ -888,6 +900,12 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>> */
>> fpga_mgr_fpga_remove(mgr);
>>
>> + mutex_lock(&mgr->mops_mutex);
>> +
>> + mgr->mops = NULL;
>> +
>> + mutex_unlock(&mgr->mops_mutex);
>> +
>> device_unregister(&mgr->dev);
>> }
>> EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
>> diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
>> index 54f63459efd6..b4d9413cb444 100644
>> --- a/include/linux/fpga/fpga-mgr.h
>> +++ b/include/linux/fpga/fpga-mgr.h
>> @@ -162,6 +162,7 @@ struct fpga_manager_info {
>> * @write_complete: set FPGA to operating state after writing is done
>> * @fpga_remove: optional: Set FPGA into a specific state during driver remove
>> * @groups: optional attribute groups.
>> + * @owner: owner module containing the ops.
>> *
>> * fpga_manager_ops are the low level functions implemented by a specific
>> * fpga manager driver. The optional ones are tested for NULL before being
>> @@ -184,6 +185,7 @@ struct fpga_manager_ops {
>> struct fpga_image_info *info);
>> void (*fpga_remove)(struct fpga_manager *mgr);
>> const struct attribute_group **groups;
>> + struct module *owner;
>> };
>>
>> /* FPGA manager status: Partial/Full Reconfiguration errors */
>> @@ -201,6 +203,7 @@ struct fpga_manager_ops {
>> * @state: state of fpga manager
>> * @compat_id: FPGA manager id for compatibility check.
>> * @mops: pointer to struct of fpga manager ops
>> + * @mops_mutex: protects mops from low-level module removal
>> * @priv: low level driver private date
>> */
>> struct fpga_manager {
>> @@ -209,6 +212,7 @@ struct fpga_manager {
>> struct mutex ref_mutex;
>> enum fpga_mgr_states state;
>> struct fpga_compat_id *compat_id;
>> + struct mutex mops_mutex;
>> const struct fpga_manager_ops *mops;
>> void *priv;
>> };
>> --
>> 2.43.0
>>
>>
>