diff mbox series

[11/14] fsi: Improve master indexing

Message ID	20230612195657.245125-12-eajames@linux.ibm.com
State	New
Headers	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; From: Eddie James <eajames@linux.ibm.com> To: linux-fsi@lists.ozlabs.org Cc: linux-kernel@vger.kernel.org, jk@ozlabs.org, joel@jms.id.au, alistair@popple.id.au, andrew@aj.id.au, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, devicetree@vger.kernel.org Subject: [PATCH 11/14] fsi: Improve master indexing Date: Mon, 12 Jun 2023 14:56:54 -0500 Message-Id: <20230612195657.245125-12-eajames@linux.ibm.com> In-Reply-To: <20230612195657.245125-1-eajames@linux.ibm.com> References: <20230612195657.245125-1-eajames@linux.ibm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	fsi: Miscellaneous fixes and I2C Responder driver \| [00/14] fsi: Miscellaneous fixes and I2C Responder driver [01/14] fsi: Move fsi_slave structure definition to header [02/14] fsi: Add aliased device numbering [03/14] fsi: Use of_match_table for bus matching if specified [04/14] fsi: sbefifo: Don't check status during probe [05/14] fsi: sbefifo: Add configurable in-command timeout [06/14] fsi: sbefifo: Remove limits on user-specified read timeout [07/14] fsi: aspeed: Reset master errors after CFAM reset [08/14] fsi: core: Add trace events for scan and unregister [09/14] fsi: core: Fix legacy minor numbering [10/14] fsi: core: Switch to ida_alloc/free [11/14] fsi: Improve master indexing [12/14] dt-bindings: fsi: Document the IBM I2C Responder virtual FSI master [13/14] fsi: Add IBM I2C Responder virtual FSI master [14/14] fsi: Add I2C Responder SCOM driver

Commit Message

Eddie James June 12, 2023, 7:56 p.m. UTC

  Master indexing is problematic if a hub is rescanned while the
root master is being rescanned. Move the IDA free below the device
unregistration, lock the scan mutex in the probe function, and
request a specific idx in the hub driver.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-core.c       | 41 ++++++++++++++++++++++--------------
 drivers/fsi/fsi-master-hub.c |  2 ++
 2 files changed, 27 insertions(+), 16 deletions(-)

Comments

Joel Stanley Aug. 9, 2023, 7:08 a.m. UTC | #1

On Mon, 12 Jun 2023 at 19:57, Eddie James <eajames@linux.ibm.com> wrote:
>
> Master indexing is problematic if a hub is rescanned while the
> root master is being rescanned. Move the IDA free below the device
> unregistration, lock the scan mutex in the probe function, and
> request a specific idx in the hub driver.

I've applied this series, but taking a closer look at this patch I
think it can be improved. If you resend, just send this patch.

>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/fsi/fsi-core.c       | 41 ++++++++++++++++++++++--------------
>  drivers/fsi/fsi-master-hub.c |  2 ++
>  2 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index ec4d02264391..503061a6740b 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -1327,46 +1327,55 @@ 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);
> -       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
> +
> +       if (master->idx) {

Why do we allocate a new idx if there's already one?

> +               master->idx = ida_alloc_range(&master_ida, master->idx,
> +                                             master->idx, GFP_KERNEL);

If we can't get one in the range we want, we ask for any? Should this
print a warning?

> +               if (master->idx < 0)
> +                       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
> +       } else {

If ixd was zero, we create one. This is the "normal" case?

> +               master->idx = ida_alloc(&master_ida, GFP_KERNEL);
> +       }
> +

We check the same error condition again.

>         if (master->idx < 0)
>                 return master->idx;

>
> -       dev_set_name(&master->dev, "fsi%d", master->idx);
> +       if (!dev_name(&master->dev))
> +               dev_set_name(&master->dev, "fsi%d", master->idx);
> +
>         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);

