linux-kernel - Re: [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA engine driver

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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 16 Mar 2015 16:57:30 +0530
From:	Vinod Koul <vinod.koul@...el.com>
To:	Rameshwar Sahu <rsahu@....com>
Cc:	dan.j.williams@...el.com, dmaengine@...r.kernel.org,
	Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	ddutile@...hat.com, jcm@...hat.com, patches@....com,
	Loc Ho <lho@....com>
Subject: Re: [PATCH v7 1/3] dmaengine: Add support for APM X-Gene SoC DMA
 engine driver

On Mon, Mar 16, 2015 at 04:00:22PM +0530, Rameshwar Sahu wrote:

> >> +static struct xgene_dma_desc_sw *xgene_dma_alloc_descriptor(
> >> +                              struct xgene_dma_chan *chan)
> >> +{
> >> +     struct xgene_dma_desc_sw *desc;
> >> +     dma_addr_t phys;
> >> +
> >> +     desc = dma_pool_alloc(chan->desc_pool, GFP_NOWAIT, &phys);
> >> +     if (!desc) {
> >> +             chan_dbg(chan, "Failed to allocate LDs\n");
> > not error?
> 
> Yes it's error only by lacking of dma memory, do I need to use dev_err
> to show the error msg ??
yes

> 
> >
> >> +static void xgene_dma_free_desc_list_reverse(struct xgene_dma_chan *chan,
> >> +                                          struct list_head *list)
> > do we really care about free order?
> 
> Yes it start dellocation of descriptor by tail.
and why by tail is not clear.

> > where are you mapping dma buffers?
> 
>  I didn't get you here. Can you please explain me here what you mean.
> As per my understanding client should map the dma buffer and give the
> physical address and size to this callback prep routines.
not for memcpy, that is true for slave transfers

For mempcy the idea is that drivers will do buffer mapping

> > why are you calling this here, status check shouldnt do this...
> 
> Okay, I will remove it.
> 
> 
> >> +                     spin_unlock_bh(&chan->lock);
> >> +                     return DMA_IN_PROGRESS;
> > residue here is size of transacation.
> 
> We can't calculate here residue size. We don't have any controller
> register which will tell about remaining transaction size.
Okay if you cant calculate residue why do we have this fn?

> 
> >> +             }
> >> +     }
> >> +
> >> +     /* Check if this descriptor is in running queue */
> >> +     list_for_each_entry(desc, &chan->ld_running, node) {
> >> +             if (desc->tx.cookie == cookie) {
> >> +                     /* Cleanup any running and executed descriptors */
> >> +                     xgene_dma_cleanup_descriptors(chan);
> > ditto?
> 
> Okay
> 
> 
> >> +                     spin_unlock_bh(&chan->lock);
> >> +                     return dma_cookie_status(&chan->dma_chan,
> >> +                                              cookie, txstate);
> > and you havent touched txstate so what is the intent here...?
> 
> txstate can filled by caller, so it may be NULL or not NULL, we are
> passing same.
something seems very wrong here. Status should return the current satue of
queried descriptor and if residue value in txstate, you seem to be doing
something else, quotesion is what and why :)

-- 
~Vinod

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/