Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there

From: Enric Balletbo i Serra
Date: Thu Sep 10 2020 - 15:24:27 EST

Next message: Alex Williamson: "Re: [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support"
Previous message: Florian Fainelli: "Re: [PATCH] PCI: Don't use Printk in raw_spinlocks"
In reply to: Guenter Roeck: "Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there"
Next in thread: Stephen Boyd: "Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 10/9/20 16:52, Guenter Roeck wrote:
> On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
> <enric.balletbo@xxxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 10/9/20 16:18, Guenter Roeck wrote:
>>> On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> Cc: Guenter Roeck <groeck@xxxxxxxxxxxx>
>>>> Cc: Lee Jones <lee.jones@xxxxxxxxxx>
>>>> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
>>>> ---
>>>>
>>>> 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")) {
>

That looks a better solution, indeed. And definetely I'd prefer use the check
features way.

Gwendal, can you confirm that the Pixel Chromebook (Link) is the _only_ one
affected? This one is the only that comes to my mind but I might miss others.

I think that Samus has this flag (I can double check) and this was discussed
with you (long, long time ago :-) )

Thanks,
Enric

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

Next message: Alex Williamson: "Re: [PATCH 3/3] Documentation: fpga: dfl: Add description for VFIO Mdev support"
Previous message: Florian Fainelli: "Re: [PATCH] PCI: Don't use Printk in raw_spinlocks"
In reply to: Guenter Roeck: "Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there"
Next in thread: Stephen Boyd: "Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]