From: Marco Pagani <marpagan@redhat.com>
To: Russ Weight <russ.weight@linux.dev>
Cc: Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>,
Xu Yilun <yilun.xu@intel.com>, Tom Rix <trix@redhat.com>,
Jonathan Corbet <corbet@lwn.net>,
Alan Tull <atull@opensource.altera.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-fpga@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] fpga: region: add owner module and take its refcount
Date: Tue, 26 Mar 2024 19:58:53 +0100 [thread overview]
Message-ID: <befc8ccf-0661-4c6f-b262-3dab3c34e0be@redhat.com> (raw)
In-Reply-To: <20240326172854.h6gww3mqlq63kxbp@4VRSMR2-DT.corp.robot.car>
On 2024-03-26 18:28, Russ Weight wrote:
>
> On Fri, Mar 22, 2024 at 06:19:30PM +0100, Marco Pagani wrote:
>> The current implementation of the fpga region assumes that the low-level
>> module registers a driver for the parent device and uses its owner pointer
>> to take the module's refcount. This approach is problematic since it can
>> lead to a null pointer dereference while attempting to get the region
>> during programming if the parent device does not have a driver.
>>
>> To address this problem, add a module owner pointer to the fpga_region
>> struct and use it to take the module's refcount. Modify the functions for
>> registering a region to take an additional owner module parameter and
>> rename them to avoid conflicts. Use the old function names for helper
>> macros that automatically set the module that registers the region as the
>> owner. This ensures compatibility with existing low-level control modules
>> and reduces the chances of registering a region without setting the owner.
>>
>> Also, update the documentation to keep it consistent with the new interface
>> for registering an fpga region.
>>
>> Other changes: unlock the mutex before calling put_device() in
>> fpga_region_put() to avoid potential use after release issues.
>>
>> Fixes: 0fa20cdfcc1f ("fpga: fpga-region: device tree control for FPGA")
>> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Suggested-by: Xu Yilun <yilun.xu@intel.com>
>> Signed-off-by: Marco Pagani <marpagan@redhat.com>
>> ---
>> Documentation/driver-api/fpga/fpga-region.rst | 13 ++++++----
>> drivers/fpga/fpga-region.c | 26 +++++++++++--------
>> include/linux/fpga/fpga-region.h | 13 +++++++---
>> 3 files changed, 33 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/driver-api/fpga/fpga-region.rst b/Documentation/driver-api/fpga/fpga-region.rst
>> index dc55d60a0b4a..3aff5199b6d8 100644
>> --- a/Documentation/driver-api/fpga/fpga-region.rst
>> +++ b/Documentation/driver-api/fpga/fpga-region.rst
>> @@ -46,13 +46,16 @@ API to add a new FPGA region
>> ----------------------------
>>
>> * struct fpga_region - The FPGA region struct
>> -* struct fpga_region_info - Parameter structure for fpga_region_register_full()
>> -* fpga_region_register_full() - Create and register an FPGA region using the
>> +* struct fpga_region_info - Parameter structure for __fpga_region_register_full()
>> +* __fpga_region_register_full() - Create and register an FPGA region using the
>> fpga_region_info structure to provide the full flexibility of options
>> -* fpga_region_register() - Create and register an FPGA region using standard
>> +* __fpga_region_register() - Create and register an FPGA region using standard
>> arguments
>> * fpga_region_unregister() - Unregister an FPGA region
>>
>> +Helper macros ``fpga_region_register()`` and ``fpga_region_register_full()``
>> +automatically sets the module that registers the FPGA region as the owner.
>
> s/sets/set/
Nice catch.
>
>> +
>> The FPGA region's probe function will need to get a reference to the FPGA
>> Manager it will be using to do the programming. This usually would happen
>> during the region's probe function.
>> @@ -82,10 +85,10 @@ following APIs to handle building or tearing down that list.
>> :functions: fpga_region_info
>>
>> .. kernel-doc:: drivers/fpga/fpga-region.c
>> - :functions: fpga_region_register_full
>> + :functions: __fpga_region_register
>>
>> .. kernel-doc:: drivers/fpga/fpga-region.c
>> - :functions: fpga_region_register
>> + :functions: __fpga_region_register_full
>>
>> .. kernel-doc:: drivers/fpga/fpga-region.c
>> :functions: fpga_region_unregister
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index b364a929425c..f8bbda024d59 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -53,7 +53,7 @@ static struct fpga_region *fpga_region_get(struct fpga_region *region)
>> }
>>
>> get_device(dev);
>> - if (!try_module_get(dev->parent->driver->owner)) {
>> + if (!try_module_get(region->get_br_owner)) {
>> put_device(dev);
>> mutex_unlock(®ion->mutex);
>> return ERR_PTR(-ENODEV);
>> @@ -75,9 +75,9 @@ static void fpga_region_put(struct fpga_region *region)
>>
>> dev_dbg(dev, "put\n");
>>
>> - module_put(dev->parent->driver->owner);
>> - put_device(dev);
>> + module_put(region->get_br_owner);
>> mutex_unlock(®ion->mutex);
>> + put_device(dev);
>> }
>>
>> /**
>> @@ -181,14 +181,16 @@ static struct attribute *fpga_region_attrs[] = {
>> ATTRIBUTE_GROUPS(fpga_region);
>>
>> /**
>> - * fpga_region_register_full - create and register an FPGA Region device
>> + * __fpga_region_register_full - create and register an FPGA Region device
>> * @parent: device parent
>> * @info: parameters for FPGA Region
>> + * @owner: owner module containing the get_bridges function
>> *
>> * Return: struct fpga_region or ERR_PTR()
>> */
>> struct fpga_region *
>> -fpga_region_register_full(struct device *parent, const struct fpga_region_info *info)
>> +__fpga_region_register_full(struct device *parent, const struct fpga_region_info *info,
>> + struct module *owner)
>> {
>> struct fpga_region *region;
>> int id, ret = 0;
>> @@ -213,6 +215,7 @@ fpga_region_register_full(struct device *parent, const struct fpga_region_info *
>> region->compat_id = info->compat_id;
>> region->priv = info->priv;
>> region->get_bridges = info->get_bridges;
>> + region->get_br_owner = owner;
>
> get_* implies a function. Maybe this would be better called br_owner?
You are right. I will change it to br_owner in v2.
>
> - Russ
>
[ ... ]
Thanks,
Marco
prev parent reply other threads:[~2024-03-26 18:59 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-22 17:19 [PATCH] fpga: region: add owner module and take its refcount Marco Pagani
2024-03-26 17:28 ` Russ Weight
2024-03-26 18:58 ` Marco Pagani [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=befc8ccf-0661-4c6f-b262-3dab3c34e0be@redhat.com \
--to=marpagan@redhat.com \
--cc=atull@opensource.altera.com \
--cc=corbet@lwn.net \
--cc=gregkh@linuxfoundation.org \
--cc=hao.wu@intel.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mdf@kernel.org \
--cc=russ.weight@linux.dev \
--cc=trix@redhat.com \
--cc=yilun.xu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).