| Message ID | 20220321153112.12199-3-eajames@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fsi and hwmon (occ): Prevent occasional checksum failures | expand |
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 <eajames@linux.ibm.com> I assume this will be applied together with patch 1 of the series. Acked-by: Guenter Roeck <linux@roeck-us.net> 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:
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 <eajames@linux.ibm.com> > > I assume this will be applied together with patch 1 of the series. > > Acked-by: Guenter Roeck <linux@roeck-us.net> > > 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)
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 <eajames@linux.ibm.com> >> I assume this will be applied together with patch 1 of the series. >> >> Acked-by: Guenter Roeck <linux@roeck-us.net> >> >> 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) >
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:
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 <eajames@linux.ibm.com> --- drivers/hwmon/occ/p9_sbe.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)