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

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

On 2/24/2023 8:25 AM, Stanislaw Gruszka wrote:
> 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.

Interesting idea.  Will have a look.

> 
>> +struct dbc_req { /* everything must be little endian encoded */
> 
> This comment does not provide much value IMHO ...

With the LE types, this does seem to be redundant now.  Will remove.

> 
>> +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.

Will review these.

> 
>> +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.

Required for remap_pfn_range().

>> +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.

I don't see "sg_copy_table" defined in 6.2.  Are you suggesting renaming 
this function?  I guess I'm not quite understanding your comment here. 
Can you elaborate?