| Message ID | 20210914213543.73351-4-eajames@linux.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/3] fsi: occ: Use a large buffer for responses | expand |
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 ? 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.name); > + } > + > 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 ? > > 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(). > + 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 >
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 > >
Hi Eddie, > 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. Super! Some comments inline: > +enum sbe_error_state { > + SBE_ERROR_NONE = 0, > + SBE_ERROR_PENDING, > + SBE_ERROR_COLLECTED > +}; > + > struct p9_sbe_occ { > struct occ occ; > + int sbe_error; Use the enum here? > + 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; > +} So any read from this file will clear out the FFDC data, making partial reads impossible. As a least-intrusive change, could we set SBE_ERROR_COLLECTED on write instead? Or is there a better interface (a pipe?) that allows multiple FFDC captures, destroyed on full consume, without odd read/write side effects? > 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; [...] > + ctx->ffdc_len = resp_len; > + memcpy(ctx->ffdc, resp, resp_len); This will clear out the previous error it if hasn't been collected by userspace. Is that really what you want for *first* fail data capture? :) Cheers, Jeremy
On Wed, 15 Sept 2021 at 21:11, Eddie James <eajames@linux.ibm.com> wrote: > > 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. Thanks for the reminder. We have a queue of FSI patches to send out. I hope to get that done this coming merge window. Cheers, Joel
On Thu, 2021-09-16 at 08:17 +0800, Jeremy Kerr wrote: > Hi Eddie, > > > 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. > > Super! Some comments inline: > > > +enum sbe_error_state { > > + SBE_ERROR_NONE = 0, > > + SBE_ERROR_PENDING, > > + SBE_ERROR_COLLECTED > > +}; > > + > > struct p9_sbe_occ { > > struct occ occ; > > + int sbe_error; > > Use the enum here? Yep :) Though I think I will switch to a bool; as I wrote to Guenter, the extra "collected" state isn't necessary if I stop trying to report two things (the FFDC itself and whether or not there is an error at all) with one interface. > > > + 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; > > +} > > So any read from this file will clear out the FFDC data, making > partial > reads impossible. As a least-intrusive change, could we set > SBE_ERROR_COLLECTED on write instead? Good point. Write would work. How about resetting the error flag once pos >= size though, for the sake of simplicity? > > Or is there a better interface (a pipe?) that allows multiple FFDC > captures, destroyed on full consume, without odd read/write side > effects? I tried to look into pipes, but I'm pretty unclear on how to set one up. Doesn't appear as though they are used in kernel much, especially as an interface to userspace. I'm just not sure its worth having more than one FFDC packet available. There would always have to be a maximum number of them anyway to put a bound on memory usage. We're somewhat helped here by the fact that OCC operations will go out a maximum of once a second, so that's hopefully plenty of time for userspace to notice the error and pick up the FFDC. > > > 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; > > [...] > > > + ctx->ffdc_len = resp_len; > > + memcpy(ctx->ffdc, resp, resp_len); > > This will clear out the previous error it if hasn't been collected by > userspace. Is that really what you want for *first* fail data > capture? > :) Ah, good point. I will fix that! Thanks for the review Jeremy! Eddie > > Cheers, > > > Jeremy >
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> > --- > drivers/hwmon/occ/p9_sbe.c | 98 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 97 insertions(+), 1 deletion(-) You forgot a Documentation/ABI/ entry :( Binary sysfs files are for "pass through to the hardware" only, you should not be dumping kernel data to userspace through them. I can't really determine if this is the case here or not, as there's no documentation saying what you are trying to represent here... thanks, greg k-h
On 9/21/21 10:37 AM, Greg KH 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> >> --- >> drivers/hwmon/occ/p9_sbe.c | 98 +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 97 insertions(+), 1 deletion(-) > You forgot a Documentation/ABI/ entry :( > > Binary sysfs files are for "pass through to the hardware" only, you > should not be dumping kernel data to userspace through them. I can't > really determine if this is the case here or not, as there's no > documentation saying what you are trying to represent here... Ok oops. I will add an entry, thanks. I believe this qualifies for binary sysfs then, since the data is an error response from the hardware directly. Thanks, Eddie > > thanks, > > greg k-h
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.name); + } + 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); 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; + 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; }
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> --- drivers/hwmon/occ/p9_sbe.c | 98 +++++++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 1 deletion(-)