[PATCH occ hwmon v3] Add OCC hwmon driver to Kernel

Norman James njames at us.ibm.com
Tue Jan 5 23:59:54 AEDT 2016


The OCC register definitions are set by the hardware and can be found in
the P8 SCOM register documentation.   They won't ever change for P8 and P8
+.


> > +
> > +/* To generate attn to OCC */
> > +#define ATTN_DATA      0x0006B035
> > +/* For BMC to read/write SRAM */
> > +#define OCB_ADDRESS            0x0006B070
> > +#define OCB_DATA               0x0006B075
> > +#define OCB_STATUS_CONTROL_AND 0x0006B072
> > +#define OCB_STATUS_CONTROL_OR  0x0006B073
> > +#define OCC_COMMAND_ADDR       0xFFFF6000
> > +#define OCC_RESPONSE_ADDR      0xFFFF7000

Regards,
Norman James
IBM - POWER Systems Architect
Phone: 1-512-286-6807 (T/L: 363-6807)
Internet: njames at us.ibm.com




From:	"Yi TZ Li" <shliyi at cn.ibm.com>
To:	Joel Stanley <joel at jms.id.au>
Cc:	OpenBMC Maillist <openbmc at lists.ozlabs.org>
Date:	01/05/2016 02:30 AM
Subject:	Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
Sent by:	"openbmc" <openbmc-bounces+njames=us.ibm.com at lists.ozlabs.org>



Hi Joel,

Thanks many for your time to review the driver.

I re-submitted to patch, fixed most of the issues you pointed out, except a
few showed bellow.
Please see my reply bellow for details:

joel.stan at gmail.com wrote on 12/10/2015 12:02:19 PM:

> From: Joel Stanley <joel at jms.id.au>
> To: Yi Li <adamliyi at msn.com>
> Cc: OpenBMC Maillist <openbmc at lists.ozlabs.org>, Jeremy Kerr
> <jk at ozlabs.org>, Yi TZ Li/China/IBM at IBMCN
> Date: 12/10/2015 12:03 PM
> Subject: Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
> Sent by: joel.stan at gmail.com
>
> Hello Adam,
>
> On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi at msn.com> wrote:
> > The default patch triggered by git pull request is hard to read.
> > I resend a clean patch to the list. Please review.
>
> Thanks! This is the preferred method for reviewing code. Even better
> would be if you used git format-patch to create the email, that way we
> can review your commit message as well.
>
> Before you submit next, please run your patch through
> scripts/checkpatch.pl in the kernel tree and fix all of the errors and
> warnings. It will warn you about common style and syntax mistakes.
>
> ./scripts/checkpatch.pl -f drivers/hwmon/occ_i2c.c
>
> [...]
>
> total: 24 errors, 21 warnings, 1315 lines checked

I run the checkpatch.pl against the new patch, here are the result:

scripts/checkpatch.pl occ_v4.patch
WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#11: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:30:
+                                               compatible = "ibm,occ-i2c";

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#15: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:34:
+                                               compatible = "ibm,occ-i2c";

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#33: FILE: arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts:35:
+                                               compatible = "ibm,occ-i2c";

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#74:
new file mode 100644

WARNING: line over 80 characters
#127: FILE: drivers/hwmon/occ_i2c.c:49:
+ *
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf


WARNING: line over 80 characters
#420: FILE: drivers/hwmon/occ_i2c.c:342:
+ *
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf


WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#1389: FILE: drivers/hwmon/occ_i2c.c:1311:
+       {.compatible = "ibm,occ-i2c"},

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 7 warnings, 1391 lines checked

occ_v4.patch has style problems, please review.

1) For  "DT compatible" warnings, I am not sure whether it is necessary,
since I run "scripts/checkpatch.pl -f
arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts",
it will report many warnings. I can add a description if necessary.

2) for "line over 80" warnings, it would break the link if wrap at 80
characters.

> > +
> > +/* To generate attn to OCC */
> > +#define ATTN_DATA      0x0006B035
> > +/* For BMC to read/write SRAM */
> > +#define OCB_ADDRESS            0x0006B070
> > +#define OCB_DATA               0x0006B075
> > +#define OCB_STATUS_CONTROL_AND 0x0006B072
> > +#define OCB_STATUS_CONTROL_OR  0x0006B073
> > +#define OCC_COMMAND_ADDR       0xFFFF6000
> > +#define OCC_RESPONSE_ADDR      0xFFFF7000
>
> Are these addresses going to change with different versions of the
> OCC firmware?
>

