Re: [PATCH v2 4/5] dmaengine: plx-dma: Implement hardware initialization and cleanup

[Logan Gunthorpe]

On 2019-12-09 11:49 p.m., Jiasen Lin wrote:
> Integrated DMA engine of PEX87xx series switch support various
> interrupts. According to my personal experience, I suggest that
> enable error interrupt, invalid decscriptor interrupt, abort done
> interrupt, graceful puse done interrupt, and
> immediate pasue done interrupt by write  DMA Channel x Interrupt
> Control/Status register.

Well, that depends on what we want to do with these interrupts:

1) We shouldn't need to handle the error/invalid descriptor interrupt.
We instead just see that a specific descriptor failed (in the usual way)
and handle it accordingly. (Though this isn't really documented well).
An invalid descriptor should really never happen unless we have a driver
bug. I suppose I could print an error message if either occur.

2) We never send an abort or immediate pause to the device, so neither
interrupt can ever fire. So there's nothing to do if they do fire and
thus no sense enabling them.

3) We do send a graceful pause to the device on teardown but prefer to
poll for the end of the pause instead of adding the extra complexity to
waiting for an interrupt. So no need for the interrupt.

Logan



> 
> Thanks,
> Jiasen Lin
> 
>>   	kref_init(&plxdev->ref);
>>   	INIT_WORK(&plxdev->release_work, plx_dma_release_work);
>> +	spin_lock_init(&plxdev->ring_lock);
>> +	tasklet_init(&plxdev->desc_task, plx_dma_desc_task,
>> +		     (unsigned long)plxdev);
>>   
>> +	RCU_INIT_POINTER(plxdev->pdev, pdev);
>>   	plxdev->bar = pcim_iomap_table(pdev)[0];
>>   
>>   	dma = &plxdev->dma_dev;
>> @@ -169,6 +501,16 @@ static void plx_dma_remove(struct pci_dev *pdev)
>>   
>>   	free_irq(pci_irq_vector(pdev, 0),  plxdev);
>>   
>> +	rcu_assign_pointer(plxdev->pdev, NULL);
>> +	synchronize_rcu();
>> +
>> +	spin_lock_bh(&plxdev->ring_lock);
>> +	plxdev->ring_active = false;
>> +	spin_unlock_bh(&plxdev->ring_lock);
>> +
>> +	__plx_dma_stop(plxdev);
>> +	plx_dma_abort_desc(plxdev);
>> +
>>   	plxdev->bar = NULL;
>>   	plx_dma_put(plxdev);
>>   
>>