Re: [PATCH 14/31] hpsa: use reserved commands

[michael.christie@oracle.com]

On 2/22/21 7:23 AM, Hannes Reinecke wrote:
> -
> -static struct CommandList *cmd_alloc(struct ctlr_info *h)
> +static struct CommandList *cmd_alloc(struct ctlr_info *h, u8 direction)
>  {
> +	struct scsi_cmnd *scmd;
>  	struct CommandList *c;
> -	int refcount, i;
> -	int offset = 0;
> -
> -	/*
> -	 * There is some *extremely* small but non-zero chance that that
> -	 * multiple threads could get in here, and one thread could
> -	 * be scanning through the list of bits looking for a free
> -	 * one, but the free ones are always behind him, and other
> -	 * threads sneak in behind him and eat them before he can
> -	 * get to them, so that while there is always a free one, a
> -	 * very unlucky thread might be starved anyway, never able to
> -	 * beat the other threads.  In reality, this happens so
> -	 * infrequently as to be indistinguishable from never.
> -	 *
> -	 * Note that we start allocating commands before the SCSI host structure
> -	 * is initialized.  Since the search starts at bit zero, this
> -	 * all works, since we have at least one command structure available;
> -	 * however, it means that the structures with the low indexes have to be
> -	 * reserved for driver-initiated requests, while requests from the block
> -	 * layer will use the higher indexes.
> -	 */
> -
> -	for (;;) {
> -		i = find_next_zero_bit(h->cmd_pool_bits,
> -					HPSA_NRESERVED_CMDS,
> -					offset);
> -		if (unlikely(i >= HPSA_NRESERVED_CMDS)) {
> -			offset = 0;
> -			continue;
> -		}
> -		c = h->cmd_pool + i;
> -		refcount = atomic_inc_return(&c->refcount);
> -		if (unlikely(refcount > 1)) {
> -			cmd_free(h, c); /* already in use */
> -			offset = (i + 1) % HPSA_NRESERVED_CMDS;
> -			continue;
> -		}
> -		set_bit(i & (BITS_PER_LONG - 1),
> -			h->cmd_pool_bits + (i / BITS_PER_LONG));
> -		break; /* it's ours now. */
> +	int idx;
> +
> +	scmd = scsi_get_internal_cmd(h->raid_ctrl_sdev,
> +				     (direction & XFER_WRITE) ?
> +				     DMA_TO_DEVICE : DMA_FROM_DEVICE,
> +				     REQ_NOWAIT);
> +	if (!scmd) {
> +		dev_warn(&h->pdev->dev, "failed to allocate reserved cmd\n");
> +		return NULL;

I think in the orig code cmd_alloc would always return a non null pointer.
It looks like we would always just keep looping.

Now, it looks like we could fail from the above code where we return NULL.
I was not sure if it's maybe impossible to hit the "return NULL" becuase we
only call this function when we know there will be a cmd availale. If we
can fail then the cmd_alloc callers should check for NULL now I think.