RE: [v6 3/7] dmaengine: fsl-qdma: Add qDMA controller driver for Layerscape SoCs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Vinod [mailto:vkoul@xxxxxxxxxx]
> Sent: 2018年6月28日 13:17
> To: Wen He <wen.he_1@xxxxxxx>
> Cc: dmaengine@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; Jiafei Pan
> <jiafei.pan@xxxxxxx>; Jiaheng Fan <jiaheng.fan@xxxxxxx>
> Subject: Re: [v6 3/7] dmaengine: fsl-qdma: Add qDMA controller driver for
> Layerscape SoCs
> 
> On 15-06-18, 18:22, Wen He wrote:
> 
> > +static struct fsl_qdma_comp *fsl_qdma_request_enqueue_desc(
> > +					struct fsl_qdma_chan *fsl_chan,
> > +					unsigned int dst_nents,
> > +					unsigned int src_nents)
> > +{
> > +	struct fsl_qdma_comp *comp_temp;
> > +	struct fsl_qdma_queue *queue = fsl_chan->queue;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&queue->queue_lock, flags);
> > +	if (list_empty(&queue->comp_free)) {
> > +		spin_unlock_irqrestore(&queue->queue_lock, flags);
> > +		comp_temp = kzalloc(sizeof(*comp_temp), GFP_KERNEL);
> 
> this is invoked from fsl_qdma_prep_memcpy, so needs to be GFP_NOWAIT
> flag
> 

OK

> > +		if (!comp_temp)
> > +			return NULL;
> > +
> > +		comp_temp->virt_addr = dma_pool_alloc(queue->comp_pool,
> > +						      GFP_KERNEL,
> 
> here as well
> 
> > +static struct fsl_qdma_queue *fsl_qdma_alloc_queue_resources(
> > +					struct platform_device *pdev,
> > +					unsigned int queue_num)
> > +{
> > +	struct fsl_qdma_queue *queue_head, *queue_temp;
> > +	int ret, len, i;
> > +	unsigned int queue_size[FSL_QDMA_QUEUE_MAX];
> > +
> > +	if (queue_num > FSL_QDMA_QUEUE_MAX)
> > +		queue_num = FSL_QDMA_QUEUE_MAX;
> > +	len = sizeof(*queue_head) * queue_num;
> > +	queue_head = devm_kzalloc(&pdev->dev, len, GFP_KERNEL);
> 
> we have kcalloc variant for this
> 

OK

> > +	if (!queue_head)
> > +		return NULL;
> > +
> > +	ret = device_property_read_u32_array(&pdev->dev, "queue-sizes",
> > +					queue_size, queue_num);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't get queue-sizes.\n");
> > +		return NULL;
> > +	}
> > +
> > +	for (i = 0; i < queue_num; i++) {
> > +		if (queue_size[i] > FSL_QDMA_CIRCULAR_DESC_SIZE_MAX ||
> > +			    queue_size[i] < FSL_QDMA_CIRCULAR_DESC_SIZE_MIN)
> {
> > +			dev_err(&pdev->dev, "Get wrong queue-sizes.\n");
> > +			return NULL;
> > +		}
> > +		queue_temp = queue_head + i;
> > +		queue_temp->cq = dma_alloc_coherent(&pdev->dev,
> > +						sizeof(struct fsl_qdma_format) *
> > +						queue_size[i],
> > +						&queue_temp->bus_addr,
> > +						GFP_KERNEL);
> > +		if (!queue_temp->cq) {
> > +			devm_kfree(&pdev->dev, queue_head);
> 
> any reason this is freed explicitly
> 
> > +static int fsl_qdma_halt(struct fsl_qdma_engine *fsl_qdma) {
> > +	void __iomem *ctrl = fsl_qdma->ctrl_base;
> > +	void __iomem *block = fsl_qdma->block_base;
> > +	int i, count = 5;
> 
> why 5
> 

Nothing, a condition.
Maybe I should be set a safeguards timers in here(count=N).

> > +static int fsl_qdma_queue_transfer_complete(struct fsl_qdma_engine
> > +*fsl_qdma) {
> > +	struct fsl_qdma_queue *fsl_queue = fsl_qdma->queue;
> > +	struct fsl_qdma_queue *fsl_status = fsl_qdma->status;
> > +	struct fsl_qdma_queue *temp_queue;
> > +	struct fsl_qdma_comp *fsl_comp;
> > +	struct fsl_qdma_format *status_addr;
> > +	struct fsl_qdma_format *csgf_src;
> > +	struct fsl_pre_status pre;
> > +	void __iomem *block = fsl_qdma->block_base;
> > +	u32 reg, i;
> > +	bool duplicate, duplicate_handle;
> > +
> > +	memset(&pre, 0, sizeof(struct fsl_pre_status));
> > +
> > +	while (1) {
> 
> this loop can potentially read bad data from HW and keep spining forever.
> Good practice would be to add safeguards and loop 'N' times which you deem
> is large enough to process
> 

Yes, it's a good idea.

> > +		duplicate = 0;
> > +		duplicate_handle = 0;
> 
> empty line here
> 
> > +		reg = qdma_readl(fsl_qdma, block + FSL_QDMA_BSQSR);
> > +		if (reg & FSL_QDMA_BSQSR_QE)
> > +			return 0;
> 
> here..
> 
> > +		status_addr = fsl_status->virt_head;
> > +		if (qdma_ccdf_get_queue(status_addr) == pre.queue &&
> > +			qdma_ccdf_addr_get64(status_addr) == pre.addr)
> > +			duplicate = 1;
> 
> here (basically after each logical block to help read)
> 

got it.

> > +		i = qdma_ccdf_get_queue(status_addr);
> > +		pre.queue = qdma_ccdf_get_queue(status_addr);
> > +		pre.addr = qdma_ccdf_addr_get64(status_addr);
> > +		temp_queue = fsl_queue + i;
> > +		spin_lock(&temp_queue->queue_lock);
> > +		if (list_empty(&temp_queue->comp_used)) {
> > +			if (duplicate) {
> > +				duplicate_handle = 1;
> 
> why do this, you can remove duplicate_handle and use duplicate below?

Thank you, I will remove it in next version.
Best Regards,
Wen

> 
> --
> ~Vinod
?韬{.n?????%??檩??w?{.n???Ё?撖)?骅w*jg????????G??⒏⒎?:+v????????????"??????



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux PCI]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux