Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200220155859.906647-1-enric.balletbo@collabora.com>
 <20200220155859.906647-5-enric.balletbo@collabora.com> <20200225195519.GC11971@google.com>
 <4e441ead-44f3-1787-28a3-3e71893105f9@collabora.com>
In-Reply-To: <4e441ead-44f3-1787-28a3-3e71893105f9@collabora.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Wed, 26 Feb 2020 09:32:36 -0800
Message-ID: <CACeCKac0c9e80txmyo3UZ8hU3rWj07n=pJai4mWzWae_jQ0Ueg@mail.gmail.com>
Subject: Re: [PATCH 4/8] platform/chrome: cros_ec_chardev: Use
 cros_ec_cmd_xfer_status helper
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Collabora Kernel ML <kernel@collabora.com>,
        Guenter Roeck <groeck@chromium.org>,
        Benson Leung <bleung@chromium.org>,
        Dmitry Torokhov <dtor@chromium.org>,
        Gwendal Grignou <gwendal@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Enric,

On Wed, Feb 26, 2020 at 7:00 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 25/2/20 20:55, Prashant Malani wrote:
> > Hi Enric,
> >
> > On Thu, Feb 20, 2020 at 04:58:55PM +0100, Enric Balletbo i Serra wrote:
> >> This patch makes use of cros_ec_cmd_xfer_status() instead of
> >> cros_ec_cmd_xfer(). In this case the change is trivial and the only
> >> reason to do it is because we want to make cros_ec_cmd_xfer() a private
> >> function for the EC protocol and let people only use the
> >> cros_ec_cmd_xfer_status() to return Linux standard error codes.
> >>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >>  drivers/platform/chrome/cros_ec_chardev.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/platform/chrome/cros_ec_chardev.c b/drivers/platform/chrome/cros_ec_chardev.c
> >> index c65e70bc168d..b51ab24055f3 100644
> >> --- a/drivers/platform/chrome/cros_ec_chardev.c
> >> +++ b/drivers/platform/chrome/cros_ec_chardev.c
> >> @@ -301,7 +301,7 @@ static long cros_ec_chardev_ioctl_xcmd(struct cros_ec_dev *ec, void __user *arg)
> >>      }
> >>
> >>      s_cmd->command += ec->cmd_offset;
> >> -    ret = cros_ec_cmd_xfer(ec->ec_dev, s_cmd);
> >> +    ret = cros_ec_cmd_xfer_status(ec->ec_dev, s_cmd);
> >
> > One issue I see here is that if we were to later convert
> > cros_ec_cmd_xfer_status() to cros_ec_cmd(), we would lose the
> > s_cmd->result value, since I was hoping to avoid returning msg->result
> > via a pointer parameter. I don't know if userspace actually uses that, but I
> > am assuming it does.
> >
>
> I'd like to avoid returning msg->result via a pointer parameter too. I didn't
> analyze all the cases but I suspect that in most cases it is not really needed,
> so let's start by converting to the cros_ec_cmd the cases that are clear and
> let's go one by one for the ones that are not clear.
>
I think the major one I was concerned about was in user-space:
https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/util/comm-dev.c#149

The above seems to return the result (after applying an offset).
FWIU the attempt would be to not change the user-space API, so would
we need to change this user-space too?

Best regards,

> IMO cros_ec_cmd should return the same as cros_ec_cmd_xfer_status. So you should
> use cros_ec_cmd_xfer_status inside cros_ec_cmd.
>
> Regards,
>  Enric
>
> > So, should cros_ec_cmd() keep the *result pointer like so ?:
> >
> > int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command,
> >                void *outdata, u32 outsize, void *indata, u32 insize, u32 *result);
> >
> > This way, we can manually re-populate s_cmd->result with |*result|.
> >
> > Or, should we drop msg->result while returning s_cmd to userspace? I am
> > guessing the answer is no, but thought I'd check with you first.
> >
> > Best regards,
> >
> >
> >>      /* Only copy data to userland if data was received. */
> >>      if (ret < 0)
> >>              goto exit;
> >> --
> >> 2.25.0
> >>