[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CACPK8XeFVjHxTZxx2GGUiWN9+yqNscCoOJSGw6vH-NkXOWw_kg@mail.gmail.com>
Date: Thu, 10 Aug 2023 05:36:39 +0000
From: Joel Stanley <joel@....id.au>
To: Eddie James <eajames@...ux.ibm.com>
Cc: linux-fsi@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
alistair@...ple.id.au, k@...abs.org
Subject: Re: [PATCH 2/2] fsi: Lock mutex for master device registration
On Wed, 9 Aug 2023 at 18:08, Eddie James <eajames@...ux.ibm.com> wrote:
>
> Because master device registration may cause hub master scans, or
> user scans may begin before device registration has ended, so the
> master scan lock must be held while registering the device.
>
> Signed-off-by: Eddie James <eajames@...ux.ibm.com>
> ---
> drivers/fsi/fsi-core.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index c7a002076292..ad50cdaf8421 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -1306,7 +1306,6 @@ static struct class fsi_master_class = {
> int fsi_master_register(struct fsi_master *master)
> {
> int rc;
> - struct device_node *np;
>
> mutex_init(&master->scan_lock);
>
> @@ -1326,20 +1325,19 @@ int fsi_master_register(struct fsi_master *master)
>
> master->dev.class = &fsi_master_class;
>
> + mutex_lock(&master->scan_lock);
> rc = device_register(&master->dev);
> if (rc) {
> ida_free(&master_ida, master->idx);
> - return rc;
> - }
> + } else {
I kept getting stuck on the "if else" behaviour, but really the else
is just the non-error path following the device_register.
I reworked the patch to be like this:
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -1326,20 +1326,21 @@ int fsi_master_register(struct fsi_master *master)
master->dev.class = &fsi_master_class;
+ mutex_lock(&master->scan_lock);
rc = device_register(&master->dev);
if (rc) {
ida_free(&master_ida, master->idx);
- return rc;
+ goto out;
}
np = dev_of_node(&master->dev);
if (!of_property_read_bool(np, "no-scan-on-init")) {
- mutex_lock(&master->scan_lock);
fsi_master_scan(master);
- mutex_unlock(&master->scan_lock);
}
- return 0;
+out:
+ mutex_unlock(&master->scan_lock);
+ return rc;
}
EXPORT_SYMBOL_GPL(fsi_master_register);
If you don't mind that style, can you send a new version with that?
> + struct device_node *np = dev_of_node(&master->dev);
>
> - np = dev_of_node(&master->dev);
> - if (!of_property_read_bool(np, "no-scan-on-init")) {
> - mutex_lock(&master->scan_lock);
> - fsi_master_scan(master);
> - mutex_unlock(&master->scan_lock);
> + if (!of_property_read_bool(np, "no-scan-on-init"))
> + fsi_master_scan(master);
> }
>
> - return 0;
> + mutex_unlock(&master->scan_lock);
> + return rc;
> }
> EXPORT_SYMBOL_GPL(fsi_master_register);
>
> --
> 2.39.3
>
Powered by blists - more mailing lists