This change looks a bit different to the idx changes. What's happening here?
>
> -       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);
>
>  void fsi_master_unregister(struct fsi_master *master)
>  {
> -       trace_fsi_master_unregister(master);
> +       int idx = master->idx;
>
> -       if (master->idx >= 0) {
> -               ida_free(&master_ida, master->idx);
> -               master->idx = -1;
> -       }
> +       trace_fsi_master_unregister(master);
>
>         mutex_lock(&master->scan_lock);
>         fsi_master_unscan(master);
> +       master->n_links = 0;
>         mutex_unlock(&master->scan_lock);
> +
>         device_unregister(&master->dev);
> +       ida_free(&master_ida, idx);
>  }
>  EXPORT_SYMBOL_GPL(fsi_master_unregister);
>
> diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
> index 6d8b6e8854e5..36da643b3201 100644
> --- a/drivers/fsi/fsi-master-hub.c
> +++ b/drivers/fsi/fsi-master-hub.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>
>  #include "fsi-master.h"
> +#include "fsi-slave.h"
>
>  #define FSI_ENGID_HUB_MASTER           0x1c
>
> @@ -229,6 +230,7 @@ static int hub_master_probe(struct device *dev)
>         hub->master.dev.release = hub_master_release;
>         hub->master.dev.of_node = of_node_get(dev_of_node(dev));
>
> +       hub->master.idx = fsi_dev->slave->link + 1;
>         hub->master.n_links = links;
>         hub->master.read = hub_master_read;
>         hub->master.write = hub_master_write;
> --
> 2.31.1
>

Joel Stanley Aug. 9, 2023, 11:55 a.m. UTC | #2

On Wed, 9 Aug 2023 at 07:08, Joel Stanley <joel@jms.id.au> wrote:
>
> On Mon, 12 Jun 2023 at 19:57, Eddie James <eajames@linux.ibm.com> wrote:
> >
> > Master indexing is problematic if a hub is rescanned while the
> > root master is being rescanned. Move the IDA free below the device
> > unregistration, lock the scan mutex in the probe function, and
> > request a specific idx in the hub driver.
>
> I've applied this series, but taking a closer look at this patch I
> think it can be improved. If you resend, just send this patch.

On hardware, it did this at FSI scan time:

 WARNING: CPU: 0 PID: 761 at /lib/idr.c:525 ida_free+0x140/0x154
 ida_free called for id=1 which is not allocated.
 CPU: 0 PID: 761 Comm: openpower-proc- Not tainted 6.1.34-d42f59e #1
 Hardware name: Generic DT based system
  unwind_backtrace from show_stack+0x18/0x1c
  show_stack from dump_stack_lvl+0x24/0x2c
  dump_stack_lvl from __warn+0x74/0xf0
  __warn from warn_slowpath_fmt+0x9c/0xd8
  warn_slowpath_fmt from ida_free+0x140/0x154
  ida_free from fsi_master_register+0xd0/0xf0
  fsi_master_register from hub_master_probe+0x11c/0x358
  hub_master_probe from really_probe+0xd4/0x3f0
  really_probe from driver_probe_device+0x38/0xd0
  driver_probe_device from __device_attach_driver+0xc8/0x148
  __device_attach_driver from bus_for_each_drv+0x90/0xdc
  bus_for_each_drv from __device_attach+0x114/0x1a4
  __device_attach from bus_probe_device+0x8c/0x94
  bus_probe_device from device_add+0x3a8/0x7fc
  device_add from fsi_master_scan+0x4e0/0x950
  fsi_master_scan from fsi_master_rescan+0x38/0x88
  fsi_master_rescan from master_rescan_store+0x14/0x20
  master_rescan_store from kernfs_fop_write_iter+0x114/0x200
  kernfs_fop_write_iter from vfs_write+0x1d0/0x374
  vfs_write from ksys_write+0x78/0x100
  ksys_write from ret_fast_syscall+0x0/0x54
 Exception stack(0x9fc51fa8 to 0x9fc51ff0)
 1fa0:                   00000001 01a01c78 00000003 01a01c78 00000001 00000001
 1fc0: 00000001 01a01c78 00000001 00000004 7eeb4ab0 7eeb4b3c 7eeb4ab4 7eeb499c
 1fe0: 76985abc 7eeb4928 76848af8 766f176c


