Re: [PATCH v2 5/8] accel/qaic: Add datapath

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

On Mon, Feb 06, 2023 at 08:41:42AM -0700, Jeffrey Hugo wrote:
> +#define SEM_VAL_MASK	GENMASK_ULL(11, 0)
> +#define SEM_INDEX_MASK	GENMASK_ULL(4, 0)
> +#define BULK_XFER	BIT(3)
> +#define GEN_COMPLETION	BIT(4)
> +#define INBOUND_XFER	1
> +#define OUTBOUND_XFER	2
> +#define REQHP_OFF	0x0 /* we read this */
> +#define REQTP_OFF	0x4 /* we write this */
> +#define RSPHP_OFF	0x8 /* we write this */
> +#define RSPTP_OFF	0xc /* we read this */
> +
> +#define ENCODE_SEM(val, index, sync, cmd, flags)			\
> +			((val) |					\
> +			(index) << 16 |					\
> +			(sync) << 22 |					\
> +			(cmd) << 24 |					\
> +			((cmd) ? BIT(31) : 0) |				\
> +			(((flags) & SEM_INSYNCFENCE) ? BIT(30) : 0) |	\
> +			(((flags) & SEM_OUTSYNCFENCE) ? BIT(29) : 0))

This could be probably better coded using FIELD_PREP()
with integrated checks of passed values not exceed
field width.

> +struct dbc_req { /* everything must be little endian encoded */

This comment does not provide much value IMHO ...

> +inline int get_dbc_req_elem_size(void)
> +{
> +	return sizeof(struct dbc_req);
> +}
> +
> +inline int get_dbc_rsp_elem_size(void)
> +{
> +	return sizeof(struct dbc_rsp);
> +}

.. as those those inliners, instead of using sizeof() directly.
Up to you to change.

> +static int reserve_pages(unsigned long start_pfn, unsigned long nr_pages,
> +			 bool reserve)
> +{
> +	unsigned long pfn;
> +	unsigned long end_pfn = start_pfn + nr_pages;
> +	struct page *page;
> +
> +	for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> +		if (!pfn_valid(pfn))
> +			return -EINVAL;
> +		page =  pfn_to_page(pfn);
> +		if (reserve)
> +			SetPageReserved(page);
> +		else
> +			ClearPageReserved(page);

It is needed? Looks like taken from some legacy code.

> +static int copy_sgt(struct qaic_device *qdev, struct sg_table **sgt_out,
> +		    struct sg_table *sgt_in, u64 size, u64 offset)
> +{
> +	int total_len, len, nents, offf = 0, offl = 0;
> +	struct scatterlist *sg, *sgn, *sgf, *sgl;
> +	struct sg_table *sgt;
> +	int ret, j;
> +
> +	/* find out number of relevant nents needed for this mem */
> +	total_len = 0;
> +	sgf = NULL;
> +	sgl = NULL;
> +	nents = 0;
> +
> +	size = size ? size : PAGE_SIZE;
> +	for (sg = sgt_in->sgl; sg; sg = sg_next(sg)) {
> +		len = sg_dma_len(sg);
> +
> +		if (!len)
> +			continue;
> +		if (offset >= total_len && offset < total_len + len) {
> +			sgf = sg;
> +			offf = offset - total_len;
> +		}
> +		if (sgf)
> +			nents++;
> +		if (offset + size >= total_len &&
> +		    offset + size <= total_len + len) {
> +			sgl = sg;
> +			offl = offset + size - total_len;
> +			break;
> +		}
> +		total_len += len;
> +	}
> +
> +	if (!sgf || !sgl) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
> +	if (!sgt) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = sg_alloc_table(sgt, nents, GFP_KERNEL);
> +	if (ret)
> +		goto free_sgt;
> +
> +	/* copy relevant sg node and fix page and length */
> +	sgn = sgf;
> +	for_each_sgtable_sg(sgt, sg, j) {
> +		memcpy(sg, sgn, sizeof(*sg));
> +		if (sgn == sgf) {
> +			sg_dma_address(sg) += offf;
> +			sg_dma_len(sg) -= offf;
> +			sg_set_page(sg, sg_page(sgn),
> +				    sg_dma_len(sg), offf);
> +		} else {
> +			offf = 0;
> +		}
> +		if (sgn == sgl) {
> +			sg_dma_len(sg) = offl - offf;
> +			sg_set_page(sg, sg_page(sgn),
> +				    offl - offf, offf);
> +			sg_mark_end(sg);
> +			break;
> +		}
> +		sgn = sg_next(sgn);
> +	}

Why not use sg_copy_table() ? Overall copy_sgt() seems to be something
that could be replaced by generic helper or at least simplify.

Regards
Stanislaw