| 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
| ||
|
Date: Sat, 8 Jul 2017 23:12:16 +0200
From: Peter Rosin <peda@...ntia.se>
To: sathyanarayanan.kuppuswamy@...ux.intel.com
Cc: linux-kernel@...r.kernel.org, sathyaosid@...il.com
Subject: Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device
name.
On 2017-07-08 00:03, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>
> Currently this driver only provides a single API, mux_control_get() to
> get mux_control reference based on mux_name, and also this API has tight
> dependency on device tree node. For devices, that does not use device
> tree, it makes it difficult to use this API. This patch adds new
> API to access mux_control reference based on device name, chip index and
> controller index value.
I assume this is for the Intel USB Multiplexer that you sent a driver for
a month or so ago? If so, you still have not answered these questions:
Is any other consumer in the charts at all? Can this existing consumer
ever make use of some other mux? If the answer to both those questions
are 'no', then I do not see much point in involving the mux subsystem at
all. The Broxton USB PHY driver could just as well write to the register
all by itself, no?
that I asked in https://lkml.org/lkml/2017/5/31/58
What is the point of that driver?
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> ---
> drivers/mux/mux-core.c | 114 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/mux/consumer.h | 6 ++-
> 2 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> index 90b8995..f8796b9 100644
> --- a/drivers/mux/mux-core.c
> +++ b/drivers/mux/mux-core.c
> @@ -422,6 +422,87 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
> return dev ? to_mux_chip(dev) : NULL;
> }
>
> +static int dev_parent_name_match(struct device *dev, const void *data)
> +{
> + const char *devname = dev_name(dev->parent);
> + unsigned int i;
> +
> + if (!devname || !data)
> + return 0;
> +
> + for (i = 0; i < strlen(devname); i++) {
> + if (devname[i] == '.')
> + break;
> + }
> +
> + return !strncmp(devname, data, i-1);
Ouch, strlen as a termination test is wasteful, you want to remove the loop
and do something like this
return !strncmp(devname, data, strcspn(devname, "."));
> +}
> +
> +/**
> + * mux_chip_get_by_index() - Get the mux-chip associated with give device.
> + * @devname: Name of the device which registered the mux-chip.
> + * @index: Index of the mux chip.
> + *
> + * Return: A pointer to the mux-chip, or an ERR_PTR with a negative errno.
> + */
> +static struct mux_chip *mux_chip_get_by_index(const char *devname, int index)
> +{
> + struct device *dev;
> + int found = -1;
> +
> + if (!devname)
> + return ERR_PTR(-EINVAL);
> +
> + do {
> + dev = class_find_device(&mux_class, NULL, devname,
> + dev_parent_name_match);
> +
> + if (dev != NULL)
> + found++;
> +
> + if (found >= index)
> + break;
> + } while (dev != NULL);
This loop is broken. class_find_device will always return the same device.
Also, if you fix the loop, why is the ordering stable and something to rely
on?
> +
> + if ((found == index) && dev)
> + return to_mux_chip(dev);
> +
> + return ERR_PTR(-ENODEV);
> +}
> +
> +/**
> + * mux_control_get_by_index() - Get the mux-control of given device based on
> + * device name, chip and control index.
> + * @devname: Name of the device which registered the mux-chip.
> + * @chip_index: Index of the mux chip.
> + * @ctrl_index: Index of the mux controller.
> + *
> + * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *mux_control_get_by_index(const char *devname,
> + unsigned int chip_index,
> + unsigned int ctrl_index)
> +{
> + struct mux_chip *mux_chip;
> +
> + mux_chip = mux_chip_get_by_index(devname, chip_index);
> +
> + if (IS_ERR(mux_chip))
> + return ERR_PTR(PTR_ERR(mux_chip));
> +
> + if (ctrl_index >= mux_chip->controllers) {
> + dev_err(&mux_chip->dev,
> + "Invalid controller index, maximum value is %d\n",
> + mux_chip->controllers);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + get_device(&mux_chip->dev);
> +
> + return &mux_chip->mux[ctrl_index];
> +}
> +EXPORT_SYMBOL_GPL(mux_control_get_by_index);
> +
> /**
> * mux_control_get() - Get the mux-control for a device.
> * @dev: The device that needs a mux-control.
> @@ -533,6 +614,39 @@ struct mux_control *devm_mux_control_get(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_mux_control_get);
>
> +/**
> + * devm_mux_control_get_by_index() - Get the mux-control for a device of given
> + * index value.
> + * @dev: The device that needs a mux-control.
> + * @devname: Name of the device which registered the mux-chip.
> + * @chip_index: Index of the mux chip.
> + * @ctrl_index: Index of the mux controller.
> + *
> + * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
> + */
> +struct mux_control *devm_mux_control_get_by_index(struct device *dev,
> + const char *devname, unsigned int chip_index,
> + unsigned int ctrl_index)
> +{
> + struct mux_control **ptr, *mux;
> +
> + ptr = devres_alloc(devm_mux_control_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + mux = mux_control_get_by_index(devname, chip_index, ctrl_index);
> + if (IS_ERR(mux)) {
> + devres_free(ptr);
> + return mux;
> + }
> +
> + *ptr = mux;
> + devres_add(dev, ptr);
> +
> + return mux;
> +}
> +EXPORT_SYMBOL_GPL(devm_mux_control_get_by_index);
> +
> /*
> * Using subsys_initcall instead of module_init here to try to ensure - for
> * the non-modular case - that the subsystem is initialized when mux consumers
> diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
> index 5577e1b..e02485b 100644
> --- a/include/linux/mux/consumer.h
> +++ b/include/linux/mux/consumer.h
> @@ -28,5 +28,9 @@ void mux_control_put(struct mux_control *mux);
>
> struct mux_control *devm_mux_control_get(struct device *dev,
> const char *mux_name);
> -
> +struct mux_control *mux_control_get_by_index(const char *devname,
> + unsigned int chip_index, unsigned int ctrl_index);
> +struct mux_control *devm_mux_control_get_by_index(struct device *dev,
> + const char *devname, unsigned int chip_index,
> + unsigned int ctrl_index);
I want an empty line here.
Cheers,
peda
> #endif /* _LINUX_MUX_CONSUMER_H */
>
Powered by blists - more mailing lists