>
> >
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > ---
> >  drivers/fsi/fsi-core.c       | 41 ++++++++++++++++++++++--------------
> >  drivers/fsi/fsi-master-hub.c |  2 ++
> >  2 files changed, 27 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> > index ec4d02264391..503061a6740b 100644
> > --- a/drivers/fsi/fsi-core.c
> > +++ b/drivers/fsi/fsi-core.c
> > @@ -1327,46 +1327,55 @@ 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);
> > -       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
> > +
> > +       if (master->idx) {
>
> Why do we allocate a new idx if there's already one?
>
> > +               master->idx = ida_alloc_range(&master_ida, master->idx,
> > +                                             master->idx, GFP_KERNEL);
>
> If we can't get one in the range we want, we ask for any? Should this
> print a warning?
>
> > +               if (master->idx < 0)
> > +                       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
> > +       } else {
>
> If ixd was zero, we create one. This is the "normal" case?
>
> > +               master->idx = ida_alloc(&master_ida, GFP_KERNEL);
> > +       }
> > +
>
> We check the same error condition again.
>
> >         if (master->idx < 0)
> >                 return master->idx;
>
> >
> > -       dev_set_name(&master->dev, "fsi%d", master->idx);
> > +       if (!dev_name(&master->dev))
> > +               dev_set_name(&master->dev, "fsi%d", master->idx);
> > +
> >         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);
>
> This change looks a bit different to the idx changes. What's happening here?
> >
> > -       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);
> >
> >  void fsi_master_unregister(struct fsi_master *master)
> >  {
> > -       trace_fsi_master_unregister(master);
> > +       int idx = master->idx;
> >
> > -       if (master->idx >= 0) {
> > -               ida_free(&master_ida, master->idx);
> > -               master->idx = -1;
> > -       }
> > +       trace_fsi_master_unregister(master);
> >
> >         mutex_lock(&master->scan_lock);
> >         fsi_master_unscan(master);
> > +       master->n_links = 0;
> >         mutex_unlock(&master->scan_lock);
> > +
> >         device_unregister(&master->dev);
> > +       ida_free(&master_ida, idx);
> >  }
> >  EXPORT_SYMBOL_GPL(fsi_master_unregister);
> >
> > diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
> > index 6d8b6e8854e5..36da643b3201 100644
> > --- a/drivers/fsi/fsi-master-hub.c
> > +++ b/drivers/fsi/fsi-master-hub.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/slab.h>
> >
> >  #include "fsi-master.h"
> > +#include "fsi-slave.h"
> >
> >  #define FSI_ENGID_HUB_MASTER           0x1c
> >
> > @@ -229,6 +230,7 @@ static int hub_master_probe(struct device *dev)
> >         hub->master.dev.release = hub_master_release;
> >         hub->master.dev.of_node = of_node_get(dev_of_node(dev));
> >
> > +       hub->master.idx = fsi_dev->slave->link + 1;
> >         hub->master.n_links = links;
> >         hub->master.read = hub_master_read;
> >         hub->master.write = hub_master_write;
> > --
> > 2.31.1
> >

Eddie James Aug. 9, 2023, 4:08 p.m. UTC | #3

On 8/9/23 06:55, Joel Stanley wrote:
> On Wed, 9 Aug 2023 at 07:08, Joel Stanley <joel@jms.id.au> wrote:
>> On Mon, 12 Jun 2023 at 19:57, Eddie James <eajames@linux.ibm.com> wrote:
>>> Master indexing is problematic if a hub is rescanned while the
>>> root master is being rescanned. Move the IDA free below the device
>>> unregistration, lock the scan mutex in the probe function, and
>>> request a specific idx in the hub driver.
>> I've applied this series, but taking a closer look at this patch I
>> think it can be improved. If you resend, just send this patch.
> On hardware, it did this at FSI scan time:


