Re: [PATCH v7] staging: unisys: visorhba: Convert module from IDR to XArray

[Fabio M. De Francesco]

On Tuesday, May 4, 2021 3:42:16 PM CEST Matthew Wilcox wrote:
> On Tue, May 04, 2021 at 03:32:53PM +0200, Fabio M. De Francesco wrote:
> > Changes from v6; Added a call to xa_destroy() that I had forgotten.
> 
> What?  No!  Go back and re-read what I wrote about this previously.
>
I remember that explanation you gave me some days ago for not using it. But I 
was mislead by a comment ("Do we not have to call xa_destroy()?") by Dan and 
your "Correct" soon after the above comment. So I thought that I had 
misunderstand and the put back that call to xa_destroy(). I lost something in 
following the flow of the reviews, I suppose.
> 
> > +static int setup_scsitaskmgmt_handles(struct xarray *xa, struct uiscmdrsp 
*cmdrsp,
> > 
> >  				       wait_queue_head_t *event, 
int *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);
> > +	int ret;
> > +	u32 id;
> > +
> > +	/* specify the event that has to be triggered when this cmd is 
complete */
> > +	ret = xa_alloc_irq(xa, &id, event, xa_limit_32b, GFP_KERNEL);
> > +	if (ret)
> > +		return ret;
> > +	else
> > +		cmdrsp->scsitaskmgmt.notify_handle = id;
> 
> This 'else' is actively confusing.
>
Unnecessary and redundant, yes.
> 
> > +	ret = xa_alloc_irq(xa, &id, result, xa_limit_32b, GFP_KERNEL);
> > +	if (ret) {
> > +		xa_erase_irq(xa, cmdrsp->scsitaskmgmt.notify_handle);
> > +		return ret;
> > +	} else
> > +		cmdrsp->scsitaskmgmt.notifyresult_handle = id;
> 
> Ditto.
>
Redundant, again.

Thanks,

Fabio