| 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 16:24:17 -0700
From: "Kuppuswamy, Sathyanarayanan" <sathyaosid@...il.com>
To: Peter Rosin <peda@...ntia.se>,
sathyanarayanan.kuppuswamy@...ux.intel.com
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] mux: Add new API to get mux_control ref by device
name.
Hi Peter,
On 7/8/2017 2:12 PM, Peter Rosin wrote:
> 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:
I am not planning to merge the Intel USB MUX driver any more. I agree
with Hans comments
and decided not to proceed further on this approach.
But I created these helper functions to get my driver working with MUX
framework. Since these
helper functions can be useful for any non-dt drivers who wants to use
MUX framework, I thought
to submit these changes for review.
>
> 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, "."));
will fix it in next version.
>
>> +}
>> +
>> +/**
>> + * 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.
Good catch. I did not test the case with multiple chips. So I failed to
notice this.
>
> 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.
Got it.
>
> Cheers,
> peda
>
>> #endif /* _LINUX_MUX_CONSUMER_H */
>>
Powered by blists - more mailing lists