Well this backtrace is without this patch, right? The hub master changes 
that went in are dependent on this patch. master->idx is 1 for the hub 
but it's not being allocated in the ida and the device name isn't 
getting set. So device registration fails and then trying to free the 
index in the ida causes this warning.

I'll reply to your comments in your other email and rebase and resend.


>
>   WARNING: CPU: 0 PID: 761 at /lib/idr.c:525 ida_free+0x140/0x154
>   ida_free called for id=1 which is not allocated.
>   CPU: 0 PID: 761 Comm: openpower-proc- Not tainted 6.1.34-d42f59e #1
>   Hardware name: Generic DT based system
>    unwind_backtrace from show_stack+0x18/0x1c
>    show_stack from dump_stack_lvl+0x24/0x2c
>    dump_stack_lvl from __warn+0x74/0xf0
>    __warn from warn_slowpath_fmt+0x9c/0xd8
>    warn_slowpath_fmt from ida_free+0x140/0x154
>    ida_free from fsi_master_register+0xd0/0xf0
>    fsi_master_register from hub_master_probe+0x11c/0x358
>    hub_master_probe from really_probe+0xd4/0x3f0
>    really_probe from driver_probe_device+0x38/0xd0
>    driver_probe_device from __device_attach_driver+0xc8/0x148
>    __device_attach_driver from bus_for_each_drv+0x90/0xdc
>    bus_for_each_drv from __device_attach+0x114/0x1a4
>    __device_attach from bus_probe_device+0x8c/0x94
>    bus_probe_device from device_add+0x3a8/0x7fc
>    device_add from fsi_master_scan+0x4e0/0x950
>    fsi_master_scan from fsi_master_rescan+0x38/0x88
>    fsi_master_rescan from master_rescan_store+0x14/0x20
>    master_rescan_store from kernfs_fop_write_iter+0x114/0x200
>    kernfs_fop_write_iter from vfs_write+0x1d0/0x374
>    vfs_write from ksys_write+0x78/0x100
>    ksys_write from ret_fast_syscall+0x0/0x54
>   Exception stack(0x9fc51fa8 to 0x9fc51ff0)
>   1fa0:                   00000001 01a01c78 00000003 01a01c78 00000001 00000001
>   1fc0: 00000001 01a01c78 00000001 00000004 7eeb4ab0 7eeb4b3c 7eeb4ab4 7eeb499c
>   1fe0: 76985abc 7eeb4928 76848af8 766f176c
>
>
>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>> ---
>>>   drivers/fsi/fsi-core.c       | 41 ++++++++++++++++++++++--------------
>>>   drivers/fsi/fsi-master-hub.c |  2 ++
>>>   2 files changed, 27 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>>> index ec4d02264391..503061a6740b 100644
>>> --- a/drivers/fsi/fsi-core.c
>>> +++ b/drivers/fsi/fsi-core.c
>>> @@ -1327,46 +1327,55 @@ 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);
>>> -       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
>>> +
>>> +       if (master->idx) {
>> Why do we allocate a new idx if there's already one?
>>
>>> +               master->idx = ida_alloc_range(&master_ida, master->idx,
>>> +                                             master->idx, GFP_KERNEL);
>> If we can't get one in the range we want, we ask for any? Should this
>> print a warning?
>>
>>> +               if (master->idx < 0)
>>> +                       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
>>> +       } else {
>> If ixd was zero, we create one. This is the "normal" case?
>>
>>> +               master->idx = ida_alloc(&master_ida, GFP_KERNEL);
>>> +       }
>>> +
>> We check the same error condition again.
>>
>>>          if (master->idx < 0)
>>>                  return master->idx;
>>> -       dev_set_name(&master->dev, "fsi%d", master->idx);
>>> +       if (!dev_name(&master->dev))
>>> +               dev_set_name(&master->dev, "fsi%d", master->idx);
>>> +
>>>          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);
>> This change looks a bit different to the idx changes. What's happening here?
>>> -       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);
>>>
>>>   void fsi_master_unregister(struct fsi_master *master)
>>>   {
>>> -       trace_fsi_master_unregister(master);
>>> +       int idx = master->idx;
>>>
>>> -       if (master->idx >= 0) {
>>> -               ida_free(&master_ida, master->idx);
>>> -               master->idx = -1;
>>> -       }
>>> +       trace_fsi_master_unregister(master);
>>>
>>>          mutex_lock(&master->scan_lock);
>>>          fsi_master_unscan(master);
>>> +       master->n_links = 0;
>>>          mutex_unlock(&master->scan_lock);
>>> +
>>>          device_unregister(&master->dev);
>>> +       ida_free(&master_ida, idx);
>>>   }
>>>   EXPORT_SYMBOL_GPL(fsi_master_unregister);
>>>
>>> diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
>>> index 6d8b6e8854e5..36da643b3201 100644
>>> --- a/drivers/fsi/fsi-master-hub.c
>>> +++ b/drivers/fsi/fsi-master-hub.c
>>> @@ -12,6 +12,7 @@
>>>   #include <linux/slab.h>
>>>
>>>   #include "fsi-master.h"
>>> +#include "fsi-slave.h"
>>>
>>>   #define FSI_ENGID_HUB_MASTER           0x1c
>>>
>>> @@ -229,6 +230,7 @@ static int hub_master_probe(struct device *dev)
>>>          hub->master.dev.release = hub_master_release;
>>>          hub->master.dev.of_node = of_node_get(dev_of_node(dev));
>>>
>>> +       hub->master.idx = fsi_dev->slave->link + 1;
>>>          hub->master.n_links = links;
>>>          hub->master.read = hub_master_read;
>>>          hub->master.write = hub_master_write;
>>> --
>>> 2.31.1
>>>

