On Mon, Apr 26, 2021 at 11:50:15AM +0200, Fabio M. De Francesco wrote:
> #define VISORHBA_ERROR_COUNT 30
>
> +static DEFINE_XARRAY_ALLOC(xa_dtstr);
> +
> static struct dentry *visorhba_debugfs_dir;
>
> /* GUIDS for HBA channel type supported by this driver */
> @@ -78,12 +80,6 @@ struct visorhba_devdata {
> unsigned int max_buff_len;
> int devnum;
> struct uiscmdrsp *cmdrsp;
> - /*
> - * allows us to pass int handles back-and-forth between us and
> - * iovm, instead of raw pointers
> - */
> - struct idr idr;
> -
Why did you change the driver from having one namespace per HBA to having
a global namespace?
> /*
> - * simple_idr_get - Associate a provided pointer with an int value
> - * 1 <= value <= INT_MAX, and return this int value;
> - * the pointer value can be obtained later by passing
> - * this int value to idr_find()
> - * @idrtable: The data object maintaining the pointer<-->int mappings
> - * @p: The pointer value to be remembered
> - * @lock: A spinlock used when exclusive access to idrtable is needed
> - *
> - * Return: The id number mapped to pointer 'p', 0 on failure
> + * simple_xa_dtstr_get - Store a pointer to xa_dtstr xarray
> + * @id: Pointer to ID
> + * @entry: New entry
> */
> -static unsigned int simple_idr_get(struct idr *idrtable, void *p,
> - spinlock_t *lock)
> +static int simple_xa_dtstr_get(u32 *id, void *entry)
> {
> - int id;
> - unsigned long flags;
> + int ret = xa_alloc_irq(&xa_dtstr, id, entry, xa_limit_32b, GFP_NOWAIT);
> + /* TODO: check for and manage errors */
>
> - idr_preload(GFP_KERNEL);
> - spin_lock_irqsave(lock, flags);
> - id = idr_alloc(idrtable, p, 1, INT_MAX, GFP_NOWAIT);
> - spin_unlock_irqrestore(lock, flags);
> - idr_preload_end();
> - /* failure */
> - if (id < 0)
> - return 0;
> - /* idr_alloc() guarantees > 0 */
> - return (unsigned int)(id);
> + return ret;
> }
I would think that this wrapper should probably be removed. It'll almost
certainly be better to inline the call to xa_alloc_irq() at the call
sites.
You've also changed the behaviour; it used to allocate an id between 1
and INT_MAX; now it allocates an ID between 0 and UINT_MAX. Maybe that's
safe, but you need to argue for it in the changelog.
And it shouldn't be using GFP_NOWAIT, but GFP_KERNEL, like the IDR code
used to do.
> /*
> @@ -216,22 +196,25 @@ static unsigned int simple_idr_get(struct idr *idrtable, void *p,
> * completion processing logic for a taskmgmt
> * cmd will be able to find who to wake up
> * and where to stash the result
> - * @idrtable: The data object maintaining the pointer<-->int mappings
> - * @lock: A spinlock used when exclusive access to idrtable is needed
> + * @xa_dtstr: The data object maintaining the pointer<-->int mappings
You added this in the documentation, but not in the function ...
> * @cmdrsp: Response from the IOVM
> * @event: The event handle to associate with an id
> * @result: The location to place the result of the event handle into
> */
> -static void setup_scsitaskmgmt_handles(struct idr *idrtable, spinlock_t *lock,
> - struct uiscmdrsp *cmdrsp,
> - wait_queue_head_t *event, int *result)
> +static void setup_scsitaskmgmt_handles(struct uiscmdrsp *cmdrsp,
> + wait_queue_head_t *event, u32 *result)
> {
> - /* specify the event that has to be triggered when this */
> - /* cmd is complete */
> - cmdrsp->scsitaskmgmt.notify_handle =
> - simple_idr_get(idrtable, event, lock);
> - cmdrsp->scsitaskmgmt.notifyresult_handle =
> - simple_idr_get(idrtable, result, lock);
> + void *entry;
> + int ret;
> +
> + /* specify the event that has to be triggered when this cmd is complete */
> + entry = &cmdrsp->scsitaskmgmt.notify_handle;
> + ret = simple_xa_dtstr_get(result, entry);
> + /* TODO: Check for and manage errors */
The prior code assigned the ID for 'event' to scsitaskmgmt.notify_handle.
Now, you're allocating an ID for the address of scsitaskmgmt.notify_handle
to 'result'. That's clearly not right.
> + entry = &cmdrsp->scsitaskmgmt.notifyresult_handle;
> + ret = simple_xa_dtstr_get(result, entry);
> + /* TODO: Check for and manage errors */
> }
>
> /*
> @@ -240,13 +223,17 @@ static void setup_scsitaskmgmt_handles(struct idr *idrtable, spinlock_t *lock,
> * @idrtable: The data object maintaining the pointer<-->int mappings
> * @cmdrsp: Response from the IOVM
> */
> -static void cleanup_scsitaskmgmt_handles(struct idr *idrtable,
> - struct uiscmdrsp *cmdrsp)
> +static void cleanup_scsitaskmgmt_handles(struct uiscmdrsp_scsitaskmgmt *scsitaskmgmt)
> {
> - if (cmdrsp->scsitaskmgmt.notify_handle)
> - idr_remove(idrtable, cmdrsp->scsitaskmgmt.notify_handle);
> - if (cmdrsp->scsitaskmgmt.notifyresult_handle)
> - idr_remove(idrtable, cmdrsp->scsitaskmgmt.notifyresult_handle);
> + struct uiscmdrsp *cmdrsp;
> + unsigned long index;
> +
> + xa_for_each(&xa_dtstr, index, cmdrsp) {
> + if (&cmdrsp->scsitaskmgmt != scsitaskmgmt)
> + continue;
> + xa_erase(&xa_dtstr, index);
> + kfree(cmdrsp);
> + }
I suspect this is part of the same confusion, but the old code passed in an
ID that we just looked up & removed. You've changed that to iterate over
all the entries and remove the ones that match ...
> @@ -1096,7 +1077,7 @@ static void visorhba_remove(struct visor_device *dev)
> scsi_remove_host(scsihost);
> scsi_host_put(scsihost);
>
> - idr_destroy(&devdata->idr);
> + xa_destroy(&xa_dtstr);
>
> dev_set_drvdata(&dev->device, NULL);
> debugfs_remove(devdata->debugfs_info);
What happens if you have two HBAs in the system, one is active and you
remove the other one?
More generally, the IDR required you call idr_destroy() to avoid leaking
preallocated memory. I changed that, but there are still many drivers
that have unnecessary calls to idr_destroy(). It's good form to just
delete them and not turn them into calls to xa_destroy().
