linux-kernel - Re: [PATCH v2 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

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]

Message-ID: <20140430055418.GC20080@newterm.pl>
Date:	Wed, 30 Apr 2014 07:54:18 +0200
From:	Darek Marcinkiewicz <reksio@...term.pl>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

On Tue, Apr 29, 2014 at 03:45:08PM -0400, David Miller wrote:
> From: Darek Marcinkiewicz <reksio@...term.pl>
> Date: Mon, 28 Apr 2014 07:36:58 +0200
> 
> > +void *ec_bh_alloc_dma_mem(struct bh_priv *priv,
> > +			     int channel,
> > +			     u8 **buf,
> > +			     dma_addr_t *phys,
> > +			     dma_addr_t *phys_buf,
> > +			     size_t *len) {
> > +	u32 mask;
> > +	int offset = channel * DMA_CHAN_SIZE + DMA_CHAN_OFFSET;
> > +
> > +	iowrite32(0xffffffff, priv->dma_io + offset);
> > +
> > +	mask = ioread32(priv->dma_io + offset);
> > +	mask &= 0xfffffffc;
> > +	dev_info(PRIV_TO_DEV(priv),
> > +		 "Read mask %x for channel %d\n",
> > +		 mask, channel);
> > +	*len = ~mask + 1;
> > +
> > +	dev_info(PRIV_TO_DEV(priv),
> > +		 "Allocating %d bytes for channel %d",
> > +		 (int)*len, channel);
> > +	*buf = pci_alloc_consistent(priv->dev,
> > +				    *len * 2,
> > +				    phys_buf);
> 
> This is really confusing, the log message says that it allocates
> "*len" bytes, but actually you are allocating "*len * 2" bytes as
> per the arguments you are passing into pci_alloc_consistent.
> 
> Either one or the other is wrong, and if "*len * 2" is the correct
> size then it should be explained why you're doubling this value
> but providing just plain "*len" to the caller.
> 
> All of these *len values seem to be doubled over and over again,
> in the memset() calls, in the pci_free_consistent() calls during
> resource release on probe error and driver shutdown.
> 
> Why not just assign X * 2 to *len and get rid of this doubling all
> over the place?
> 
This device imposes some limitations on location and size of the buffers
that it dma data from/to. It has a "mask" parameter saying two things:
1. RX/TX buffers have to be aligned to "mask" bits.
2. Their max size is 2^mask.
In order to have a buffer meeting above criteria driver allocates
chunk of size 2 * max buffer size, and half of this chunk gets
used later on. So, len here is the size of that aligned buffer, whereas
2 * len is how much memory was actually allocated. 

So, yes, this is a little bit confusing. I'll try to clean this up
a little bit and send updated patch.

Thank you!

-- 
DM

--
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/