Eddie James Aug. 9, 2023, 4:21 p.m. UTC | #4

On 8/9/23 02:08, Joel Stanley wrote:
> On Mon, 12 Jun 2023 at 19:57, Eddie James <eajames@linux.ibm.com> wrote:
>> Master indexing is problematic if a hub is rescanned while the
>> root master is being rescanned. Move the IDA free below the device
>> unregistration, lock the scan mutex in the probe function, and
>> request a specific idx in the hub driver.
> I've applied this series, but taking a closer look at this patch I
> think it can be improved. If you resend, just send this patch.
>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/fsi/fsi-core.c       | 41 ++++++++++++++++++++++--------------
>>   drivers/fsi/fsi-master-hub.c |  2 ++
>>   2 files changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index ec4d02264391..503061a6740b 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -1327,46 +1327,55 @@ 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);
>> -       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
>> +
>> +       if (master->idx) {
> Why do we allocate a new idx if there's already one?


At this point, the master driver (aspeed, hub, i2cr, whatever) might 
just be requesting a certain index. It's not allocated yet, so it needs 
to be allocated so that we don't get overlap.


>
>> +               master->idx = ida_alloc_range(&master_ida, master->idx,
>> +                                             master->idx, GFP_KERNEL);
> If we can't get one in the range we want, we ask for any? Should this
> print a warning?


Perhaps, we could also return error here if we decide that the requested 
index is needed and not just wanted.


>
>> +               if (master->idx < 0)
>> +                       master->idx = ida_alloc(&master_ida, GFP_KERNEL);
>> +       } else {
> If ixd was zero, we create one. This is the "normal" case?


Yes, the assumption is: zero is the default due to zero-alloc'd master 
structures.


>
>> +               master->idx = ida_alloc(&master_ida, GFP_KERNEL);
>> +       }
>> +
> We check the same error condition again.


Yes I might be able to make this cleaner...


>
>>          if (master->idx < 0)
>>                  return master->idx;
>> -       dev_set_name(&master->dev, "fsi%d", master->idx);
>> +       if (!dev_name(&master->dev))
>> +               dev_set_name(&master->dev, "fsi%d", master->idx);
>> +
>>          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);
> This change looks a bit different to the idx changes. What's happening here?


