Thanks for the review Ben -
On Wed, Jun 16, 2021 at 09:17:40AM -0700, Ben Widawsky wrote:
> On 21-06-15 17:20:39, Alison Schofield wrote:
snip
> > +static unsigned long cfmws_to_decoder_flags(int restrictions)
> > +{
> > + unsigned long flags = 0;
> > +
> > + if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE2)
> > + flags |= CXL_DECODER_F_TYPE2;
> > + if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_TYPE3)
> > + flags |= CXL_DECODER_F_TYPE3;
> > + if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_VOLATILE)
> > + flags |= CXL_DECODER_F_RAM;
> > + if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_PMEM)
> > + flags |= CXL_DECODER_F_PMEM;
> > + if (restrictions & ACPI_CEDT_CFMWS_RESTRICT_FIXED)
> > + flags |= CXL_DECODER_F_LOCK;
> > +
> > + return flags;
> > +}
>
> I know these flags aren't introduced by this patch, but I'm wondering if it
> makes sense to not just use the spec definitions rather than defining our own.
> It doesn't do much harm, but it's extra typing everytime the spec adds new flags
> and I don't really see the upside.
>
I think Dan's email in this thread covered this.
snip
> > +
> > +static int cxl_acpi_cfmws_verify(struct device *dev,
> > + struct acpi_cedt_cfmws *cfmws)
> > +{
snip
> > +
> > +
> > + expected_len = struct_size((cfmws), interleave_targets,
> > + CFMWS_INTERLEAVE_WAYS(cfmws));
> > +
> > + if (expected_len != cfmws->header.length) {
>
> I'd switch this to:
> if (expected_len < cfmws->header.length)
>
> If it's too big, just print a dev_dbg.
>
Got it.
snip
> > + void *cedt_base;
> > + int rc;
> > +
> > + len = cedt_table->length - sizeof(*cedt_table);
> > + cedt_base = cedt_table + 1;
>
> naming suggestions per previous patch... up to you though.
>
Ditto w previous patch.
snip
> > +
> > + }
> > +
> > + cxld = devm_cxl_add_decoder(dev, root_port,
> > + CFMWS_INTERLEAVE_WAYS(cfmws),
> > + cfmws->base_hpa, cfmws->window_size,
> > + CFMWS_INTERLEAVE_WAYS(cfmws),
>
> Interesting... this made me question, how can we have a different number of
> targets and ways?
>
Dan explained this previously:
"nr_targets is the number of possible targets that this decoder can
target. For CFMWS it just equals interleave_ways because the target
list can't be changed. A switch on the other hand could support up to
16 possible targets, but be dynamically configured to only do a 1-way
interleave. So this is an artifact of 'struct cxl_decoder'
representing both fixed CFMWS entries and dynamically programmable
switch entries. nr_targets tells devm_cxl_add_decoder() how much
memory to allocate for its target list, interleave_ways tells
devm_cxl_add_decoder() what the decoder is currently programmed to
decode."
>
snip
>
