| 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: <161159998973.76967.1213998704222248070@swboyd.mtv.corp.google.com>
Date: Mon, 25 Jan 2021 10:39:49 -0800
From: Stephen Boyd <swboyd@...omium.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>,
Douglas Anderson <dianders@...omium.org>,
Gwendal Grignou <gwendal@...omium.org>
Subject: Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver
Quoting Jonathan Cameron (2021-01-24 09:38:20)
> On Fri, 22 Jan 2021 14:54:43 -0800
> Stephen Boyd <swboyd@...omium.org> wrote:
>
> > Add support for a ChromeOS EC proximity driver that exposes a "front"
> > proximity sensor via the IIO subsystem. The EC decides when front
> > proximity is near and sets an MKBP switch 'EC_MKBP_FRONT_PROXIMITY' to
> > notify the kernel of proximity. Similarly, when proximity detects
> > something far away it sets the switch bit to 0. For now this driver
> > exposes a single sensor, but it could be expanded in the future via more
> > MKBP bits if desired.
> >
> > Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>
> > Cc: Benson Leung <bleung@...omium.org>
> > Cc: Guenter Roeck <groeck@...omium.org>
> > Cc: Douglas Anderson <dianders@...omium.org>
> > Cc: Gwendal Grignou <gwendal@...omium.org>
> > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
>
> Hi Stephen,
>
> Looks more or less fine to me. My main concern is potential confusion
> in naming with the cros_ec_prox_light driver that we already have.
>
> A few other minor bits and bobs inline.
>
Cool thanks.
> > diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> > index 9c1aca1a8b79..b1330dd8e212 100644
> > --- a/drivers/iio/proximity/Makefile
> > +++ b/drivers/iio/proximity/Makefile
> > @@ -5,6 +5,7 @@
> >
> > # When adding new entries keep the list in alphabetical order
> > obj-$(CONFIG_AS3935) += as3935.o
> > +obj-$(CONFIG_CROS_EC_PROXIMITY) += cros_ec_proximity.o
> > obj-$(CONFIG_ISL29501) += isl29501.o
> > obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
> > obj-$(CONFIG_MB1232) += mb1232.o
> > diff --git a/drivers/iio/proximity/cros_ec_proximity.c b/drivers/iio/proximity/cros_ec_proximity.c
> > new file mode 100644
> > index 000000000000..a3aef911e3cc
> > --- /dev/null
> > +++ b/drivers/iio/proximity/cros_ec_proximity.c
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for cros-ec proximity sensor exposed through MKBP switch
> > + *
> > + * Copyright 2021 Google LLC.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/kernel.h>
>
> Slight preference for alphabetical order though keeping specific
> sections separate as done here is fine.
Will fix.
>
> > +#include <linux/notifier.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_ec_proto.h>
> > +
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#include <asm/unaligned.h>
> > +
> > +struct cros_ec_proximity_data {
> > + struct cros_ec_device *ec;
> > + struct iio_dev *indio_dev;
> > + struct mutex lock;
> > + struct notifier_block notifier;
> > + bool enabled;
> > +};
> > +
> > +static const struct iio_event_spec cros_ec_prox_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_EITHER,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > +static const struct iio_chan_spec cros_ec_prox_chan_spec[] = {
> > + {
> > + .type = IIO_PROXIMITY,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > + .event_spec = cros_ec_prox_events,
> > + .num_event_specs = ARRAY_SIZE(cros_ec_prox_events),
> > + },
> > +};
> > +
> > +static int cros_ec_proximity_parse_state(const void *data)
> > +{
> > + u32 switches = get_unaligned_le32(data);
> > +
> > + return !!(switches & BIT(EC_MKBP_FRONT_PROXIMITY));
> > +}
> > +
> > +static int cros_ec_proximity_query(struct cros_ec_device *ec_dev, int *state)
> > +{
> > + struct ec_params_mkbp_info *params;
> > + struct cros_ec_command *msg;
> > + int ret;
> > +
> > + msg = kzalloc(sizeof(*msg) + max(sizeof(u32), sizeof(*params)),
> > + GFP_KERNEL);
>
> Given this is known at build time, perhaps better to add it to the
> iio_priv() accessed structure and avoid having to handle allocations
> separately.
Ok.
>
> > + if (!msg)
> > + return -ENOMEM;
[...]
> > +static int cros_ec_proximity_read_raw(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan, int *val,
> > + int *val2, long mask)
> > +{
> > + struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > + struct cros_ec_device *ec = data->ec;
> > + int ret;
> > +
> > + if (chan->type != IIO_PROXIMITY)
> > + return -EINVAL;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + ret = iio_device_claim_direct_mode(indio_dev);
>
> Normally we only introduce these protections when adding the ability
> to change the state from direct to buffered (which these prevent).
> This driver doesn't yet support any other modes so I don't think
> there is any benefit in having these.
>
> If the aim is more local protection then should use a local lock
> as the semantics fo these functions might change in future.
Alright I'll drop it.
>
>
> > + if (ret)
> > + return ret;
> > +
> > + ret = cros_ec_proximity_query(ec, val);
> > + iio_device_release_direct_mode(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + return IIO_VAL_INT;
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int cros_ec_proximity_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +
> > + return data->enabled;
> > +}
> > +
> > +static int cros_ec_proximity_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir, int state)
> > +{
> > + struct cros_ec_proximity_data *data = iio_priv(indio_dev);
> > +
> > + mutex_lock(&data->lock);
> > + data->enabled = state;
> > + mutex_unlock(&data->lock);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct iio_info cros_ec_proximity_info = {
> > + .read_raw = cros_ec_proximity_read_raw,
> > + .read_event_config = cros_ec_proximity_read_event_config,
> > + .write_event_config = cros_ec_proximity_write_event_config,
> > +};
> > +
> > +static int cros_ec_proximity_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cros_ec_device *ec = dev_get_drvdata(dev->parent);
> > + struct iio_dev *indio_dev;
> > + struct cros_ec_proximity_data *data;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + data->ec = ec;
> > + data->indio_dev = indio_dev;
> > + mutex_init(&data->lock);
> > + platform_set_drvdata(pdev, data);
> > +
> > + indio_dev->name = "cros_ec_proximity";
> > + indio_dev->dev.parent = dev;
> > + indio_dev->info = &cros_ec_proximity_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->channels = cros_ec_prox_chan_spec;
> > + indio_dev->num_channels = ARRAY_SIZE(cros_ec_prox_chan_spec);
> > +
> > + ret = devm_iio_device_register(dev, indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + data->notifier.notifier_call = cros_ec_proximity_notify;
> > + ret = blocking_notifier_chain_register(&ec->event_notifier,
> > + &data->notifier);
> > + if (ret)
> > + dev_err(dev, "cannot register notifier: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int cros_ec_proximity_remove(struct platform_device *pdev)
> > +{
> > + struct cros_ec_proximity_data *data = platform_get_drvdata(pdev);
> > + struct cros_ec_device *ec = data->ec;
> > +
> > + blocking_notifier_chain_unregister(&ec->event_notifier,
> > + &data->notifier);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
>
> As a general rule, we are trying to clear out protections on CONFIG_OF etc
> and use of of_match_ptr() on the basis they don't really gain us anything
> and prevent use of some other firmware types. Here I guess you know what
> your firmware looks like, but I'm still keen to drop it in the interests
> of there being fewer places to copy it from.
>
> It may be a good idea to give this a more specific name as well given
> we already have cros-ec-prox as a platform device id from
> the cros_ec_light_prox driver.
Alright. I renamed it to cros_ec_mkbp_proximity throughout this driver.
I'm concerned about dropping CONFIG_OF because of_match_ptr() and
CONFIG_OF=n makes it unused but I suppose that will be OK as long as
compilation passes.
>
>
> > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > + { .compatible = "google,cros-ec-proximity" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > +#endif
> > +
> > +static struct platform_driver cros_ec_proximity_driver = {
> > + .driver = {
> > + .name = "cros-ec-proximity",
> > + .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> > + },
> > + .probe = cros_ec_proximity_probe,
> > + .remove = cros_ec_proximity_remove,
> > +};
> > +module_platform_driver(cros_ec_proximity_driver);
> > +
Powered by blists - more mailing lists