From: Pratik Sampat <psampat@linux.ibm.com>
To: Fabiano Rosas <farosas@linux.ibm.com>,
mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org,
linuxppc-dev@lists.ozlabs.org, kvm-ppc@vger.kernel.org,
linux-kernel@vger.kernel.org, pratik.r.sampat@gmail.com
Subject: Re: [PATCH v4 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
Date: Sat, 17 Jul 2021 06:53:01 +0000 [thread overview]
Message-ID: <fb840e20-0b8d-b80a-d7cd-60b0b890e126@linux.ibm.com> (raw)
In-Reply-To: <87im1a2i5k.fsf@linux.ibm.com>
On 17/07/21 12:35 am, Fabiano Rosas wrote:
> "Pratik R. Sampat" <psampat@linux.ibm.com> writes:
>
>> +#include <linux/module.h>
>> +#include <linux/types.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/hugetlb.h>
>> +#include <asm/lppaca.h>
>> +#include <asm/hvcall.h>
>> +#include <asm/firmware.h>
>> +#include <asm/time.h>
>> +#include <asm/prom.h>
>> +#include <asm/vdso_datapage.h>
>> +#include <asm/vio.h>
>> +#include <asm/mmu.h>
>> +#include <asm/machdep.h>
>> +#include <asm/drmem.h>
> Do you need all of these headers? Sorry to mention just now, I seem to have
> dropped this comment from a previous review.
Ah yes, that was a TODO on my part I had missed as cleanups.
I'll verify if I do need all of them and get back to you.
>> +
>> +#include "pseries.h"
>> +
>> +/*
>> + * Flag attributes to fetch either all or one attribute from the HCALL
>> + * flag = BE(0) => fetch all attributes with firstAttributeId = 0
>> + * flag = BE(1) => fetch a single attribute with firstAttributeId = id
>> + */
>> +#define ESI_FLAGS_ALL 0
>> +#define ESI_FLAGS_SINGLE PPC_BIT(0)
>> +
>> +#define MAX_ATTRS 3
>> +
>> +struct papr_attr {
>> + u64 id;
>> + struct kobj_attribute kobj_attr;
>> +};
>> +struct papr_group {
>> + struct attribute_group pg;
>> + struct papr_attr pgattrs[MAX_ATTRS];
>> +} *pgs;
>> +
>> +/* /sys/firmware/papr */
>> +struct kobject *papr_kobj;
>> +/* /sys/firmware/papr/energy_scale_info */
>> +struct kobject *esi_kobj;
>> +
>> +/*
>> + * Extract and export the description of the energy scale attribute
>> + *
> Extra line here.
ack.
>> + */
>> +static ssize_t papr_show_desc(struct kobject *kobj,
>> + struct kobj_attribute *kobj_attr,
>> + char *buf)
>> +{
>> + struct papr_attr *pattr = container_of(kobj_attr, struct papr_attr,
>> + kobj_attr);
>> + struct h_energy_scale_info_hdr *t_hdr;
>> + struct energy_scale_attribute *t_esi;
>> + char *t_buf;
>> + int ret = 0;
>> +
>> + t_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
>> + if (t_buf = NULL)
>> + return -ENOMEM;
>> +
>> + ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_SINGLE,
>> + pattr->id, virt_to_phys(t_buf),
>> + MAX_BUF_SZ);
>> +
>> + if (ret != H_SUCCESS) {
>> + pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
>> + goto out;
>> + }
>> +
>> + t_hdr = (struct h_energy_scale_info_hdr *) t_buf;
>> + t_esi = (struct energy_scale_attribute *)
>> + (t_buf + be64_to_cpu(t_hdr->array_offset));
>> +
>> + ret = snprintf(buf, sizeof(t_esi->desc), "%s\n", t_esi->desc);
>> + if (ret < 0)
>> + ret = -EIO;
>> +out:
>> + kfree(t_buf);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Extract and export the numeric value of the energy scale attributes
>> + */
>> +static ssize_t papr_show_value(struct kobject *kobj,
>> + struct kobj_attribute *kobj_attr,
>> + char *buf)
>> +{
>> + struct papr_attr *pattr = container_of(kobj_attr, struct papr_attr,
>> + kobj_attr);
>> + struct h_energy_scale_info_hdr *t_hdr;
>> + struct energy_scale_attribute *t_esi;
>> + char *t_buf;
>> + int ret = 0;
>> +
>> + t_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
>> + if (t_buf = NULL)
>> + return -ENOMEM;
>> +
>> + ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_SINGLE,
>> + pattr->id, virt_to_phys(t_buf),
>> + MAX_BUF_SZ);
>> +
>> + if (ret != H_SUCCESS) {
>> + pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
>> + goto out;
>> + }
>> +
>> + t_hdr = (struct h_energy_scale_info_hdr *) t_buf;
>> + t_esi = (struct energy_scale_attribute *)
>> + (t_buf + be64_to_cpu(t_hdr->array_offset));
>> +
>> + ret = snprintf(buf, sizeof(t_esi->value), "%llu\n",
>> + be64_to_cpu(t_esi->value));
>> + if (ret < 0)
>> + ret = -EIO;
>> +out:
>> + kfree(t_buf);
>> +
>> + return ret;
>> +}
>> +
>> +/*
>> + * Extract and export the value description in string format of the energy
>> + * scale attributes
>> + */
>> +static ssize_t papr_show_value_desc(struct kobject *kobj,
>> + struct kobj_attribute *kobj_attr,
>> + char *buf)
>> +{
>> + struct papr_attr *pattr = container_of(kobj_attr, struct papr_attr,
>> + kobj_attr);
>> + struct h_energy_scale_info_hdr *t_hdr;
>> + struct energy_scale_attribute *t_esi;
>> + char *t_buf;
>> + int ret = 0;
>> +
>> + t_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
>> + if (t_buf = NULL)
>> + return -ENOMEM;
>> +
>> + ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_SINGLE,
>> + pattr->id, virt_to_phys(t_buf),
>> + MAX_BUF_SZ);
>> +
>> + if (ret != H_SUCCESS) {
>> + pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
>> + goto out;
>> + }
>> +
>> + t_hdr = (struct h_energy_scale_info_hdr *) t_buf;
>> + t_esi = (struct energy_scale_attribute *)
>> + (t_buf + be64_to_cpu(t_hdr->array_offset));
>> +
>> + ret = snprintf(buf, sizeof(t_esi->value_desc), "%s\n",
>> + t_esi->value_desc);
>> + if (ret < 0)
>> + ret = -EIO;
>> +out:
>> + kfree(t_buf);
>> +
>> + return ret;
>> +}
>> +
>> +static struct papr_ops_info {
>> + const char *attr_name;
>> + ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *kobj_attr,
>> + char *buf);
>> +} ops_info[MAX_ATTRS] = {
>> + { "desc", papr_show_desc },
>> + { "value", papr_show_value },
>> + { "value_desc", papr_show_value_desc },
>> +};
>> +
>> +static void add_attr(u64 id, int index, struct papr_attr *attr)
>> +{
>> + attr->id = id;
>> + sysfs_attr_init(&attr->kobj_attr.attr);
>> + attr->kobj_attr.attr.name = ops_info[index].attr_name;
>> + attr->kobj_attr.attr.mode = 0444;
>> + attr->kobj_attr.show = ops_info[index].show;
>> +}
>> +
>> +static int add_attr_group(u64 id, int len, struct papr_group *pg,
>> + bool show_val_desc)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < len; i++) {
> Could use MAX_ATTRS directly.
Sure, while writing this initially I wasn't sure how many attrs we'd have so it
passed as param, now I see that's not needed so I can remove that and use
MAX_ATTRS itself
>> + if (!strcmp(ops_info[i].attr_name, "value_desc") &&
>> + !show_val_desc) {
>> + continue;
>> + }
>> + add_attr(id, i, &pg->pgattrs[i]);
>> + pg->pg.attrs[i] = &pg->pgattrs[i].kobj_attr.attr;
>> + }
>> +
>> + return sysfs_create_group(esi_kobj, &pg->pg);
>> +}
>> +
>> +static int __init papr_init(void)
>> +{
>> + struct h_energy_scale_info_hdr *esi_hdr;
>> + struct energy_scale_attribute *esi_attrs;
>> + uint64_t num_attrs;
>> + int ret, idx, i;
>> + char *esi_buf;
>> +
>> + if (!firmware_has_feature(FW_FEATURE_LPAR))
>> + return -ENXIO;
>> +
>> + esi_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
>> + if (esi_buf = NULL)
>> + return -ENOMEM;
>> + /*
>> + * hcall(
>> + * uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info
>> + * uint64 flags, // Per the flag request
>> + * uint64 firstAttributeId, // The attribute id
>> + * uint64 bufferAddress, // Guest physical address of the output buffer
>> + * uint64 bufferSize); // The size in bytes of the output buffer
>> + */
>> + ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_ALL, 0,
>> + virt_to_phys(esi_buf), MAX_BUF_SZ);
>> + if (ret != H_SUCCESS) {
>> + pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
>> + goto out;
>> + }
>> +
>> + esi_hdr = (struct h_energy_scale_info_hdr *) esi_buf;
>> + if (esi_hdr->data_header_version != ESI_VERSION) {
>> + pr_warn("H_GET_ENERGY_SCALE_INFO VER MISMATCH - EXP: 0x%x, REC: 0x%x",
>> + ESI_VERSION, esi_hdr->data_header_version);
>> + }
>> +
>> + num_attrs = be64_to_cpu(esi_hdr->num_attrs);
>> + esi_attrs = (struct energy_scale_attribute *)
>> + (esi_buf + be64_to_cpu(esi_hdr->array_offset));
>> +
>> + pgs = kcalloc(num_attrs, sizeof(*pgs), GFP_KERNEL);
>> + if (!pgs)
>> + goto out;
>> +
>> + papr_kobj = kobject_create_and_add("papr", firmware_kobj);
>> + if (!papr_kobj) {
>> + pr_warn("kobject_create_and_add papr failed\n");
>> + goto out_pgs;
>> + }
>> +
>> + esi_kobj = kobject_create_and_add("energy_scale_info", papr_kobj);
>> + if (!esi_kobj) {
>> + pr_warn("kobject_create_and_add energy_scale_info failed\n");
>> + goto out_kobj;
>> + }
>> +
>> + for (idx = 0; idx < num_attrs; idx++) {
>> + bool show_val_desc = true;
>> +
>> + pgs[idx].pg.attrs = kcalloc(MAX_ATTRS + 1,
>> + sizeof(*pgs[idx].pg.attrs),
>> + GFP_KERNEL);
>> + if (!pgs[idx].pg.attrs)
>> + goto out_ekobj;
> What about the attrs allocated during the previous iterations?
Yes, you're right that needs cleanups.
I could write the following to clean up old entries like I did when space for
name couldn't be allocated:
for (i = idx - 1; i >= 0; i++)
kfree(pgs[i].pg.attrs);
Thanks for pointing that out.
>> +
>> + pgs[idx].pg.name = kasprintf(GFP_KERNEL, "%lld",
>> + be64_to_cpu(esi_attrs[idx].id));
>> + if (pgs[idx].pg.name = NULL) {
>> + for (i = idx; i >= 0; i--)
>> + kfree(pgs[i].pg.attrs);
>> + goto out_ekobj;
>> + }
>> + /* Do not add the value description if it does not exist */
>> + if (strlen(esi_attrs[idx].value_desc) = 0)
> strnlen
ack
>
>> + show_val_desc = false;
>> +
>> + if (add_attr_group(be64_to_cpu(esi_attrs[idx].id),
>> + MAX_ATTRS, &pgs[idx], show_val_desc)) {
>> + pr_warn("Failed to create papr attribute group %s\n",
>> + pgs[idx].pg.name);
>> + goto out_pgattrs;
>> + }
>> + }
>> +
>> + kfree(esi_buf);
>> + return 0;
>> +
>> +out_pgattrs:
>> + for (i = 0; i < MAX_ATTRS ; i++) {
> pgs is num_attrs long
Ah yes, you're right. It should loop over num_attrs
>
>> + kfree(pgs[i].pg.attrs);
>> + kfree(pgs[i].pg.name);
>> + }
>> +out_ekobj:
>> + kobject_put(esi_kobj);
>> +out_kobj:
>> + kobject_put(papr_kobj);
>> +out_pgs:
>> + kfree(pgs);
>> +out:
>> + kfree(esi_buf);
>> +
>> + return -ENOMEM;
>> +}
>> +
>> +machine_device_initcall(pseries, papr_init);
prev parent reply other threads:[~2021-07-17 6:53 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-16 15:33 [PATCH v4 0/1] Interface to represent PAPR firmware attributes Pratik R. Sampat
2021-07-16 15:33 ` [PATCH v4 1/1] powerpc/pseries: " Pratik R. Sampat
2021-07-16 19:05 ` Fabiano Rosas
2021-07-17 6:53 ` Pratik Sampat [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fb840e20-0b8d-b80a-d7cd-60b0b890e126@linux.ibm.com \
--to=psampat@linux.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=farosas@linux.ibm.com \
--cc=kvm-ppc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
--cc=pratik.r.sampat@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).