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

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 4 Jan 2017 13:30:00 +0000
From:   Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "vinod.koul@...el.com" <vinod.koul@...el.com>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        Soren Brinkmann <sorenb@...inx.com>,
        "moritz.fischer@...us.com" <moritz.fischer@...us.com>,
        "laurent.pinchart@...asonboard.com" 
        <laurent.pinchart@...asonboard.com>,
        "luis@...ethencourt.com" <luis@...ethencourt.com>
CC:     "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v3 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame
 stores scenario in vdma

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@...inx.com>
> 
> Looks fine. I have a couple of minor comments, if you address them you can add
> "Reviewed-by: Jose Abreu <joabreu@...opsys.com>"
> 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
>