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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
