Re: [PATCH v3 4/4] hwmon: (occ) Provide the SBEFIFO FFDC in binary sysfs

Guenter Roeck
2021-10-11T14:41:45Z

On Mon, Sep 27, 2021 at 10:59:25AM -0500, Eddie James wrote:
> Save any FFDC provided by the OCC driver, and provide it to userspace
> through a binary sysfs entry. Notify userspace pollers when there is an
> error too.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

For my reference (waiting for infra patches to be accepted/acked):

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
> Changes since v1:
> - Remove "collected" error state in favor of a boolean
> - Clear the error flag once the FFDC has been completely read once
> - Only store FFDC if there is no FFDC waiting to be retrieved
>
> drivers/hwmon/occ/p9_sbe.c | 86 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 9709f2b9c052..e50243580269 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -4,18 +4,79 @@
> #include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/fsi-occ.h>
> +#include <linux/mm.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/platform_device.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
>
> #include "common.h"
>
> struct p9_sbe_occ {
> struct occ occ;
> + bool sbe_error;
> + void *ffdc;
> + size_t ffdc_len;
> + size_t ffdc_size;
> + struct mutex sbe_error_lock; /* lock access to ffdc data */
> struct device *sbe;
> };
>
> #define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ)
>
> +static ssize_t ffdc_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *battr, char *buf, loff_t pos,
> + size_t count)
> +{
> + ssize_t rc = 0;
> + struct occ *occ = dev_get_drvdata(kobj_to_dev(kobj));
> + struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
> +
> + mutex_lock(&ctx->sbe_error_lock);
> + if (ctx->sbe_error) {
> + rc = memory_read_from_buffer(buf, count, &pos, ctx->ffdc,
> + ctx->ffdc_len);
> + if (pos >= ctx->ffdc_len)
> + ctx->sbe_error = false;
> + }
> + mutex_unlock(&ctx->sbe_error_lock);
> +
> + return rc;
> +}
> +static BIN_ATTR_RO(ffdc, OCC_MAX_RESP_WORDS * 4);
> +
> +static bool p9_sbe_occ_save_ffdc(struct p9_sbe_occ *ctx, const void *resp,
> + size_t resp_len)
> +{
> + bool notify = false;
> +
> + mutex_lock(&ctx->sbe_error_lock);
> + if (!ctx->sbe_error) {
> + if (resp_len > ctx->ffdc_size) {
> + if (ctx->ffdc)
> + kvfree(ctx->ffdc);
> + ctx->ffdc = kvmalloc(resp_len, GFP_KERNEL);
> + if (!ctx->ffdc) {
> + ctx->ffdc_len = 0;
> + ctx->ffdc_size = 0;
> + goto done;
> + }
> +
> + ctx->ffdc_size = resp_len;
> + }
> +
> + notify = true;
> + ctx->sbe_error = true;
> + ctx->ffdc_len = resp_len;
> + memcpy(ctx->ffdc, resp, resp_len);
> + }
> +
> +done:
> + mutex_unlock(&ctx->sbe_error_lock);
> + return notify;
> +}
> +
> static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
> {
> struct occ_response *resp = &occ->resp;
> @@ -24,8 +85,15 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
> int rc;
>
> rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> - if (rc < 0)
> + if (rc < 0) {
> + if (resp_len) {
> + if (p9_sbe_occ_save_ffdc(ctx, resp, resp_len))
> + sysfs_notify(&occ->bus_dev->kobj, NULL,
> + bin_attr_ffdc.attr.name);
> + }
> +
> return rc;
> + }
>
> switch (resp->return_status) {
> case OCC_RESP_CMD_IN_PRG:
> @@ -65,6 +133,8 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
> if (!ctx)
> return -ENOMEM;
>
> + mutex_init(&ctx->sbe_error_lock);
> +
> ctx->sbe = pdev->dev.parent;
> occ = &ctx->occ;
> occ->bus_dev = &pdev->dev;
> @@ -78,6 +148,15 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
> if (rc == -ESHUTDOWN)
> rc = -ENODEV; /* Host is shutdown, don't spew errors */
>
> + if (!rc) {
> + rc = device_create_bin_file(occ->bus_dev, &bin_attr_ffdc);
> + if (rc) {
> + dev_warn(occ->bus_dev,
> + "failed to create SBE error ffdc file\n");
> + rc = 0;
> + }
> + }
> +
> return rc;
> }
>
> @@ -86,9 +165,14 @@ static int p9_sbe_occ_remove(struct platform_device *pdev)
> struct occ *occ = platform_get_drvdata(pdev);
> struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>
> + device_remove_bin_file(occ->bus_dev, &bin_attr_ffdc);
> +
> ctx->sbe = NULL;
> occ_shutdown(occ);
>
> + if (ctx->ffdc)
> + kvfree(ctx->ffdc);
> +
> return 0;
> }
>