RE: [PATCH v3 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma

[Appana Durga Kedareswara Rao]

Hi 

	Thanks for the review...
> 
> Hi Kedar,
> 
> 
> On 04-01-2017 06:54, Kedareswara rao Appana wrote:
> > When VDMA is configured for more than one frame in the h/w for example
> > h/w is configured for n number of frames and user Submits n number of
> > frames and triggered the DMA using issue_pending API.
> > In the current driver flow we are submitting one frame at a time but
> > we should submit all the n number of frames at one time as the h/w Is
> > configured for n number of frames.
> >
> > This patch fixes this issue.
> >
> > Signed-off-by: Kedareswara rao Appana <appanad@xxxxxxxxxx>
> 
> Looks fine. I have a couple of minor comments, if you address them you can add
> "Reviewed-by: Jose Abreu <joabreu@xxxxxxxxxxxx>"
> in next version.

Thanks...
Sure will fix the comments and will add your' s Reviewed-by....

[snip]
> > +	/*
> > +	 * Note: When VDMA is built with default h/w configuration
> > +	 * On the S2MM(recv) side user should submit frames upto
> > +	 * H/W configured. If users submits less than h/w configured
> > +	 * VDMA engine tries to write to a invalid location
> > +	 * Results undefined behaviour/memory corruption.
> > +	 *
> > +	 * If user would like to submit frames less than h/w capable
> > +	 * On S2MM side please enable debug info 13 at the h/w level
> > +	 * It will allows the frame buffers numbers to be modified at runtime.
> > +	 */
> > +	if (!chan->has_fstoreconfig && chan->direction == DMA_DEV_TO_MEM
> &&
> > +	    chan->desc_pendingcount < chan->num_frms) {
> > +		dev_dbg(chan->dev, "Frame Store Configuration is not enabled
> at the");
> > +		dev_dbg(chan->dev, " H/w level enable Debug info 13 at the
> h/w level");
> > +		dev_dbg(chan->dev, " OR Submit the frames upto h/w
> Capable\n\r");
> > +
> > +		return;
> > +	}
> 
> Hmm, may dev_warn would be more suitable because with dev_dbg and no
> dynamic debug enabled user will not know what happened. Also, I am aware
> that in direction DMA_MEM_TO_DEV there will be no corruption in PC side but it
> will be corruption in VDMA side because it will read from invalid memory
> locations. Maybe drop the check for channel direction.

Sure will fix it in the next version....

> 
> I am also not fancy about dropping prints that are not grep'able (you do not
> break line in each print so a user searching for the whole string will not find it).
> Try to do a line break in each print or change the string to be smaller.
> 

Sure will add a line break in each print in the next version...

Regards,
Kedar.

> Best regards,
> Jose Miguel Abreu
>