Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230809180814.151984-1-eajames@linux.ibm.com> <20230809180814.151984-3-eajames@linux.ibm.com>
In-Reply-To: <20230809180814.151984-3-eajames@linux.ibm.com>
From:   Joel Stanley <joel@jms.id.au>
Date:   Thu, 10 Aug 2023 05:36:39 +0000
Message-ID: <CACPK8XeFVjHxTZxx2GGUiWN9+yqNscCoOJSGw6vH-NkXOWw_kg@mail.gmail.com>
Subject: Re: [PATCH 2/2] fsi: Lock mutex for master device registration
To:     Eddie James <eajames@linux.ibm.com>
Cc:     linux-fsi@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        alistair@popple.id.au, k@ozlabs.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>