Re: [PATCH v3 3/4] fpga: dfl: add basic support for DFHv1

[Andy Shevchenko]

On Tue, Oct 04, 2022 at 07:37:17AM -0700, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
> From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> 
> Add generic support for MSIX interrupts for DFL devices.

$ git grep -n -w MSI[xX] | wc -l
421

$ git grep -n -w MSI-[xX] | wc -l
1224

MSI-X (This is I believe the official name for that)

And everywhere.

> The location of a feature's registers is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.

...

> +	ddev->csr_res.start = feature->csr_res.start;
> +	ddev->csr_res.end = feature->csr_res.end;
> +	ddev->csr_res.flags = IORESOURCE_MEM;

Why simple assignment of the resource can't work?

	ddev->csr_res = feature->csr_res;

(I know the downside of this, but still)

...

> +		feature->csr_res.start = finfo->csr_res.start;
> +		feature->csr_res.end = finfo->csr_res.end;

Ditto.

...

> +	case 0:
> +		type = feature_dev_id_type(binfo->feature_dev);
> +		if (type == PORT_ID) {
> +			switch (fid) {
> +			case PORT_FEATURE_ID_UINT:
> +				v = readq(base + PORT_UINT_CAP);
> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +				break;
> +			case PORT_FEATURE_ID_ERROR:
> +				v = readq(base + PORT_ERROR_CAP);
> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +				break;

No default?

> +			}
> +		} else if (type == FME_ID) {

> +			if (fid == FME_FEATURE_ID_GLOBAL_ERR) {

Don't remember if that was discussed already or not, but

I would use switch-case here as well in order to be consistent with the
previous code piece pattern.

> +				v = readq(base + FME_ERROR_CAP);
> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +			}
> +		}
> +		break;

...

> +		if (v & DFHv1_CSR_ADDR_REL)
> +			finfo->csr_res.start = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +		else
> +			finfo->csr_res.start = binfo->start + ofst
> +					       + FIELD_GET(DFHv1_CSR_ADDR_MASK, v);

Locate + on the previous line.

> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		finfo->csr_res.end = finfo->csr_res.start
> +				     + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;

Ditto.

...

> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param)
> +{
> +	int off = DFHv1_PARAM_HDR;
> +	u64 v, next;
> +
> +	while (off < max) {
> +		v = readq(base + off);
> +		if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))

> +			return (DFHv1_PARAM_DATA + off);

Too many parentheses.

> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!next)
> +			break;
> +
> +		off += next;
> +	}
> +
> +	return -ENOENT;
> +}

The entire function seems a bit dangerous to me. You can ask for any max which
covers (up to) 64-bit address space and then do MMIO by basically arbitrary
address. How do you protect against wrong MMIO window here? (This is FPGA, so
anything can be read from HW, i.o.w. it's _untrusted_ source of the data.)

Also, have you tested this with IOMMU enabled? How do they work together (if
there is any collision at all between two?)

...

> +int dfhv1_find_param(void __iomem *base, resource_size_t max, int param);

> +int dfhv1_has_params(void __iomem *base);

I would expect to see some struct instead of base which will provide means of
protection against wrong MMIO accesses.

...

Kernel doc usually accompanies the C-code, i.o.w. implementations and not
declarations.

-- 
With Best Regards,
Andy Shevchenko