linux-kernel - Re: [PATCH] hpsa: SCSI driver for HP Smart Array controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 03 Mar 2009 10:49:15 -0600
From:	Mike Christie <michaelc@...wisc.edu>
To:	scameron@...rdog.cca.cpqcorp.net
CC:	linux-kernel@...r.kernel.org, mike.miller@...com,
	jens.axboe@...cle.com, fujita.tomonori@....ntt.co.jp,
	akpm@...ux-foundation.org, linux-scsi@...r.kernel.org,
	coldwell@...hat.com, hare@...ell.com, iss_storagedev@...com
Subject: Re: [PATCH] hpsa: SCSI driver for HP Smart Array controllers

scameron@...rdog.cca.cpqcorp.net wrote:
> [...]
>>> +/*
>>> + * For operations that cannot sleep, a command block is allocated at init,
>>> + * and managed by cmd_alloc() and cmd_free() using a simple bitmap to track
>>> + * which ones are free or in use.  Lock must be held when calling this.
>>> + * cmd_free() is the complement.
>>> + */
>>> +static struct CommandList_struct *cmd_alloc(struct ctlr_info *h)
>>> +{
>>> +     struct CommandList_struct *c;
>>> +     int i;
>>> +     union u64bit temp64;
>>> +     dma_addr_t cmd_dma_handle, err_dma_handle;
>>> +
>>> +     do {
>>> +             i = find_first_zero_bit(h->cmd_pool_bits, h->nr_cmds);
>>> +             if (i == h->nr_cmds)
>>> +                     return NULL;
>>> +     } while (test_and_set_bit
>>> +              (i & (BITS_PER_LONG - 1),
>>> +               h->cmd_pool_bits + (i / BITS_PER_LONG)) != 0);
>> Using bitmap to manage free commands looks too complicated a bit to
>> me. Can we just use lists for command management?
> 
> Hmm, this doesn't seem all that complicated to me, and this code snippet
> has been pretty stable for about 10 years. it's nearly identical to what's in
> cpqarray in the 2.2.13 kernel from 1999:
> 
>                 do {
>                         i = find_first_zero_bit(h->cmd_pool_bits, NR_CMDS);
>                         if (i == NR_CMDS)
>                                 return NULL;
>                 } while(test_and_set_bit(i%32, h->cmd_pool_bits+(i/32)) != 0)
> 
> It's fast, works well, and has needed very little maintenance over the 
> years.  Without knowing what you have in mind specifically, I don't see a
> big need to change this.
> 

Other drivers have had to convert to and modify the host tagging to get 
merged. They too had stable and fast code, and we complained and fought 
against changing it :) I have had to convert or help convert libfc and 
qla4xxx and I will now convert iscsi, so I feel your pain :)

To create the map call scsi_init_shared_tag_map after you allocate the 
scsi host and before you add it. Then in slave_alloc set the 
sdev->tag_supported  = 1 and call scsi_activate_tcq. Then replace your 
bitmap with:

for scsi commands:

c = h->cmd_pool + scsi_cmnd->request->tag

And for the reset path I was thinking you could use my patch in the 
other mail and do

tag = blk_map_start_tag(scsi_host->bqt, NULL, 0);
if (tag < 0)
	goto fail;
c = h->cmd_pool + tag;
c->cmdindex = tag;

In the completion path you need to do a blk_map_end_tag(scsi_host->bqt, 
c->cmdindex);. The scsi/block layer will free the tag for scsi commadns.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/