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
