2022-03-21 22:41:53

by Eddie James

Subject: [PATCH 2/2] hwmon (occ): Retry for checksum failure

Due to the OCC communication design with a shared SRAM area,
checkum errors are expected due to corrupted buffer from OCC
communications with other system components. Therefore, retry
the command twice in the event of a checksum failure.

Signed-off-by: Eddie James <[email protected]>
---
drivers/hwmon/occ/p9_sbe.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
index 49b13cc01073..7f4c3f979c54 100644
--- a/drivers/hwmon/occ/p9_sbe.c
+++ b/drivers/hwmon/occ/p9_sbe.c
@@ -84,17 +84,25 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
size_t resp_len = sizeof(*resp);
int rc;
-
- rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
- 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);
+ int tries = 0;
+
+ do {
+ rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
+ 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;
+ } else if (rc != -EBADE) {
+ return rc;
+ }
+ /* retry twice for checksum failures */
+ } else {
+ break;
}
-
- return rc;
- }
+ } while (++tries < 3);

switch (resp->return_status) {
case OCC_RESP_CMD_IN_PRG:
--
2.27.0

2022-04-25 04:09:10

by Guenter Roeck

Subject: Re: [PATCH 2/2] hwmon (occ): Retry for checksum failure

On Mon, Mar 21, 2022 at 10:31:12AM -0500, Eddie James wrote:
> Due to the OCC communication design with a shared SRAM area,
> checkum errors are expected due to corrupted buffer from OCC
> communications with other system components. Therefore, retry
> the command twice in the event of a checksum failure.
>
> Signed-off-by: Eddie James <[email protected]>

I assume this will be applied together with patch 1 of the series.

Acked-by: Guenter Roeck <[email protected]>

Guenter

> ---
> drivers/hwmon/occ/p9_sbe.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> index 49b13cc01073..7f4c3f979c54 100644
> --- a/drivers/hwmon/occ/p9_sbe.c
> +++ b/drivers/hwmon/occ/p9_sbe.c
> @@ -84,17 +84,25 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
> struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
> size_t resp_len = sizeof(*resp);
> int rc;
> -
> - rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> - 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);
> + int tries = 0;
> +
> + do {
> + rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> + 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;
> + } else if (rc != -EBADE) {
> + return rc;
> + }
> + /* retry twice for checksum failures */
> + } else {
> + break;
> }
> -
> - return rc;
> - }
> + } while (++tries < 3);
>
> switch (resp->return_status) {
> case OCC_RESP_CMD_IN_PRG:

2022-04-25 19:19:35

by David Laight

Subject: RE: [PATCH 2/2] hwmon (occ): Retry for checksum failure

From: Guenter Roeck
> Sent: 24 April 2022 18:18
>
> On Mon, Mar 21, 2022 at 10:31:12AM -0500, Eddie James wrote:
> > Due to the OCC communication design with a shared SRAM area,
> > checkum errors are expected due to corrupted buffer from OCC
> > communications with other system components. Therefore, retry
> > the command twice in the event of a checksum failure.
> >
> > Signed-off-by: Eddie James <[email protected]>
>
> I assume this will be applied together with patch 1 of the series.
>
> Acked-by: Guenter Roeck <[email protected]>
>
> Guenter
>
> > ---
> > drivers/hwmon/occ/p9_sbe.c | 28 ++++++++++++++++++----------
> > 1 file changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
> > index 49b13cc01073..7f4c3f979c54 100644
> > --- a/drivers/hwmon/occ/p9_sbe.c
> > +++ b/drivers/hwmon/occ/p9_sbe.c
> > @@ -84,17 +84,25 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
> > struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
> > size_t resp_len = sizeof(*resp);
> > int rc;
> > -
> > - rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> > - 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);
> > + int tries = 0;
> > +
> > + do {

Why not use a for() loop?

> > + rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
> > + 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;
> > + } else if (rc != -EBADE) {
> > + return rc;
> > + }

No need for else after return.

> > + /* retry twice for checksum failures */
> > + } else {
> > + break;

I'd break on the success path after testing (rc >= 0).
Saves a level of indent.

> > }
> > -
> > - return rc;
> > - }
> > + } while (++tries < 3);
> >
> > switch (resp->return_status) {
> > case OCC_RESP_CMD_IN_PRG:

Probably end up with something like:
for (tries = 0; tries < 3; tries++) {
rc = ...;
if (rc >= 0)
break;
if (resp_len) {
...
return rc;
}
if (rc != -EBADE)
return rc;
}

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

2022-04-27 10:58:53

by Eddie James

Subject: Re: [PATCH 2/2] hwmon (occ): Retry for checksum failure

On 4/25/22 04:10, David Laight wrote:
> From: Guenter Roeck
>> Sent: 24 April 2022 18:18
>>
>> On Mon, Mar 21, 2022 at 10:31:12AM -0500, Eddie James wrote:
>>> Due to the OCC communication design with a shared SRAM area,
>>> checkum errors are expected due to corrupted buffer from OCC
>>> communications with other system components. Therefore, retry
>>> the command twice in the event of a checksum failure.
>>>
>>> Signed-off-by: Eddie James <[email protected]>
>> I assume this will be applied together with patch 1 of the series.
>>
>> Acked-by: Guenter Roeck <[email protected]>
>>
>> Guenter
>>
>>> ---
>>> drivers/hwmon/occ/p9_sbe.c | 28 ++++++++++++++++++----------
>>> 1 file changed, 18 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c
>>> index 49b13cc01073..7f4c3f979c54 100644
>>> --- a/drivers/hwmon/occ/p9_sbe.c
>>> +++ b/drivers/hwmon/occ/p9_sbe.c
>>> @@ -84,17 +84,25 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd, size_t len)
>>> struct p9_sbe_occ *ctx = to_p9_sbe_occ(occ);
>>> size_t resp_len = sizeof(*resp);
>>> int rc;
>>> -
>>> - rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
>>> - 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);
>>> + int tries = 0;
>>> +
>>> + do {
> Why not use a for() loop?
>
>>> + rc = fsi_occ_submit(ctx->sbe, cmd, len, resp, &resp_len);
>>> + 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;
>>> + } else if (rc != -EBADE) {
>>> + return rc;
>>> + }
> No need for else after return.
>
>>> + /* retry twice for checksum failures */
>>> + } else {
>>> + break;
> I'd break on the success path after testing (rc >= 0).
> Saves a level of indent.
>
>>> }
>>> -
>>> - return rc;
>>> - }
>>> + } while (++tries < 3);
>>>
>>> switch (resp->return_status) {
>>> case OCC_RESP_CMD_IN_PRG:
> Probably end up with something like:
> for (tries = 0; tries < 3; tries++) {
> rc = ...;
> if (rc >= 0)
> break;
> if (resp_len) {
> ...
> return rc;
> }
> if (rc != -EBADE)
> return rc;
> }
>
> David


Thanks David, Ack.


>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>