Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <ac8d30e988ab6cc16d4c7446d259b87deb734910.camel@linux.ibm.com>
Subject: Re: [PATCH 3/3] hwmon: (occ) Provide the SBEFIFO FFDC in binary
 sysfs
From:   Eddie James <eajames@linux.ibm.com>
To:     Guenter Roeck <linux@roeck-us.net>, joel@jms.id.au
Cc:     linux-fsi@lists.ozlabs.org, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org, jdelvare@suse.com,
        alistair@popple.id.au, jk@ozlabs.org
Date:   Wed, 15 Sep 2021 16:11:50 -0500
In-Reply-To: <20210915161333.GA3712393@roeck-us.net>
References: <20210914213543.73351-1-eajames@linux.ibm.com>
         <20210914213543.73351-4-eajames@linux.ibm.com>
         <20210915161333.GA3712393@roeck-us.net>
Organization: IBM
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2021-09-15 at 09:13 -0700, Guenter Roeck wrote:
> On Tue, Sep 14, 2021 at 04:35:43PM -0500, Eddie James wrote:
> > Save any FFDC provided by the OCC driver, and provide it to
> > userspace
> > through a binary sysfs entry. Do some basic state management to
> > ensure that userspace can always collect the data if there was an
> > error. Notify polling userspace when there is an error too.
> > 
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> 
> This is now the 2nd series that we have pending, and the first series
> (from July) still didn't make it into the upstream kernel because the
> fsi code
> seems to go nowhere. Any chance to address that ?

Yes... Joel, can we merge that? I don't have any comments to address.

> 
> Additional comment inline.
> 
> Thanks,
> Guenter
> 
> > ---
> >  drivers/hwmon/occ/p9_sbe.c | 98
> > +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 97 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/occ/p9_sbe.c
> > b/drivers/hwmon/occ/p9_sbe.c
> > index 9709f2b9c052..505f489832a4 100644
> > --- a/drivers/hwmon/occ/p9_sbe.c
> > +++ b/drivers/hwmon/occ/p9_sbe.c
> > @@ -4,18 +4,54 @@
> >  #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"
> >  
> > +enum sbe_error_state {
> > +	SBE_ERROR_NONE = 0,
> > +	SBE_ERROR_PENDING,
> > +	SBE_ERROR_COLLECTED
> > +};
> > +
> >  struct p9_sbe_occ {
> >  	struct occ occ;
> > +	int sbe_error;
> > +	void *ffdc;
> > +	size_t ffdc_len;
> > +	size_t ffdc_size;
> > +	struct mutex sbe_error_lock;	/* lock access to ffdc data
> > */
> > +	u32 no_ffdc_magic;
> >  	struct device *sbe;
> >  };
> >  
> >  #define to_p9_sbe_occ(x)	container_of((x), struct p9_sbe_occ,
> > occ)
> >  
> > +static ssize_t sbe_error_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 == SBE_ERROR_PENDING) {
> > +		rc = memory_read_from_buffer(buf, count, &pos, ctx-
> > >ffdc,
> > +					     ctx->ffdc_len);
> > +		ctx->sbe_error = SBE_ERROR_COLLECTED;
> > +	}
> > +	mutex_unlock(&ctx->sbe_error_lock);
> > +
> > +	return rc;
> > +}
> > +static BIN_ATTR_RO(sbe_error, OCC_MAX_RESP_WORDS * 4);
> > +
> >  static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t
> > len)
> >  {
> >  	struct occ_response *resp = &occ->resp;
> > @@ -24,8 +60,47 @@ 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) {
> > +			bool notify = false;
> > +
> > +			mutex_lock(&ctx->sbe_error_lock);
> > +			if (ctx->sbe_error != SBE_ERROR_PENDING)
> > +				notify = true;
> > +			ctx->sbe_error = SBE_ERROR_PENDING;
> > +
> > +			if (resp_len > ctx->ffdc_size) {
> > +				if (ctx->ffdc_size)
> > +					kvfree(ctx->ffdc);
> > +				ctx->ffdc = kvmalloc(resp_len,
> > GFP_KERNEL);
> > +				if (!ctx->ffdc) {
> > +					ctx->ffdc_size = 0;
> > +					ctx->ffdc_len = sizeof(u32);
> > +					ctx->ffdc = &ctx-
> > >no_ffdc_magic;
> > +					goto unlock;
> > +				}
> > +
> > +				ctx->ffdc_size = resp_len;
> > +			}
> > +
> > +			ctx->ffdc_len = resp_len;
> > +			memcpy(ctx->ffdc, resp, resp_len);
> > +
> > +unlock:
> > +			mutex_unlock(&ctx->sbe_error_lock);
> > +
> > +			if (notify)
> > +				sysfs_notify(&occ->bus_dev->kobj, NULL,
> > +					     bin_attr_sbe_error.attr.na
> > me);
> > +		}
> > +
> >  		return rc;
> > +	}
> > +
> > +	mutex_lock(&ctx->sbe_error_lock);
> > +	if (ctx->sbe_error == SBE_ERROR_COLLECTED)
> > +		ctx->sbe_error = SBE_ERROR_NONE;
> > +	mutex_unlock(&ctx->sbe_error_lock);
> 
> I am not entirely sure I understand the benefit of
> SBE_ERROR_COLLECTED.
> Can you explain why it is needed, and why the status is not just set
> to SBE_ERROR_NONE after the error data was collected ?

