Re: [PATCH 07/10] esp: Use FIFO for command submission

[Hannes Reinecke <hare@suse.de>]

On 11/21/2014 11:04 AM, Paolo Bonzini wrote:
> 
> 
> On 21/11/2014 10:27, Hannes Reinecke wrote:
>> The am53c974 has a design flaw causing it to throw an
>> DMA interrupt whenever a DMA transmission completed,
>> even though DMA interrupt reporting is disabled.
>> This confuses the esp routines as it would register
>> a DMA interrupt whenever a cdb has been transmitted
>> to the drive.
>> So implement an option to use the FIFO for command
>> submission and enable it for am53c974.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/scsi/am53c974.c |  1 +
>>  drivers/scsi/esp_scsi.c | 83 ++++++++++++++++++++++++++++++++++++++-----------
>>  drivers/scsi/esp_scsi.h |  1 +
>>  3 files changed, 66 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/scsi/am53c974.c b/drivers/scsi/am53c974.c
>> index b9af8b0..652762e 100644
>> --- a/drivers/scsi/am53c974.c
>> +++ b/drivers/scsi/am53c974.c
>> @@ -401,6 +401,7 @@ static int pci_esp_probe_one(struct pci_dev *pdev,
>>  	esp->host = shost;
>>  	esp->dev = pdev;
>>  	esp->ops = &pci_esp_ops;
>> +	esp->flags |= ESP_FLAG_USE_FIFO;
> 
> Why not invert this patch and the previous one, and add this line
> directly when the am53c974 driver is born?
> 
I've added it as separate patch as the original esp driver would
always be using DMA for CDB transfer.
And you could actually use the am53c974 driver with that, only you'd
see lots of 'spurious IRQ' messages.

But for the order I don't have any preference; sure I can rearrange
those.

>>  	pep->esp = esp;
>>  
>>  	if (pci_request_regions(pdev, DRV_MODULE_NAME)) {
>> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
>> index 92ab921..ab7d2bc 100644
>> --- a/drivers/scsi/esp_scsi.c
>> +++ b/drivers/scsi/esp_scsi.c
>> @@ -605,7 +605,7 @@ static void esp_autosense(struct esp *esp, struct esp_cmd_entry *ent)
>>  {
>>  	struct scsi_cmnd *cmd = ent->cmd;
>>  	struct scsi_device *dev = cmd->device;
>> -	int tgt, lun;
>> +	int tgt, lun, i;
>>  	u8 *p, val;
>>  
>>  	tgt = dev->id;
>> @@ -626,7 +626,10 @@ static void esp_autosense(struct esp *esp, struct esp_cmd_entry *ent)
>>  
>>  	esp->active_cmd = ent;
>>  
>> -	p = esp->command_block;
>> +	if (esp->flags & ESP_FLAG_USE_FIFO)
>> +		p = esp->fifo;
>> +	else
>> +		p = esp->command_block;
> 
> Any reason not to use esp->command_block unconditionally?
> 
'command_block' is actually mapped onto PCI DMA space, the FIFO is
not. Plus I'd thought to use the 'fifo' array here to make things
more obvious about what's going on.
For FIFO submission both are pretty close, so, yeah, I could be
using command_block here.

>>  	esp->msg_out_len = 0;
>>  
>>  	*p++ = IDENTIFY(0, lun);
>> @@ -648,12 +651,21 @@ static void esp_autosense(struct esp *esp, struct esp_cmd_entry *ent)
>>  	esp_write_tgt_sync(esp, tgt);
>>  	esp_write_tgt_config3(esp, tgt);
>>  
>> -	val = (p - esp->command_block);
>> +	if (esp->flags & ESP_FLAG_USE_FIFO)
>> +		val = (p - esp->fifo);
>> +	else
>> +		val = (p - esp->command_block);
>>  
>>  	if (esp->rev == FASHME)
>>  		scsi_esp_cmd(esp, ESP_CMD_FLUSH);
> 
> For consistency with what you do elsewhere, please move this in the "else".
> 
No. The 'USE_FIFO' mechanism is a general feature which _should_
work on all chips. The above 'if' condition is a chipset-specific
feature which should be treated separately.

>> -	esp->ops->send_dma_cmd(esp, esp->command_block_dma,
>> -			       val, 16, 0, ESP_CMD_DMA | ESP_CMD_SELA);
>> +	if (esp->flags & ESP_FLAG_USE_FIFO) {
>> +		scsi_esp_cmd(esp, ESP_CMD_FLUSH);
>> +		for (i = 0; i < val; i++)
>> +			esp_write8(esp->fifo[i], ESP_FDATA);
>> +		scsi_esp_cmd(esp, ESP_CMD_SELA);
>> +	} else
>> +		esp->ops->send_dma_cmd(esp, esp->command_block_dma,
>> +				       val, 16, 0, ESP_CMD_DMA | ESP_CMD_SELA);
>>  }
> 
> Hmm, what about a wrapper
> 
>     __send_cmd(esp, data, esp_count, dma_count, cmd, force_flush,
>                force_fifo)
>     {
>         use_fifo =
>             force_fifo || (esp->flags & ESP_FLAG_USE_FIFO) ||
>             esp_count == 1;
>         if (force_flush || use_fifo || esp->rev == FASHME)
>             scsi_esp_cmd(esp, ESP_CMD_FLUSH);
>         if (use_fifo) {
>             for (i = 0; i < val; i++)
>                 esp_write8(esp->fifo[i], ESP_FDATA);
>             scsi_esp_cmd(esp, cmd);
>         } else {
>             if (data != esp->command_block)
>                 memcpy(esp->command_block, data, length)
>             esp->ops->send_dma_cmd(esp,
>                                    esp->command_block_dma,
>                                    esp_count, dma_count, 0,
>                                    cmd | ESP_CMD_DMA);
>         }
>     }
> 
>     send_cmd(esp, data, esp_count, dma_count, cmd)
>     {
>         __send_cmd(esp, data, esp_count, dma_count, cmd, false, false);
>     }
> 
> This would be:
> 
>     send_cmd(esp, esp->command_block, val, 16, ESP_CMD_SELA);
> 
Good point.

Will be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html