This is just restructuring to get the lock before the scan. It could be 
a separate commit, yea.

Thanks for the review!

Eddie


>> -       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);
>>
>>   void fsi_master_unregister(struct fsi_master *master)
>>   {
>> -       trace_fsi_master_unregister(master);
>> +       int idx = master->idx;
>>
>> -       if (master->idx >= 0) {
>> -               ida_free(&master_ida, master->idx);
>> -               master->idx = -1;
>> -       }
>> +       trace_fsi_master_unregister(master);
>>
>>          mutex_lock(&master->scan_lock);
>>          fsi_master_unscan(master);
>> +       master->n_links = 0;
>>          mutex_unlock(&master->scan_lock);
>> +
>>          device_unregister(&master->dev);
>> +       ida_free(&master_ida, idx);
>>   }
>>   EXPORT_SYMBOL_GPL(fsi_master_unregister);
>>
>> diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
>> index 6d8b6e8854e5..36da643b3201 100644
>> --- a/drivers/fsi/fsi-master-hub.c
>> +++ b/drivers/fsi/fsi-master-hub.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/slab.h>
>>
>>   #include "fsi-master.h"
>> +#include "fsi-slave.h"
>>
>>   #define FSI_ENGID_HUB_MASTER           0x1c
>>
>> @@ -229,6 +230,7 @@ static int hub_master_probe(struct device *dev)
>>          hub->master.dev.release = hub_master_release;
>>          hub->master.dev.of_node = of_node_get(dev_of_node(dev));
>>
>> +       hub->master.idx = fsi_dev->slave->link + 1;
>>          hub->master.n_links = links;
>>          hub->master.read = hub_master_read;
>>          hub->master.write = hub_master_write;
>> --
>> 2.31.1
>>

diff mbox series

Patch

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index ec4d02264391..503061a6740b 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -1327,46 +1327,55 @@  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);
-	master->idx = ida_alloc(&master_ida, GFP_KERNEL);
+
+	if (master->idx) {
+		master->idx = ida_alloc_range(&master_ida, master->idx,
+					      master->idx, GFP_KERNEL);
+		if (master->idx < 0)
+			master->idx = ida_alloc(&master_ida, GFP_KERNEL);
+	} else {
+		master->idx = ida_alloc(&master_ida, GFP_KERNEL);
+	}
+
 	if (master->idx < 0)
 		return master->idx;
 
-	dev_set_name(&master->dev, "fsi%d", master->idx);
+	if (!dev_name(&master->dev))
+		dev_set_name(&master->dev, "fsi%d", master->idx);
+
 	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);
 
 void fsi_master_unregister(struct fsi_master *master)
 {
-	trace_fsi_master_unregister(master);
+	int idx = master->idx;
 
-	if (master->idx >= 0) {
-		ida_free(&master_ida, master->idx);
-		master->idx = -1;
-	}
+	trace_fsi_master_unregister(master);
 
 	mutex_lock(&master->scan_lock);
 	fsi_master_unscan(master);
+	master->n_links = 0;
 	mutex_unlock(&master->scan_lock);
+
 	device_unregister(&master->dev);
+	ida_free(&master_ida, idx);
 }
 EXPORT_SYMBOL_GPL(fsi_master_unregister);
 
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
index 6d8b6e8854e5..36da643b3201 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -12,6 +12,7 @@ 
 #include <linux/slab.h>
 
 #include "fsi-master.h"
+#include "fsi-slave.h"
 
 #define FSI_ENGID_HUB_MASTER		0x1c
 
@@ -229,6 +230,7 @@  static int hub_master_probe(struct device *dev)
 	hub->master.dev.release = hub_master_release;
 	hub->master.dev.of_node = of_node_get(dev_of_node(dev));
 
+	hub->master.idx = fsi_dev->slave->link + 1;
 	hub->master.n_links = links;
 	hub->master.read = hub_master_read;
 	hub->master.write = hub_master_write;