Actually I did not find the document for most of the addresses. I copied
them from Alvin's OCC code.
If anyone know the source (possibly from OCC firmware developers), please
help.
The only OCC document I can find is:
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf

It defines OCC_COMMAND_ADDR and OCC_RESPONSE_ADDR only.

>
> > +
> > +       if (!p)
> > +               return;
>
> It's common in kernel code to omit checking for null arguments.
>
> If you think there is a likely hood of this happening, then perhaps
> WARN_ON(!b) would be better; this way we get a backtrace in the kernel
> log.
>

The deinit_occ_resp_buf() function is supposed to be called in multiple
situation.
E.g, error handling code in a function. In those case, pointer might be
NULL.
So WARN_ON() is not expected in that case.


> > +
> > +       if (!p->blocks)
> > +               return;
> > +
> > +       for (b = 0; b < p->header.sensor_block_num; b++) {
> > +               if (p->blocks[b].sensor)
> > +                       kfree(p->blocks[b].sensor);
> > +               if (p->blocks[b].power)
> > +                       kfree(p->blocks[b].power);
> > +               if (p->blocks[b].caps)
> > +                       kfree(p->blocks[b].caps);
> > +       }
>
> I think Jeremy suggested in his review that you statically allocate
> these instead of kmalloc and kfreeing at runtime.
>

The OCC sensor type and sensor number will change depending on whether OCC
is master or slave.
Also there is possible of bad cores and the number of sensors may change.
So using dynamically allocated sensor block is nature I think.
It is possible that the driver allocates a block of memory and manage
itself.
But looks there is no great benefit.

> > +{
> > +       int b;
> > +       int s;
> > +       int ret;
> > +       int dnum = SENSOR_BLOCK_OFFSET;
> > +       struct occ_sensor *f_sensor;
> > +       struct occ_sensor *t_sensor;
> > +       struct power_sensor *p_sensor;
> > +       struct caps_sensor *c_sensor;
> > +       uint8_t sensor_block_num;
> > +       uint8_t sensor_type[4];
> > +       uint8_t sensor_format;
> > +       uint8_t sensor_length;
> > +       uint8_t num_of_sensors;
> > +
> > +       /* check if the data is valid */
> > +       if (strncmp(&d[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {
>
> If you have a literal string you don't need to use strncmp.
>

"d[SENSOR_STR_OFFSET]" does not have '\0' to terminate a string. So have to
use strncmp.


>
> > +                               p_sensor->accumulator =
> > +                                       (d[dnum+6] << 24) | (d
> [dnum+7] << 16) |
> > +                                       (d[dnum+8] << 8) | d[dnum+9];
> > +                               p_sensor->value =
> > +                                       (d[dnum+10] << 8) | d[dnum+11];
> > +
> > +                               dev_dbg(&client->dev,
> > +                                       "sensor[%d]-[%d]: id: %u,
> value: %u\n",
> > +                                       b, s, p_sensor->sensor_id,
> > +                                       p_sensor->value);
> > +
> > +                               dnum = dnum + sensor_length;
> > +                       }
> > +               } else if (strncmp(sensor_type, "CAPS", 4) == 0) {
>
> If you have a literal string you don't need to use strncmp.

"sensor_type" has no '\0' to terminate it. strncmp() is necessary.


> > +               val = -1;
> > +       else
> > +               /* in millidegree Celsius */
> > +               val = sensor[n].value * 1000;
> > +
> > +       return sprintf(buf, "%d\n", val);
>
> How do you know "%d\n" will fit into buf?

Do you mean using snprintf()? After reading the man page:
"Because  sprintf()  and vsprintf() assume an arbitrarily long string,
callers must be careful not to overflow the actual space; this
       is often impossible to assure.  Note that the length of the strings
produced is locale-dependent  and  difficult  to  predict.   Use
       snprintf() and vsnprintf() instead (or asprintf(3) and vasprintf
(3))."

snprintf() seems more safe, but we can find a lot of "sprintf()" in sysfs
show* functions in drivers/hwmon/. Looks to me not a big issue.
Anyway, I will change to snprintf() to be safer:
I use the same code as driver/hwmon/lm95241.c

"        return snprintf(buf, PAGE_SIZE - 1,
                        data->config & to_sensor_dev_attr(attr)->index ?
                        "127000\n" : "255000\n");
"

Thanks,
-adam


_______________________________________________
openbmc mailing list
openbmc at lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160105/2c32a53e/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20160105/2c32a53e/attachment-0001.gif>


More information about the openbmc mailing list