Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation

[Eddie James <eajames@linux.ibm.com>]

On Wed, 2021-07-21 at 02:43 +0000, Joel Stanley wrote:
> On Fri, 16 Jul 2021 at 15:19, Eddie James <eajames@linux.ibm.com>
> wrote:
> > Checksumming of the request and sequence numbering is now done in
> > the
> > OCC interface driver in order to keep unique sequence numbers. So
> > remove those in the hwmon driver.
> > 
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > ---
> >  drivers/hwmon/occ/common.c | 30 ++++++++++++------------------
> >  drivers/hwmon/occ/common.h |  3 +--
> >  drivers/hwmon/occ/p8_i2c.c | 15 +++++++++------
> >  drivers/hwmon/occ/p9_sbe.c |  4 ++--
> >  4 files changed, 24 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/hwmon/occ/common.c
> > b/drivers/hwmon/occ/common.c
> > index 0d68a78be980..fc298268c89e 100644
> > --- a/drivers/hwmon/occ/common.c
> > +++ b/drivers/hwmon/occ/common.c
> > @@ -132,22 +132,20 @@ struct extended_sensor {
> >  static int occ_poll(struct occ *occ)
> >  {
> >         int rc;
> > -       u16 checksum = occ->poll_cmd_data + occ->seq_no + 1;
> > -       u8 cmd[8];
> > +       u8 cmd[7];
> 
> The shortening of the command seems unrelated?
> 
> If you leave it at 8 then you avoid the special casing below. Is
> there
> any downside to sending the extra 0 byte at the end?

Yes, it would break the checksumming unfortunately. The checksum is
calculated and added at the last two bytes, so if you send more than
your command actually is, the checksum will be in the wrong spot.
> 
> >         struct occ_poll_response_header *header;
> > 
> >         /* big endian */
> > -       cmd[0] = occ->seq_no++;         /* sequence number */
> > +       cmd[0] = 0;                     /* sequence number */
> >         cmd[1] = 0;                     /* cmd type */
> >         cmd[2] = 0;                     /* data length msb */
> >         cmd[3] = 1;                     /* data length lsb */
> >         cmd[4] = occ->poll_cmd_data;    /* data */
> > -       cmd[5] = checksum >> 8;         /* checksum msb */
> > -       cmd[6] = checksum & 0xFF;       /* checksum lsb */
> > -       cmd[7] = 0;
> > +       cmd[5] = 0;                     /* checksum msb */
> > +       cmd[6] = 0;                     /* checksum lsb */
> > --- a/drivers/hwmon/occ/p8_i2c.c> +++ b/drivers/hwmon/occ/p8_i2c.c
> > @@ -97,18 +97,21 @@ static int p8_i2c_occ_putscom_u32(struct
> > i2c_client *client, u32 address,
> >  }
> > 
> >  static int p8_i2c_occ_putscom_be(struct i2c_client *client, u32
> > address,
> > -                                u8 *data)
> > +                                u8 *data, size_t len)
> >  {
> > -       __be32 data0, data1;
> > +       __be32 data0 = 0, data1 = 0;
> > 
> > -       memcpy(&data0, data, 4);
> > -       memcpy(&data1, data + 4, 4);
> > +       memcpy(&data0, data, min(len, 4UL));
> 
> The UL here seems unnecessary (and dropping it should fix your 0day
> bot warnings).

Yea, I think I just need min_t

Thanks for the review!
Eddie

> 
> But I think it would be simpler to go back to a fixed length of 8.
> 
> > +       if (len > 4UL) {
> > +               len -= 4;
> > +               memcpy(&data1, data + 4, min(len, 4UL));
> > +       }
> > 
> >         return p8_i2c_occ_putscom_u32(client, address,
> > be32_to_cpu(data0),
> >                                       be32_to_cpu(data1));
> >  }