Re: [PATCH v3] cxl: Export AFU error buffer via sysfs

[Michael Neuling <mikey@neuling.org>]

+ */
> +ssize_t cxl_afu_read_err_buffer(struct cxl_afu *afu, char *buf,
> +				loff_t off, size_t count)
> +{
> +	loff_t aligned_off;
> +	size_t aligned_count;
> +	const void __iomem *ebuf =3D afu->afu_desc_mmio + afu->eb_offset;
> +
> +	if (!afu->eb_len || count =3D=3D 0 || off < 0)

if eb_len =3D=3D 0 we don't even create the sysfs file.  So is this check
needed?

> +		return 0;
> +
> +	/* calculate aligned read window */
> +	count =3D min((size_t)(afu->eb_len - off), count);

What if count ends up being negative because off > afu->eb_len??

> +	aligned_off =3D round_down(off, 8);
> +	aligned_count =3D round_up(off + count, 8) - aligned_off;

I kinda preferred the start end variables, and length was just end -
start.

I though it was more readable. IMHO

How about:

	aligned_start =3D round_down(off, 8);
	aligned_end =3D round_up(off + count, 8);
	aligned_length =3D aligned_end - aligned_start;

> +
> +	/* fast path */
> +	if ((aligned_off =3D=3D off) && (aligned_count =3D=3D count)) {
> +		/* no need to use the bounce buffer */
> +		_memcpy_fromio(buf, ebuf + off, count);

I would drop this, as the other code path should work fine.
Premature optimisation.....

> +
> +	} else {
> +		/* use bounce buffer for copy */
> +		void *tbuf =3D (void *)__get_free_page(GFP_TEMPORARY);
> +
> +		if (!tbuf)
> +			return -ENOMEM;
> +
> +		/* max we can copy in one read is PAGE_SIZE */
> +		aligned_count =3D min(aligned_count, PAGE_SIZE);
> +		_memcpy_fromio(tbuf, ebuf + aligned_off, aligned_count);
> +
> +		count =3D min(count, aligned_count);

This doesn't seem right.  count will equal PAGE_SIZE if it's too big but
it has to be smaller by (off & 7) in this case.

How about this?

        #define MAX_COPY_SIZE PAGE_SIZE
	...

	void *bbuf;

	/* Bounds check count with err buf length */
	count =3D min((size_t)(afu->eb_len - off), count);
	if ((off < 0) || (count < 0))
		return 0;

	/* Create aligned bounce buffer to copy into */
	aligned_start =3D round_down(off, 8);
	aligned_end =3D round_up(off + count, 8);
	aligned_length =3D aligned_end - aligned_start;

	if (aligned_length > MAX_COPY_SIZE) {
		aligned_length =3D MAX_COPY_SIZE;
		count =3D MAX_COPY_SIZE - (off & 0x7);
        }

	bbuf =3D (void *)__get_free_page(GFP_TEMPORARY);
 	if (!bbuf)
		return -ENOMEM;

	/* Use _memcpy_fromio() so the reads are aligned */
	_memcpy_fromio(bbuf, ebuf + aligned_start, aligned_length);
	memcpy(buf, bbuf + (off & 0x7), count);

	free_page(bbuf);


> +		memcpy(buf, tbuf + (off & 0x7), count);
> +
> +		free_page((unsigned long)tbuf);
> +	}
> +
> +	return count;