lkml.org 
[lkml]   [2015]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 5/8] lib: introduce sg_nents_len_chained
From
Date
On 09/18/2015 08:57 AM, LABBE Corentin wrote:
> + for (nents = 0, total = 0; sg; sg = sg_next(sg)) {
> + nents++;
> + total += sg->length;
> + if (!sg_is_last(sg) && (sg + 1)->length == 0 && chained)
> + *chained = true;
> + if (total >= len)
> + return nents;
> + }
> +
>

(resending with fixed formatting; Thunderbird seems braindamaged lately)

It seems to me like the check for total >= len should be above the check
for chaining. The way the code is now, it will return chained = true if
the first "unneeded" sg vector is a chain, which does not make intuitive
sense.

But why do drivers even need this at all? Here is a typical usage:

int qce_mapsg(struct device *dev, struct scatterlist *sg, int nents,
enum dma_data_direction dir, bool chained)
{
int err;

if (chained) {
while (sg) {
err = dma_map_sg(dev, sg, 1, dir);
if (!err)
return -EFAULT;
sg = sg_next(sg);
}
} else {
err = dma_map_sg(dev, sg, nents, dir);
if (!err)
return -EFAULT;
}

return nents;
}

Here is another:

static int talitos_map_sg(struct device *dev, struct scatterlist *sg,
unsigned int nents, enum dma_data_direction dir,
bool chained)
{
if (unlikely(chained))
while (sg) {
dma_map_sg(dev, sg, 1, dir);
sg = sg_next(sg);
}
else
dma_map_sg(dev, sg, nents, dir);
return nents;
}

Can anyone clarify why you can't just use dma_map_sg(dev, sg, nents,
dir) always? It should be able to handle chained scatterlists just fine.

If the check for chaining is a just workaround for some problem in
dma_map_sg(), maybe it would be better to fix dma_map_sg() instead,
which would eliminate the need for sg_nents_len_chained() and all these
buggy workarounds (e.g. if chained is true, qce_mapsg() can leave the
DMA list partially mapped when it returns -EFAULT, and talitos_map_sg()
doesn't even check for errors).

One problem that I see is that sg_last() in scatterlist.c has a
"BUG_ON(!sg_is_last(ret));" if CONFIG_DEBUG_SG is enabled, and using a
smaller-than-original nents (as returned by sg_nents_len_chained()) with
the same scatterlist will trigger that bug. But that should be true
regardless of whether chaining is used or not. For example, talitos.c
calls sg_last() in a way that can trigger that bug.

For anyone willing to dig further, these are the first two commits that
introduce code like this:

4de9d0b547b9 "crypto: talitos - Add ablkcipher algorithms" (2009)
643b39b031f5 "crypto: caam - chaining support" (2012)
(CC'ing the original authors)

Tony Battersby
Cybernetics



\
 
 \ /
  Last update: 2015-09-18 18:41    [W:0.088 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site