On Fri, 21 Oct 2022, Ilpo Järvinen wrote:

On Thu, 20 Oct 2022, matthew.gerlach@linux.intel.com wrote:

quoted

From: Matthew Gerlach <redacted>

Add generic support for MSI-X interrupts for DFL devices.

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.

Signed-off-by: Matthew Gerlach <redacted>
---
v4: s/MSIX/MSI_X
    move kernel doc to implementation
    use structure assignment
    fix decode of absolute address
    clean up comment in parse_feature_irqs
    remove use of csr_res

v3: remove unneeded blank line
    use clearer variable name
    pass finfo into parse_feature_irqs()
    refactor code for better indentation
    use switch statement for irq parsing
    squash in code parsing register location

v2: fix kernel doc
    clarify use of DFH_VERSION field
---

quoted

+static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
+{
+	int size = 0;
+	u64 v, next;
+
+	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
+		return 0;
+
+	while (size + DFHv1_PARAM_HDR < max) {
+		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!(next & ~DFHv1_PARAM_HDR_NEXT_MASK))

In general, try to not use inverse logic for defining masks. However here,
just change DFHv1_PARAM_HDR_NEXT_OFFSET to not include any extra bits
(no rsvd nor eop) and you no longer need this extra masking.

I agree that defining the fields better and using FIELD_GET would make 
this code cleaner.

quoted

+			return -EINVAL;
+
+		size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;

...Then you can drop this anding too.

quoted

+
+		if (next & DFHv1_PARAM_HDR_NEXT_EOL)

Your docs say EOP, but here you use EOL.

Thanks for catching the inconsistency.

Change DFHv1_PARAM_HDR_NEXT_EOL such that this is extracted directly from
v.

-- 
i.