On Fri, 21 Oct 2022, Andy Shevchenko wrote:
On Thu, Oct 20, 2022 at 02:26:09PM -0700, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
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.
...
+static void *find_param(void *base, resource_size_t max, int param)
Why base can't be u64 * to begin with?
It can be u64, and I will consider it for the next iteration.
+{
+ int off = 0;
+ u64 v, next;
+
+ while (off < max) {
Maybe you need a comment somewhere to tell that the caller guarantees that max
won't provoke OOB accesses.
+ v = *(u64 *)(base + off);
Okay, if offset is not multiple of at least 4, how do you guarantee no
exception on the architectures with disallowed misaligned accesses?
Making base to be u64 * solves this, but you need to take care to provide
offset in terms of u64 words.
The masking of next below ensures that the offset it at least 4 byte
aligned, but it might make sense to define the next field in terms of 8
byte words.
+ if (param == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+ return base + off + DFHv1_PARAM_DATA;
+
+ next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+ off += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+ if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+ break;
+
+ }
+
+ return NULL;
+}
...
+ /*
+ * DFHv0 only provides mmio resource information for each feature
MMIO
I'll change mmio to MMIO here and a place in the documentation that I
noticed.
+ * in the DFL header. There is no generic interrupt information.
+ * Instead, features with interrupt functionality provide
+ * the information in feature specific registers.
+ */
...
+ if (!finfo->param_size)
break;
This is redundant as it's implied by find_param().
I will remove the redundant code.
+ p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
+ if (!p)
break;
...
+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))
+ return -EINVAL;
+
+ size += next & ~DFHv1_PARAM_HDR_NEXT_MASK;
+
+ if (next & DFHv1_PARAM_HDR_NEXT_EOL)
+ return size;
These 3 looks like they deserve different fields and hence separate FIELD_GET()
will return exactly what we need without additional masking, right?
I agree separate FIELD_GET() calls will be cleaner.
+ }
+
+ return -ENOENT;
+}
...
+ if (dfh_psize > 0) {
Isn't this implied by memcpy_fromio()? I mean if it's 0, nothing bad will
happen if you call the above directly.
+ memcpy_fromio(finfo->params,
+ binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
+ finfo->param_size = dfh_psize;
+ }
...
finfo->mmio_res.flags = IORESOURCE_MEM;
+ if (dfh_ver == 1) {
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+ if (v & DFHv1_CSR_ADDR_REL)
+ finfo->mmio_res.start = v & ~DFHv1_CSR_ADDR_REL;
+ else
+ finfo->mmio_res.start = binfo->start + ofst +
+ FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+
+ v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+ finfo->mmio_res.end = finfo->mmio_res.start +
+ FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+ } else {
+ finfo->mmio_res.start = binfo->start + ofst;
+ finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+ }
You may define
resource_size_t start, end;
locally and simplify above quite a bit.
That is a good suggestion that should clean up the code quite a bit.
...
+void *dfh_find_param(struct dfl_device *dfl_dev, int param);
+ Blank line.
#endif /* __LINUX_DFL_H */
--
With Best Regards,
Andy Shevchenko
