[PATCH linux 1/7] hwmon: Add core On-Chip Controller support for POWER CPUs

[Andrew Jeffery]

Hi Eddie,

On Mon, 2016-12-19 at 17:05 -0600, Eddie James wrote:
...
> > > +
> > > +static int parse_occ_response(struct occ *driver, u8 *data,
> > > > +                         struct occ_response *resp)
> > > 
> > > +{
> > > > +   int b;
> > > > +   int s;
> > > > +   int rc;
> > > > +   int offset = SENSOR_BLOCK_OFFSET;
> > > > +   int sensor_type;
> > > > +   u8 sensor_block_num;
> > > 
> > > +     char sensor_type_string[5] = { 0 };
> > 
> > Given how you rearranged things below, do you think we can get away
> > with removing sensor_type_string? Also, struct sensor_data_block_type
> > defines sensor_type as a char * that can't be null-terminated as its
> > size is 4, which still makes me feel uncomfortable. What are your
> > thoughts here?
> 
> The idea of sensor_type_string was to have a null-terminated string
> that I could use in the debug statements below. sensor_type is
> restricted to 4 chars because that's how its' formatted in the OCC
> data block. Anywhere I use sensor_type I use the length-limited string
> function, such as strncmp or the like, and restrict it to 4 chars. I
> feel this is an acceptable solution, but you're right,
> sensor_type_string isn't strictly necessary, just useful to be able to
> print that out.

Okay, I'm happy with that.

> 
> > > +
> > > +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16
> > > length,
> > > > +                  u8 *data, u8 *resp)
> > > 
> > > +{
> > > > +   u32 cmd1, cmd2;
> > > > +   u16 checksum = 0;
> > > > +   u16 length_be = cpu_to_be16(length);
> > > > +   int i, rc, tries = 0;
> > > 
> > > +     const int max_tries = (OCC_BMC_TIMEOUT_S * 1000) /
> > > CMD_IN_PRG_INT_MS;
> > 
> > I expect we should just #define the expression?
> > 
> > > +
> > > > +   cmd1 = (seq << 24) | (type << 16) | length_be;
> > > > +   memcpy(&cmd2, data, length);
> > > > +   cmd2 <<= ((4 - length) * 8);
> > > 
> > > +
> > > > +   /* checksum: sum of every bytes of cmd1, cmd2 */
> > > > +   for (i = 0; i < 4; i++) {
> > > > +           checksum += (cmd1 >> (i * 8)) & 0xFF;
> > > 
> > > +             checksum += (cmd2 >> (i * 8)) & 0xFF;
> > 
> > I have to apologise here, as my review was wrong: Addition is
> > commutative, but not when we're masking the result! So the original
> > approach is probably more correct, and it only occurred to me when
> > reading over this patch. Sorry! Did you get checksum errors as a
> > result?
> 
> No worries, I didn't catch it either, but I think it worked out OK
> since the mask is really applied to the shifted cmd, 

Yes, I thought about this further after I sent the email and I agree.

> but I will test
> both ways to make sure... Thanks!
> 
> > 
> > > +     }
> > > +
> > > > +   cmd2 |= checksum << ((2 - length) * 8);
> > > 
> > > +
> > > > +   /* Init OCB */
> > > > +   rc = driver->bus_ops.putscom(driver->bus,
> > > > OCB_STATUS_CONTROL_OR,
> > > > +                                OCB_OR_INIT0, OCB_OR_INIT1);
> > > > +   if (rc)
> > > > +           goto err;
> > > 
> > > +
> > > > +   rc = driver->bus_ops.putscom(driver->bus,
> > > > OCB_STATUS_CONTROL_AND,
> > > > +                                OCB_AND_INIT0,
> > > > OCB_AND_INIT1);
> > > > +   if (rc)
> > > > +           goto err;
> > > 
> > > +
> > > > +   /* Send command, 2nd half of the 64-bit addr is unused
> > > > (write 0) */
> > > > +   rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> > > > +                                driver->config.command_addr,
> > > > 0);
> > > > +   if (rc)
> > > > +           goto err;
> > > 
> > > +
> > > > +   rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> > > > +                                driver->config.command_addr,
> > > > 0);
> > > > +   if (rc)
> > > 
> > > +             goto err;
> > 
> > This change highlighted to me that we're sending command_addr twice -
> > is this necessary?
> 
> I'm not sure... I'll test both without and see if everything works OK.
> I noticed it was sent twice, but I assumed Yi had some reason for
> doing it.

I think this will be a win either way: Either we remove one, or we
document why we send it twice.

> 
> > 
> > > +
> > > > +   rc = driver->bus_ops.putscom(driver->bus, OCB_DATA, cmd1,
> > > > cmd2);
> > > > +   if (rc)
> > > > +           goto err;
> > > 
> > > +
> > > > +   /* Trigger attention */
> > > > +   rc = driver->bus_ops.putscom(driver->bus, ATTN_DATA,
> > > > ATTN0, ATTN1);
> > > > +   if (rc)
> > > > +           goto err;
> > > 
> > > +
> > > > +   /* Get response data */
> > > > +   rc = driver->bus_ops.putscom(driver->bus, OCB_ADDRESS,
> > > > +                                driver->config.response_addr,
> > > > 0);
> > > > +   if (rc)
> > > > +           goto err;
> > > 
> > > +
> > > > +   do {
> > > > +           rc = driver->bus_ops.getscom(driver->bus,
> > > > OCB_DATA, resp);
> > > > +           if (rc)
> > > > +                   goto err;
> > > 
> > > +
> > > > +           /* retry if we get "command in progress" return
> > > > status */
> > > > +           if (resp[RESP_RETURN_STATUS] ==
> > > > RESP_RETURN_CMD_IN_PRG) {
> > > > +                   set_current_state(TASK_INTERRUPTIBLE);
> > > > +                   schedule_timeout(msecs_to_jiffies(CMD_IN_P
> > > > RG_INT_MS));
> > > > +                   tries++;
> > > > +           } else
> > > > +                   break;
> > > 
> > > +     } while (tries < max_tries);
> > 
> > I'll bikeshed this to try save the break and a timeout, which is really
> > picking at the nits but reduces cyclomatic complexity and saves some
> > time. What do you think of:
> > 
> >     bool retry = false;
> >     int tries = 0;
> > 
> >     ...
> > 
> >     do {
> >         if (retry) {
> >             set_current_state(TASK_INTERRUPTIBLE);
> >             shedule_timeout(msecs_to_jiffies(CMD_IN_PRG_INT_MS));
> >         }
> > 
> >         rc = driver->bus_ops.getscom(driver->bus, OCB_DATA, resp);
> >         if (rc)
> >             goto err;
> > 
> >         retry = (resp[RESP_RETURN_STATUS] == RESP_RETURN_CMD_IN_PRG) && tries++ < max_tries);
> >     } while (retry);
> > 
> > Are we okay with returning RESP_RETURN_CMD_IN_PRG to the caller?
> > Because that's what happens in either case if we exceed max_tries.
> 
> Looks a little bit cleaner, I'll incorporate this. And yes, any
> non-zero return will be flagged as an error. If we timeout and are are
> still getting the busy response we do want an error.

But a "kernel" error? Or the value RESP_RETURN_CMD_IN_PRG?

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161220/b1dedcaf/attachment.sig>