Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210716151850.28973-1-eajames@linux.ibm.com> <20210716151850.28973-3-eajames@linux.ibm.com>
In-Reply-To: <20210716151850.28973-3-eajames@linux.ibm.com>
From:   Joel Stanley <joel@jms.id.au>
Date:   Wed, 21 Jul 2021 02:43:08 +0000
Message-ID: <CACPK8XcgF7i+b8P1AUDRYtWZeMDwG7Mjw74pFpVKVx6ZdJJKzw@mail.gmail.com>
Subject: Re: [PATCH 2/3] hwmon: (occ) Remove sequence numbering and checksum calculation
To:     Eddie James <eajames@linux.ibm.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-hwmon@vger.kernel.org, linux-fsi@lists.ozlabs.org,
        Guenter Roeck <linux@roeck-us.net>,
        Jean Delvare <jdelvare@suse.com>, Jeremy Kerr <jk@ozlabs.org>,
        Alistair Popple <alistair@popple.id.au>,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

>         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).

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));
>  }