| 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: <CABXOdTdsVyae9oHKEMSbXOsi5VWUZhLbKVxeNVedc4redZineQ@mail.gmail.com>
Date: Thu, 10 Sep 2020 07:52:08 -0700
From: Guenter Roeck <groeck@...gle.com>
To: Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc: Stephen Boyd <swboyd@...omium.org>,
Benson Leung <bleung@...omium.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Gwendal Grignou <gwendal@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it
isn't there
On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
<enric.balletbo@...labora.com> wrote:
>
> Hi,
>
> On 10/9/20 16:18, Guenter Roeck wrote:
> > On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >>
> >> The cros ec lightbar driver has a check in probe to fail early if the ec
> >> device isn't the cros_ec one or if it can't read the lightbar version
> >> from the EC. Let's move this check to the place where the lightbar
> >> device is registered. This way we don't expose devices that aren't
> >> actually there on devices that don't have a lightbar.
> >>
> >> This should improve memory and possibly performance too because struct
> >> devices don't exactly have a small memory footprint and they contribute
> >> to suspend/resume regardless of driver attachment.
> >>
> >> Cc: Gwendal Grignou <gwendal@...omium.org>
> >> Cc: Guenter Roeck <groeck@...omium.org>
> >> Cc: Lee Jones <lee.jones@...aro.org>
> >> Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> >> ---
> >>
> >> Changes from v1:
> >> * Rebased on linux-next patches to this file
> >>
> >> drivers/mfd/cros_ec_dev.c | 16 ++-
> >> drivers/platform/chrome/cros_ec_lightbar.c | 102 ++------------------
> >> drivers/platform/chrome/cros_ec_proto.c | 84 ++++++++++++++++
> >> include/linux/platform_data/cros_ec_proto.h | 4 +
> >> 4 files changed, 111 insertions(+), 95 deletions(-)
> >>
> >> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> >> index d07b43d7c761..9e98b2ec5d92 100644
> >> --- a/drivers/mfd/cros_ec_dev.c
> >> +++ b/drivers/mfd/cros_ec_dev.c
> >> @@ -74,6 +74,10 @@ static const struct mfd_cell cros_ec_cec_cells[] = {
> >> { .name = "cros-ec-cec", },
> >> };
> >>
> >> +static const struct mfd_cell cros_ec_lightbar_cells[] = {
> >> + { .name = "cros-ec-lightbar", },
> >> +};
> >> +
> >> static const struct mfd_cell cros_ec_rtc_cells[] = {
> >> { .name = "cros-ec-rtc", },
> >> };
> >> @@ -112,7 +116,6 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
> >> static const struct mfd_cell cros_ec_platform_cells[] = {
> >> { .name = "cros-ec-chardev", },
> >> { .name = "cros-ec-debugfs", },
> >> - { .name = "cros-ec-lightbar", },
> >> { .name = "cros-ec-sysfs", },
> >> };
> >>
> >> @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device *pdev)
> >> }
> >> }
> >>
> >> + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> >> + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
> >
> > Any idea why the lightbar code doesn't use cros_ec_check_features() ?
> > There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
> > be used. It would be much more convenient if that feature check could
> > be used instead of moving the get_lightbar_version command and its
> > helper function around.
> >
>
> IIRC it was to support a very old device, the Pixel Chromebook (Link). This flag
> is not set in this device but has a lightbar, hence we had this 'weird' way to
> detect the lightbar.
>
If that is the only reason, wouldn't it be better to use something
else (eg dmi_match) to determine if the system in question is a Pixel
Chromebook (Link) ?
if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
(cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) ||
dmi_match(DMI_PRODUCT_NAME, "Link")) {
Thanks,
Guenter
> > Thanks,
> > Guenter
> >
> >> + retval = mfd_add_hotplug_devices(ec->dev,
> >> + cros_ec_lightbar_cells,
> >> + ARRAY_SIZE(cros_ec_lightbar_cells));
> >> + if (retval)
> >> + dev_err(ec->dev,
> >> + "failed to add lightbar device: %d\n",
> >> + retval);
> >> + }
> >> +
> >> /*
> >> * The PD notifier driver cell is separate since it only needs to be
> >> * explicitly added on platforms that don't have the PD notifier ACPI
> >> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> >> index de8dfb12e486..a7cfde90c8e5 100644
> >> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> >> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> >> @@ -82,77 +82,6 @@ static int lb_throttle(void)
> >> return ret;
> >> }
> >>
> >> -static struct cros_ec_command *alloc_lightbar_cmd_msg(struct cros_ec_dev *ec)
> >> -{
> >> - struct cros_ec_command *msg;
> >> - int len;
> >> -
> >> - len = max(sizeof(struct ec_params_lightbar),
> >> - sizeof(struct ec_response_lightbar));
> >> -
> >> - msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
> >> - if (!msg)
> >> - return NULL;
> >> -
> >> - msg->version = 0;
> >> - msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
> >> - msg->outsize = sizeof(struct ec_params_lightbar);
> >> - msg->insize = sizeof(struct ec_response_lightbar);
> >> -
> >> - return msg;
> >> -}
> >> -
> >> -static int get_lightbar_version(struct cros_ec_dev *ec,
> >> - uint32_t *ver_ptr, uint32_t *flg_ptr)
> >> -{
> >> - struct ec_params_lightbar *param;
> >> - struct ec_response_lightbar *resp;
> >> - struct cros_ec_command *msg;
> >> - int ret;
> >> -
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> - if (!msg)
> >> - return 0;
> >> -
> >> - param = (struct ec_params_lightbar *)msg->data;
> >> - param->cmd = LIGHTBAR_CMD_VERSION;
> >> - msg->outsize = sizeof(param->cmd);
> >> - msg->result = sizeof(resp->version);
> >> - ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> >> - if (ret < 0 && ret != -EINVAL) {
> >> - ret = 0;
> >> - goto exit;
> >> - }
> >> -
> >> - switch (msg->result) {
> >> - case EC_RES_INVALID_PARAM:
> >> - /* Pixel had no version command. */
> >> - if (ver_ptr)
> >> - *ver_ptr = 0;
> >> - if (flg_ptr)
> >> - *flg_ptr = 0;
> >> - ret = 1;
> >> - goto exit;
> >> -
> >> - case EC_RES_SUCCESS:
> >> - resp = (struct ec_response_lightbar *)msg->data;
> >> -
> >> - /* Future devices w/lightbars should implement this command */
> >> - if (ver_ptr)
> >> - *ver_ptr = resp->version.num;
> >> - if (flg_ptr)
> >> - *flg_ptr = resp->version.flags;
> >> - ret = 1;
> >> - goto exit;
> >> - }
> >> -
> >> - /* Anything else (ie, EC_RES_INVALID_COMMAND) - no lightbar */
> >> - ret = 0;
> >> -exit:
> >> - kfree(msg);
> >> - return ret;
> >> -}
> >> -
> >> static ssize_t version_show(struct device *dev,
> >> struct device_attribute *attr, char *buf)
> >> {
> >> @@ -165,7 +94,7 @@ static ssize_t version_show(struct device *dev,
> >> return ret;
> >>
> >> /* This should always succeed, because we check during init. */
> >> - if (!get_lightbar_version(ec, &version, &flags))
> >> + if (cros_ec_get_lightbar_version(ec, &version, &flags))
> >> return -EIO;
> >>
> >> return scnprintf(buf, PAGE_SIZE, "%d %d\n", version, flags);
> >> @@ -184,7 +113,7 @@ static ssize_t brightness_store(struct device *dev,
> >> if (kstrtouint(buf, 0, &val))
> >> return -EINVAL;
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -222,7 +151,7 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
> >> unsigned int val[4];
> >> int ret, i = 0, j = 0, ok = 0;
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -289,7 +218,7 @@ static ssize_t sequence_show(struct device *dev,
> >> int ret;
> >> struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -324,7 +253,7 @@ static int lb_send_empty_cmd(struct cros_ec_dev *ec, uint8_t cmd)
> >> struct cros_ec_command *msg;
> >> int ret;
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -352,7 +281,7 @@ static int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
> >> struct cros_ec_command *msg;
> >> int ret;
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -399,7 +328,7 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
> >> return ret;
> >> }
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -444,7 +373,7 @@ static ssize_t program_store(struct device *dev, struct device_attribute *attr,
> >> return -EINVAL;
> >> }
> >>
> >> - msg = alloc_lightbar_cmd_msg(ec);
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> if (!msg)
> >> return -ENOMEM;
> >>
> >> @@ -531,24 +460,9 @@ static struct attribute_group cros_ec_lightbar_attr_group = {
> >> static int cros_ec_lightbar_probe(struct platform_device *pd)
> >> {
> >> struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> >> - struct cros_ec_platform *pdata = dev_get_platdata(ec_dev->dev);
> >> struct device *dev = &pd->dev;
> >> int ret;
> >>
> >> - /*
> >> - * Only instantiate the lightbar if the EC name is 'cros_ec'. Other EC
> >> - * devices like 'cros_pd' doesn't have a lightbar.
> >> - */
> >> - if (strcmp(pdata->ec_name, CROS_EC_DEV_NAME) != 0)
> >> - return -ENODEV;
> >> -
> >> - /*
> >> - * Ask then for the lightbar version, if it's 0 then the 'cros_ec'
> >> - * doesn't have a lightbar.
> >> - */
> >> - if (!get_lightbar_version(ec_dev, NULL, NULL))
> >> - return -ENODEV;
> >> -
> >> /* Take control of the lightbar from the EC. */
> >> lb_manual_suspend_ctrl(ec_dev, 1);
> >>
> >> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> >> index dda182132a6a..a08747eec36a 100644
> >> --- a/drivers/platform/chrome/cros_ec_proto.c
> >> +++ b/drivers/platform/chrome/cros_ec_proto.c
> >> @@ -922,3 +922,87 @@ int cros_ec_get_sensor_count(struct cros_ec_dev *ec)
> >> return sensor_count;
> >> }
> >> EXPORT_SYMBOL_GPL(cros_ec_get_sensor_count);
> >> +
> >> +struct cros_ec_command *cros_ec_alloc_lightbar_cmd_msg(struct cros_ec_dev *ec)
> >> +{
> >> + struct cros_ec_command *msg;
> >> + int len;
> >> +
> >> + len = max(sizeof(struct ec_params_lightbar),
> >> + sizeof(struct ec_response_lightbar));
> >> +
> >> + msg = kmalloc(sizeof(*msg) + len, GFP_KERNEL);
> >> + if (!msg)
> >> + return NULL;
> >> +
> >> + msg->version = 0;
> >> + msg->command = EC_CMD_LIGHTBAR_CMD + ec->cmd_offset;
> >> + msg->outsize = sizeof(struct ec_params_lightbar);
> >> + msg->insize = sizeof(struct ec_response_lightbar);
> >> +
> >> + return msg;
> >> +}
> >> +EXPORT_SYMBOL_GPL(cros_ec_alloc_lightbar_cmd_msg);
> >> +
> >> +/**
> >> + * cros_ec_get_lightbar_version() - Get the EC lightbar version
> >> + *
> >> + * @ec: EC device, does not have to be connected directly to the AP,
> >> + * can be daisy chained through another device.
> >> + * @ver_ptr: Detected lightbar version number
> >> + * @flag_ptr: Detected lightbar flags
> >> + *
> >> + * Call this function to determine the EC's lightbar version and flags
> >> + * information. If it doesn't exist then this function returns a negative
> >> + * error value.
> >> + *
> >> + * Return: 0 on success, negative errno on failure to detect a lightbar.
> >> + */
> >> +int cros_ec_get_lightbar_version(struct cros_ec_dev *ec, uint32_t *ver_ptr,
> >> + uint32_t *flg_ptr)
> >> +{
> >> + struct ec_params_lightbar *param;
> >> + struct ec_response_lightbar *resp;
> >> + struct cros_ec_command *msg;
> >> + int ret;
> >> +
> >> + msg = cros_ec_alloc_lightbar_cmd_msg(ec);
> >> + if (!msg)
> >> + return -ENOMEM;
> >> +
> >> + param = (struct ec_params_lightbar *)msg->data;
> >> + param->cmd = LIGHTBAR_CMD_VERSION;
> >> + msg->outsize = sizeof(param->cmd);
> >> + msg->result = sizeof(resp->version);
> >> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> >> + if (ret < 0 && ret != -EINVAL)
> >> + goto exit;
> >> +
> >> + switch (msg->result) {
> >> + case EC_RES_INVALID_PARAM:
> >> + /* Pixel had no version command. */
> >> + if (ver_ptr)
> >> + *ver_ptr = 0;
> >> + if (flg_ptr)
> >> + *flg_ptr = 0;
> >> + ret = 0;
> >> + break;
> >> + case EC_RES_SUCCESS:
> >> + resp = (struct ec_response_lightbar *)msg->data;
> >> +
> >> + /* Future devices w/lightbars should implement this command */
> >> + if (ver_ptr)
> >> + *ver_ptr = resp->version.num;
> >> + if (flg_ptr)
> >> + *flg_ptr = resp->version.flags;
> >> + ret = 0;
> >> + break;
> >> + default:
> >> + /* Anything else (ie, EC_RES_INVALID_COMMAND) - no lightbar */
> >> + ret = -ENODEV;
> >> + }
> >> +exit:
> >> + kfree(msg);
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(cros_ec_get_lightbar_version);
> >> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> >> index 4a415ae851ef..50221254956c 100644
> >> --- a/include/linux/platform_data/cros_ec_proto.h
> >> +++ b/include/linux/platform_data/cros_ec_proto.h
> >> @@ -229,6 +229,10 @@ u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
> >>
> >> int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
> >>
> >> +struct cros_ec_command *cros_ec_alloc_lightbar_cmd_msg(struct cros_ec_dev *ec);
> >> +int cros_ec_get_lightbar_version(struct cros_ec_dev *ec, uint32_t *ver_ptr,
> >> + uint32_t *flg_ptr);
> >> +
> >> int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
> >>
> >> /**
> >>
> >> base-commit: 1e7913ff5f9f1b73146ad8522958bd266f22a510
> >> --
> >> Sent by a computer, using git, on the internet
> >>
Powered by blists - more mailing lists