The purpose was to make sure the data can be collected even if a
successful transfer (which clears the flag) comes through before the
user comes and reads the file. If the error is just set to NONE, then
the user might never see it, with the current implementation. I think I
will drop the state management though and just return the last error
data.

> 
> >  
> >  	switch (resp->return_status) {
> >  	case OCC_RESP_CMD_IN_PRG:
> > @@ -65,6 +140,13 @@ static int p9_sbe_occ_probe(struct
> > platform_device *pdev)
> >  	if (!ctx)
> >  		return -ENOMEM;
> >  
> > +	ctx->no_ffdc_magic = OCC_NO_FFDC_MAGIC;
> 
> This is ... odd. Why not just return a file size of 0 if there is no
> data ?
> The binary file is an ABI and needs to be documented, including the
> use
> of this "magic". The use of that magic needs to be explained because
> it
> does add a lot of what sems to be unnecessary complexity to the code.
> 
> Besides, most of that complexity seems unnecessary: If the magic is
> really
> needed, the read code could just write it into the buffer if ctx-
> >ffdc
> is NULL. There is a lot of complexity just to avoid an if statement
> in
> sbe_error_read().

Yea, I will admit this is pretty awkward. The reason for all this is
because I was trying to use a single sysfs entry to communicate both
whether or not there is an error at all, and the data from the error.
So returning  file size 0 means "no error" and then we can't capture
the case where there is an error but there is no data.

So on second thought, I should probably use two sysfs entries: one to
indicate if there is an error, and the other to report the data (if
there is any). There is the existing OCC error file of course, but
that's supposed to be for actual OCC response errors, so I will have to
investigate if it can serve both purposes.

Thanks for the review, Guenter!
Eddie

> 
> > +	ctx->sbe_error = SBE_ERROR_NONE;
> > +	ctx->ffdc = &ctx->no_ffdc_magic;
> > +	ctx->ffdc_len = sizeof(u32);
> > +	ctx->ffdc_size = 0;
> > +	mutex_init(&ctx->sbe_error_lock);
> > +
> >  	ctx->sbe = pdev->dev.parent;
> >  	occ = &ctx->occ;
> >  	occ->bus_dev = &pdev->dev;
> > @@ -78,6 +160,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_sbe_error);
> > +		if (rc) {
> > +			dev_warn(occ->bus_dev,
> > +				 "failed to create SBE error ffdc
> > file\n");
> > +			rc = 0;
> > +		}
> > +	}
> > +
> >  	return rc;
> >  }
> >  
> > @@ -86,9 +177,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_sbe_error);
> > +
> >  	ctx->sbe = NULL;
> >  	occ_shutdown(occ);
> >  
> > +	if (ctx->ffdc_size)
> > +		kvfree(ctx->ffdc);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.27.0
> >