On Wed, 19 Apr 2023 13:21:49 -0700
Dave Jiang <dave.jiang@xxxxxxxxx> wrote:
> CXL spec v3.0 9.17.3 CXL Root Device Specific Methods (_DSM)
>
> Add support to retrieve QTG ID via ACPI _DSM call. The _DSM call requires
> an input of an ACPI package with 4 dwords (read latency, write latency,
> read bandwidth, write bandwidth). The call returns a package with 1 WORD
> that provides the max supported QTG ID and a package that may contain 0 or
> more WORDs as the recommended QTG IDs in the recommended order.
>
> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
>
A few minor comments inline.
> +/**
> + * cxl_acpi_evaluate_qtg_dsm - Retrieve QTG ids via ACPI _DSM
> + * @handle: ACPI handle
> + * @input: bandwidth and latency data
> + *
> + * Issue QTG _DSM with accompanied bandwidth and latency data in order to get
> + * the QTG IDs that falls within the performance data.
Falls within is a little vague. Perhaps something like
the QTG IDs that are suitable for the performance point in order of most suitable
to least suitable.
> + */
> +struct qtg_dsm_output *cxl_acpi_evaluate_qtg_dsm(acpi_handle handle,
> + struct qtg_dsm_input *input)
> +{
> + union acpi_object *out_obj, *out_buf, *pkg;
> + union acpi_object in_buf = {
> + .buffer = {
> + .type = ACPI_TYPE_BUFFER,
> + .pointer = (u8 *)input,
> + .length = sizeof(u32) * 4,
sizeof(*input)?
Also, ACPI structures are always little endian. Do we need to be careful of that
here?
> + },
> + };
> + union acpi_object in_obj = {
> + .package = {
> + .type = ACPI_TYPE_PACKAGE,
> + .count = 1,
> + .elements = &in_buf
> + },
> + };
> + struct qtg_dsm_output *output = NULL;
> + int len, rc, i;
> + u16 *max_qtg;
> +
> + out_obj = acpi_evaluate_dsm(handle, &acpi_cxl_qtg_id_guid, 1, 1, &in_obj);
> + if (!out_obj)
> + return ERR_PTR(-ENXIO);
> +
> + if (out_obj->type != ACPI_TYPE_PACKAGE) {
> + rc = -ENXIO;
> + goto err;
> + }
> +
> + /* Check Max QTG ID */
> + pkg = &out_obj->package.elements[0];
> + if (pkg->type != ACPI_TYPE_BUFFER) {
> + rc = -ENXIO;
> + goto err;
> + }
> +
> + if (pkg->buffer.length != sizeof(u16)) {
> + rc = -ENXIO;
> + goto err;
> + }
> + max_qtg = (u16 *)pkg->buffer.pointer;
> +
> + /* Retrieve QTG IDs package */
> + pkg = &out_obj->package.elements[1];
> + if (pkg->type != ACPI_TYPE_PACKAGE) {
> + rc = -ENXIO;
> + goto err;
> + }
> +
> + out_buf = &pkg->package.elements[0];
> + if (out_buf->type != ACPI_TYPE_BUFFER) {
> + rc = -ENXIO;
> + goto err;
> + }
> +
> + len = out_buf->buffer.length;
> +
> + /* It's legal to have 0 QTG entries */
> + if (len == 0)
> + goto out;
> +
> + /* Malformed package, not multiple of WORD size */
> + if (len % sizeof(u16)) {
> + rc = -ENXIO;
> + goto out;
> + }
> +
> + output = kmalloc(len + sizeof(*output), GFP_KERNEL);
> + if (!output) {
> + rc = -ENOMEM;
> + goto err;
> + }
> +
> + output->nr = len / sizeof(u16);
> + memcpy(output->qtg_ids, out_buf->buffer.pointer, len);
> +
> + for (i = 0; i < output->nr; i++) {
> + if (output->qtg_ids[i] > *max_qtg)
> + pr_warn("QTG ID %u greater than MAX %u\n",
> + output->qtg_ids[i], *max_qtg);
> + }
> +
> +out:
> + ACPI_FREE(out_obj);
> + return output;
> +
> +err:
> + ACPI_FREE(out_obj);
> + return ERR_PTR(rc);
Why not combine these with something like
return IS_ERR(rc) ? ERR_PTR(rc) : output;
I'm fine with leaving as it is, if this is common style for these
sorts of ACPI functions.
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_acpi_evaluate_qtg_dsm, CXL);
