<html><body><p><tt>Hi Joel,</tt><br><br><tt>Thanks many for your time to review the driver.</tt><br><br><tt>I re-submitted to patch, fixed most of the issues you pointed out, except a few showed bellow.</tt><br><tt>Please see my reply bellow for details:</tt><br><br><tt>joel.stan@gmail.com wrote on 12/10/2015 12:02:19 PM:<br><br>> From: Joel Stanley <joel@jms.id.au></tt><br><tt>> To: Yi Li <adamliyi@msn.com></tt><br><tt>> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>, Jeremy Kerr <br>> <jk@ozlabs.org>, Yi TZ Li/China/IBM@IBMCN</tt><br><tt>> Date: 12/10/2015 12:03 PM</tt><br><tt>> Subject: Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel</tt><br><tt>> Sent by: joel.stan@gmail.com</tt><br><tt>> <br>> Hello Adam,<br>> <br>> On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi@msn.com> wrote:<br>> > The default patch triggered by git pull request is hard to read.<br>> > I resend a clean patch to the list. Please review.<br>> <br>> Thanks! This is the preferred method for reviewing code. Even better<br>> would be if you used git format-patch to create the email, that way we<br>> can review your commit message as well.<br>> <br>> Before you submit next, please run your patch through<br>> scripts/checkpatch.pl in the kernel tree and fix all of the errors and<br>> warnings. It will warn you about common style and syntax mistakes.<br>> <br>> ./scripts/checkpatch.pl -f drivers/hwmon/occ_i2c.c<br>> <br>> [...]<br>> <br>> total: 24 errors, 21 warnings, 1315 lines checked<br></tt><br><tt>I run the checkpatch.pl against the new patch, here are the result:</tt><br><br><tt>scripts/checkpatch.pl occ_v4.patch</tt><br><tt>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/</tt><br><tt>#11: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:30:</tt><br><tt>+ compatible = "ibm,occ-i2c";</tt><br><br><tt>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/</tt><br><tt>#15: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:34:</tt><br><tt>+ compatible = "ibm,occ-i2c";</tt><br><br><tt>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/</tt><br><tt>#33: FILE: arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts:35:</tt><br><tt>+ compatible = "ibm,occ-i2c";</tt><br><br><tt>WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?</tt><br><tt>#74:</tt><br><tt>new file mode 100644</tt><br><br><tt>WARNING: line over 80 characters</tt><br><tt>#127: FILE: drivers/hwmon/occ_i2c.c:49:</tt><br><tt>+ * </tt><a href="https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf"><tt>https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf</tt></a><br><br><tt>WARNING: line over 80 characters</tt><br><tt>#420: FILE: drivers/hwmon/occ_i2c.c:342:</tt><br><tt>+ * </tt><a href="https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf"><tt>https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf</tt></a><br><br><tt>WARNING: DT compatible string "ibm,occ-i2c" appears un-documented -- check ./Documentation/devicetree/bindings/</tt><br><tt>#1389: FILE: drivers/hwmon/occ_i2c.c:1311:</tt><br><tt>+ {.compatible = "ibm,occ-i2c"},</tt><br><br><tt>ERROR: Missing Signed-off-by: line(s)</tt><br><br><tt>total: 1 errors, 7 warnings, 1391 lines checked</tt><br><br><tt>occ_v4.patch has style problems, please review. <br></tt><br><tt>1) For "DT compatible" warnings, I am not sure whether it is necessary, since I run "</tt><tt>scripts/checkpatch.pl -f arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts</tt><tt>",</tt><br><tt>it will report many warnings. I can add a description if necessary.</tt><br><br><tt>2) for "line over 80" warnings, it would break the link if wrap at 80 characters.</tt><br><tt> <br>> > +<br>> > +/* To generate attn to OCC */<br>> > +#define ATTN_DATA 0x0006B035<br>> > +/* For BMC to read/write SRAM */<br>> > +#define OCB_ADDRESS 0x0006B070<br>> > +#define OCB_DATA 0x0006B075<br>> > +#define OCB_STATUS_CONTROL_AND 0x0006B072<br>> > +#define OCB_STATUS_CONTROL_OR 0x0006B073<br>> > +#define OCC_COMMAND_ADDR 0xFFFF6000<br>> > +#define OCC_RESPONSE_ADDR 0xFFFF7000<br>> <br>> Are these addresses going to change with different versions of the <br>> OCC firmware?<br>></tt><br><br><tt>Actually I did not find the document for most of the addresses. I copied them from Alvin's OCC code.</tt><br><tt>If anyone know the source (possibly from OCC firmware developers), please help.</tt><br><tt>The only OCC document I can find is:</tt><br><a href="https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf"><tt>https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf</tt></a><br><tt>It defines OCC_COMMAND_ADDR and OCC_RESPONSE_ADDR only.</tt><br><br><tt>> <br>> > +<br>> > + if (!p)<br>> > + return;<br>> <br>> It's common in kernel code to omit checking for null arguments.<br>> <br>> If you think there is a likely hood of this happening, then perhaps<br>> WARN_ON(!b) would be better; this way we get a backtrace in the kernel<br>> log.<br>> <br></tt><br><tt>The </tt><tt>deinit_occ_resp_buf() function is supposed to be called in multiple situation.</tt><br><tt>E.g, error handling code in a function. In those case, pointer might be NULL.</tt><br><tt>So WARN_ON() is not expected in that case.</tt><br><br><br><tt>> > +<br>> > + if (!p->blocks)<br>> > + return;<br>> > +<br>> > + for (b = 0; b < p->header.sensor_block_num; b++) {<br>> > + if (p->blocks[b].sensor)<br>> > + kfree(p->blocks[b].sensor);<br>> > + if (p->blocks[b].power)<br>> > + kfree(p->blocks[b].power);<br>> > + if (p->blocks[b].caps)<br>> > + kfree(p->blocks[b].caps);<br>> > + }<br>> <br>> I think Jeremy suggested in his review that you statically allocate<br>> these instead of kmalloc and kfreeing at runtime.<br>> <br></tt><br><tt>The OCC sensor type and sensor number will change depending on whether OCC is master or slave.</tt><br><tt>Also there is possible of bad cores and the number of sensors may change.</tt><br><tt>So using dynamically allocated sensor block is nature I think.</tt><br><tt>It is possible that the driver allocates a block of memory and manage itself.</tt><br><tt>But looks there is no great benefit.</tt><br><tt><br>> > +{<br>> > + int b;<br>> > + int s;<br>> > + int ret;<br>> > + int dnum = SENSOR_BLOCK_OFFSET;<br>> > + struct occ_sensor *f_sensor;<br>> > + struct occ_sensor *t_sensor;<br>> > + struct power_sensor *p_sensor;<br>> > + struct caps_sensor *c_sensor;<br>> > + uint8_t sensor_block_num;<br>> > + uint8_t sensor_type[4];<br>> > + uint8_t sensor_format;<br>> > + uint8_t sensor_length;<br>> > + uint8_t num_of_sensors;<br>> > +<br>> > + /* check if the data is valid */<br>> > + if (strncmp(&d[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {<br>> <br>> If you have a literal string you don't need to use strncmp.<br>> <br></tt><br><tt>"d[SENSOR_STR_OFFSET]" does not have '\0' to terminate a string. So have to use strncmp.<br></tt><br><br><tt>> <br>> > + p_sensor->accumulator =<br>> > + (d[dnum+6] << 24) | (d<br>> [dnum+7] << 16) |<br>> > + (d[dnum+8] << 8) | d[dnum+9];<br>> > + p_sensor->value =<br>> > + (d[dnum+10] << 8) | d[dnum+11];<br>> > +<br>> > + dev_dbg(&client->dev,<br>> > + "sensor[%d]-[%d]: id: %u, <br>> value: %u\n",<br>> > + b, s, p_sensor->sensor_id,<br>> > + p_sensor->value);<br>> > +<br>> > + dnum = dnum + sensor_length;</tt><br><tt>> > + }<br>> > + } else if (strncmp(sensor_type, "CAPS", 4) == 0) {<br>> <br>> If you have a literal string you don't need to use strncmp.</tt><br><br><tt>"sensor_type" has no '\0' to terminate it. strncmp() is necessary. </tt><br><tt><br><br>> > + val = -1;<br>> > + else<br>> > + /* in millidegree Celsius */<br>> > + val = sensor[n].value * 1000;<br>> > +<br>> > + return sprintf(buf, "%d\n", val);<br>> <br>> How do you know "%d\n" will fit into buf?</tt><br><br><tt>Do you mean using snprintf()? After reading the man page:</tt><br><tt>"Because sprintf() and vsprintf() assume an arbitrarily long string, callers must be careful not to overflow the actual space; this</tt><br><tt> is often impossible to assure. Note that the length of the strings produced is locale-dependent and difficult to predict. Use</tt><br><tt> snprintf() and vsnprintf() instead (or asprintf(3) and vasprintf(3))."</tt><br><br><tt>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.</tt><br><tt>Anyway, I will change to snprintf() to be safer:</tt><br><tt>I use the same code as driver/hwmon/</tt><tt>lm95241.c</tt><br><br><tt>"</tt><tt> return snprintf(buf, PAGE_SIZE - 1,</tt><br><tt> data->config & to_sensor_dev_attr(attr)->index ?</tt><br><tt> "127000\n" : "255000\n");</tt><br><tt>"</tt><br><br><tt>Thanks,</tt><br><tt>-adam</tt><br><br><tt><br></tt><BR>
</body></html>