[PATCH 2/2] fsi: Lock mutex for master device registration

Eddie James posted 2 patches 2 years, 1 month ago
[PATCH 2/2] fsi: Lock mutex for master device registration
Posted by Eddie James 2 years, 1 month ago
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@linux.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 {
+		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
Re: [PATCH 2/2] fsi: Lock mutex for master device registration
Posted by Joel Stanley 2 years, 1 month ago
On Wed, 9 Aug 2023 at 18:08, Eddie James <eajames@linux.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@linux.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
>