| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPUE2usO-Ny61+wEdTcwR3b+RgGjeQ4Jb24UeF8siscqFQ5ogQ@mail.gmail.com>
Date: Fri, 7 Feb 2020 10:47:15 -0800
From: Gwendal Grignou <gwendal@...omium.org>
To: Prashant Malani <pmalani@...omium.org>
Cc: Enric Balletbo i Serra <enric.balletbo@...labora.com>,
Jonathan Cameron <jic23@...nel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Lee Jones <lee.jones@...aro.org>,
Fabien Lahoudere <fabien.lahoudere@...labora.com>,
"open list:IIO SUBSYSTEM AND DRIVERS" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 10/17] iio: cros_ec: Use cros_ec_cmd()
On Thu, Feb 6, 2020 at 10:50 AM Prashant Malani <pmalani@...omium.org> wrote:
>
> Hi Enric,
>
> Thanks for taking a look at the patch. Please see my response inline:
>
> On Thu, Feb 6, 2020 at 5:45 AM Enric Balletbo i Serra
> <enric.balletbo@...labora.com> wrote:
> >
> > Hi Prashant,
> >
> > On 6/2/20 13:17, Jonathan Cameron wrote:
> > > On Wed, 5 Feb 2020 11:00:13 -0800
> > > Prashant Malani <pmalani@...omium.org> wrote:
> > >
> > >> Replace cros_ec_cmd_xfer_status() with cros_ec_cmd()
> > >> which does the message buffer setup and cleanup.
> > >>
> > >> For one other usage, replace the cros_ec_cmd_xfer_status() call with a
> > >> call to cros_ec_cmd_xfer(), in preparation for the removal of the former
> > >> function.
> > >>
> > >> Signed-off-by: Prashant Malani <pmalani@...omium.org>
> > >
> > > Acked-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > >
> > >> ---
> > >>
> > >> Changes in v2:
> > >> - Updated to use new function name and parameter list.
> > >> - Used C99 element setting to initialize param struct.
> > >> - For second usage, replaced cros_ec_cmd_xfer_status() with
> > >> cros_ec_cmd_xfer() which is functionally similar.
> > >>
> > >> .../cros_ec_sensors/cros_ec_sensors_core.c | 25 +++++++------------
> > >> 1 file changed, 9 insertions(+), 16 deletions(-)
> > >>
> > >> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > >> index d3a3626c7cd834..94e22e7d927631 100644
> > >> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > >> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > >> @@ -30,24 +30,15 @@ static int cros_ec_get_host_cmd_version_mask(struct cros_ec_device *ec_dev,
> > >> u16 cmd_offset, u16 cmd, u32 *mask)
> > >> {
> > >> int ret;
> > >> - struct {
> > >> - struct cros_ec_command msg;
> > >> - union {
> > >> - struct ec_params_get_cmd_versions params;
> > >> - struct ec_response_get_cmd_versions resp;
> > >> - };
> > >> - } __packed buf = {
> > >> - .msg = {
> > >> - .command = EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> > >> - .insize = sizeof(struct ec_response_get_cmd_versions),
> > >> - .outsize = sizeof(struct ec_params_get_cmd_versions)
> > >> - },
> > >> - .params = {.cmd = cmd}
> > >> + struct ec_params_get_cmd_versions params = {
> > >> + .cmd = cmd,
> > >> };
> > >> + struct ec_response_get_cmd_versions resp = {0};
> > >>
> > >> - ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > >> + ret = cros_ec_cmd(ec_dev, 0, EC_CMD_GET_CMD_VERSIONS + cmd_offset,
> > >> + ¶ms, sizeof(params), &resp, sizeof(resp), NULL);
> > >> if (ret >= 0)
> > >> - *mask = buf.resp.version_mask;
> > >> + *mask = resp.version_mask;
> > >> return ret;
> > >> }
> > >>
> > >> @@ -171,9 +162,11 @@ int cros_ec_motion_send_host_cmd(struct cros_ec_sensors_core_state *state,
> > >>
> > >> memcpy(state->msg->data, &state->param, sizeof(state->param));
> > >>
> > >> - ret = cros_ec_cmd_xfer_status(state->ec, state->msg);
> > >> + ret = cros_ec_cmd_xfer(state->ec, state->msg);
> > >> if (ret < 0)
> > >> return ret;
> > >> + else if (state->msg->result != EC_RES_SUCCESS)
> > >> + return -EPROTO;
> > >>
> >
> > There is no way to use the new cros_ec_cmd here?
When the EC does not support sensor fifo,
cros_ec_motion_send_host_cmd() is on the data path. For instance, it
is called 2 times every 10ms by chrome to calculate the lid angle. I
would be reluctant to call malloc. Given it is well encapsulated into
the sensor stack. Does it make sense to call cros_ec_cmd_xfer
directly?
Gwendal.
>
> I think it is doable. From looking at the code I felt the factors we
> need to be careful about are:
> - The function cros_ec_motion_send_host_cmd() is called from a few
> other files, each of which set up the struct cros_ec_command
> differently (reference:
> https://elixir.bootlin.com/linux/latest/ident/cros_ec_motion_send_host_cmd)
> - It is not clear to me how readability will be affected by making the
> change to cros_ec_cmd().
>
> Due to the above two factors, but primarily because I wanted to avoid
> making such an involved large change in this 17 patch series, I
> reasoned it would be better to make the transition to cros_ec_cmd()
> for these files in a separate patch/series.
> My plan after this patch series is to work on this driver(perhaps we
> can eliminate cros_ec_motion_send_host_cmd() itself?), and then remove
> cros_ec_cmd_xfer() usage.
>
> WDYT?
>
> Best regards,
>
>
> >
> >
> > >> if (ret &&
> > >> state->resp != (struct ec_response_motion_sense *)state->msg->data)
> > >
Powered by blists - more mailing lists