| 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, 5 Sep 2015 16:47:21 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Chee Nouk Phoo <cnphoon@...era.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
cnphoon.linux@...il.com
Subject: Re: [[PATCH v2] 2/2] Altera Modular ADC driver support
On 01/09/15 22:49, Chee Nouk Phoo wrote:
> From: Chee Nouk Phoon <cnphoon@...era.com>
>
> Altera Modular ADC is soft IP that wraps the hardened ADC block in a Max10
> device. It can be configured to dual ADC mode that supports two channel
> synchronous sampling or independent single ADCs. ADC sampled values will be
> written into memory slots in sequence determined by a user configurable
> sequencer block.
>
> This patch adds Altera Modular ADC driver support
>
> Signed-off-by: Chee Nouk Phoon <cnphoon@...era.com>
Sorry, I wrote a review of version 1 but for some reason seem not have
sent it. Anyhow, will comment on this version now.
I'm feeling rather guilty about this as there are a number of elements
in this driver that are major issues and I meant to get back to you on
them over a week ago! Oops.
Anyhow, big ones in here are unusual uses of the ABI. One point to note
is that if you ever add any ABI not documented in Documentation/ABI/testing/*
then you MUST document it as part of your patch. It saves a lot
of time whilst the reviewer figures out what the ABI being presented
actually is.
Dual ADCs should not have their two elements passed out through a single
sysfs attribute. If you need to maintain their relationship it must
be done by using the buffered interface not the sysfs one. Doing it
any other way leads to inconsistent and confusing userspace ABI which
would be a disaster.
Extended naming (tied up with the above) has very limited use cases and
must be documented. It confuses userspace applications by encoding
information in a freeform way within the attribute name. As such the
form must be clearly documented and justified. The use case here is
not justified.
Anyhow, whilst these sound major, the driver is actually in pretty good
shape and the other stuff is minor tidy ups / formatting suggestions.
Thanks,
Jonathan
> ---
> drivers/iio/adc/Kconfig | 11 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/alt_modular_adc.c | 322 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 334 insertions(+), 0 deletions(-)
> mode change 100644 => 100755 drivers/iio/adc/Kconfig
> create mode 100755 drivers/iio/adc/alt_modular_adc.c
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> old mode 100644
> new mode 100755
> index 50c103d..e1a67cb
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -118,6 +118,17 @@ config AD799X
> To compile this driver as a module, choose M here: the module will be
> called ad799x.
>
> +config ALT_MODULAR_ADC
> + tristate "Altera Modular ADC driver"
> + select ANON_INODES
> +
> + help
> + Say yes here to have support for Altera Modular ADC. The driver
> + supports both Altera Modular ADC and Altera Modular Dual ADC.
> +
> + The driver can also be build as a module. If so the module will be
> + called alt_modular_adc.
> +
> config AT91_ADC
> tristate "Atmel AT91 ADC"
> depends on ARCH_AT91
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a096210..d7f10e0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_VF610_ADC) += vf610_adc.o
> obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> xilinx-xadc-y := xilinx-xadc-core.o xilinx-xadc-events.o
> obj-$(CONFIG_XILINX_XADC) += xilinx-xadc.o
> +obj-$(CONFIG_ALT_MODULAR_ADC) += alt_modular_adc.o
> diff --git a/drivers/iio/adc/alt_modular_adc.c b/drivers/iio/adc/alt_modular_adc.c
> new file mode 100755
> index 0000000..3d2d870
> --- /dev/null
> +++ b/drivers/iio/adc/alt_modular_adc.c
> @@ -0,0 +1,322 @@
> +/*
> + * Copyright (C) 2015 Altera Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Constant Definitions */
Please prefix all constants iwth ALT_MODULAR_ADC_ or similar
to avoid potential name clashes in the future.
> +#define MAX_SLOT 64
> +#define MAX_ADC 2
> +#define MAX_CHANNEL 18
> +#define MODE_SINGLE_ADC 1
> +#define MODE_DUAL_ADC 2
> +#define ADC_BITS 12
> +#define ADC_STORE_BITS 16
> +#define ADC_MAX_STR_SIZE 20
> +
> +/* Register Definitions */
> +#define ADC_CMD_REG 0x0
> +#define ADC_IER_REG 0x100
> +#define ADC_ISR_REG 0x104
> +
> +#define ADC_RUN_MSK 0x1
> +#define ADC_SINGLE_MKS 0x2
> +#define ADC_STOP_MSK 0x0
> +#define ADC_LOW_MSK 0xFFF
> +#define ADC_HIGH_MSK 0xFFF0000
> +
I'd like to see kernel-doc description fo this structure.
It's not immediately obvious what all the parts are!
> +struct altera_adc {
> + void __iomem *seq_regs;
> + void __iomem *sample_regs;
> +
> + unsigned int mode;
> + unsigned int slot_count;
> + unsigned int slot_sequence[MAX_ADC][MAX_SLOT];
Without looking at the bindings doc, it's not obvious what adc_number.
I'd prefer enough local docs to make that clear!
> + unsigned int adc_number;
> +};
> +
> +static int alt_modular_adc_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long mask)
> +{
> + struct altera_adc *adc = iio_priv(indio_dev);
> + int ret = -EINVAL;
> + u32 value;
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + value = readl_relaxed(adc->sample_regs + (chan->address * 4));
> +
> + if (adc->mode == MODE_SINGLE_ADC) {
> + *val = (value & ADC_LOW_MSK);
> + ret = IIO_VAL_INT;
> + } else if (adc->mode == MODE_DUAL_ADC) {
> + *val = (value & ADC_LOW_MSK);
> + *val2 = ((value & ADC_HIGH_MSK) >> 16);
No. This is not part of the ABI and is absolutely not the intent.
You can't pass two separate ADC readings from different channels through
the sysfs interface like this. It's been requested a number of times
but the write way to do this is to implement the buffered interface
and push the out that way. One sysfs attribute, one value is the rule!
The exceptions are for element that have no meaning whatsoever on their own
(such as quaternions) where any given element has no scale without the rest.
That is not true here.
> + ret = IIO_VAL_INT_MULTIPLE;
> + }
> +
> + return ret;
> +}
> +
> +static const struct iio_info adc_iio_info = {
> + .read_raw = &alt_modular_adc_read_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int alt_modular_adc_channel_init(struct iio_dev *indio_dev)
> +{
> + struct altera_adc *adc = iio_priv(indio_dev);
> + struct iio_chan_spec *chan_array;
> + struct iio_chan_spec *chan;
> + char **channel_name;
> + int i;
> +
> + chan_array = devm_kzalloc(&indio_dev->dev, (adc->slot_count *
> + sizeof(*chan_array)), GFP_KERNEL);
> + if (chan_array == NULL)
> + return -ENOMEM;
> +
> + channel_name = devm_kzalloc(&indio_dev->dev, adc->slot_count,
> + GFP_KERNEL);
> + if (channel_name == NULL)
> + return -ENOMEM;
> +
> + for (i = 0; i < adc->slot_count; i++) {
> + channel_name[i] = devm_kzalloc(&indio_dev->dev,
> + ADC_MAX_STR_SIZE, GFP_KERNEL);
No need to do this here, use devm_kasprintf as suggested below.
If not I think you could easily have combined this loop with the one
below simplifying the code.
> + if (channel_name == NULL)
> + return -ENOMEM;
> + }
> +
> + chan = chan_array;
> + for (i = 0; i < adc->slot_count; i++, chan++) {
> + chan->type = IIO_VOLTAGE;
> + chan->indexed = 1;
> + chan->channel = i;
> + chan->address = i;
No need to set address if it is always being set to the same
value as channel. Channel is available everywhere address is so
just use that value.
> +
> + /* Construct iio sysfs name*/
> + if (adc->mode == MODE_SINGLE_ADC) {
Use kasprintf and you can skip the earlier allocation of channel_name[i]
plus allocate exactly what is needed.
> + snprintf(channel_name[i], ADC_MAX_STR_SIZE,
> + "adc%d-ch%d",
> + adc->adc_number,
> + adc->slot_sequence[0][i]);
> + } else if (adc->mode == MODE_DUAL_ADC) {
> + snprintf(channel_name[i], ADC_MAX_STR_SIZE,
> + "adc1-ch%d_adc2-ch%d",
> + adc->slot_sequence[0][i],
> + adc->slot_sequence[1][i]);
> + } else {
> + return -EINVAL;
> + }
> +
> + chan->datasheet_name = channel_name[i];
> + chan->extend_name = channel_name[i];
You are making use of extend_name in an unusual way.
1) This results in non standard ABI so should be documented under
Documentation/ABI/testing/sysfs-bus-iio*
2) The naming here makes me think you are abusing the abi and pushing two
channels through the same sysfs attribute. Don't do that. If you need
to clearly capture two channels together then implement the buffered
interface (chardev reading of 'scans' - groups of channels captured at
a similar time) rather than abusing the simpler sysfs route.
> + chan->scan_index = i;
You aren't providing buffered access yet so don't bother filling in
scan_index or scan_type fo rhte channels (they are unusued).
> + chan->scan_type.sign = 'u';
> + chan->scan_type.realbits = ADC_BITS;
> + chan->scan_type.storagebits = ADC_STORE_BITS;
> + chan->scan_type.endianness = IIO_CPU;
> + chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> + }
> +
> + indio_dev->channels = chan_array;
Might as well just use indio_dev->channels directly rather than
having the local variable.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id alt_modular_adc_match[] = {
> + { .compatible = "altr,modular-adc-1.0" },
> + { .compatible = "altr,modular-dual-adc-1.0" },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, alt_modular_adc_match);
> +
> +static int alt_modular_adc_parse_dt(struct iio_dev *indio_dev,
> + struct device *dev)
> +{
> + struct altera_adc *adc = iio_priv(indio_dev);
> + struct device_node *np = dev->of_node;
> + u32 value;
> + int ret, i, j;
> + char str[50];
> +
> + ret = of_property_read_u32(np, "altr,adc-mode", &value);
> + if (ret < 0)
> + return ret;
> + if (value == 0 || value > MAX_ADC) {
> + dev_err(dev, "Invalid ADC mode value");
> + return -EINVAL;
> + }
> + adc->mode = value;
> +
> + ret = of_property_read_u32(np, "altr,adc-slot-count", &value);
> + if (ret < 0)
> + return ret;
> + if (value == 0 || value > MAX_SLOT) {
> + dev_err(dev, "Invalid ADC slot count value");
> + return -EINVAL;
> + }
> + adc->slot_count = value;
> +
> + ret = of_property_read_u32(np, "altr,adc-number", &value);
> + if (ret < 0)
> + return ret;
> + if (value == 0 || value > MAX_ADC) {
> + dev_err(dev, "Invalid ADC number value");
> + return -EINVAL;
> + }
> + adc->adc_number = value;
> +
> + /* Device tree lookup for channels for each memory slots */
> + for (j = 0; j < adc->mode; j++) {
> + for (i = 0; i < adc->slot_count; i++) {
> + str[0] = '\0';
> +
> + snprintf(str, sizeof(str),
> + "altr,adc%d-slot-sequence-%d",
> + (j + 1), (i + 1));
> +
> + ret = of_property_read_u32(np, str, &value);
> + if (ret < 0)
> + return ret;
> + if (value > MAX_CHANNEL) {
> + dev_err(dev, "Invalid ADC channel value");
> + return -EINVAL;
> + }
> + adc->slot_sequence[j][i] = value;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int alt_modular_adc_probe(struct platform_device *pdev)
> +{
> + struct altera_adc *adc;
> + struct device_node *np = pdev->dev.of_node;
> + struct iio_dev *indio_dev;
> + struct resource *mem;
> + int ret;
> +
> + if (!np)
> + return -ENODEV;
> +
> + indio_dev = iio_device_alloc(sizeof(struct altera_adc));
Convention has become sizeof(*adc) as it makes the connection slightly
more explicit.
> + if (!indio_dev) {
> + dev_err(&pdev->dev, "failed allocating iio device\n");
> + return -ENOMEM;
> + }
> +
> + adc = iio_priv(indio_dev);
> +
> + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "sequencer_csr");
> + adc->seq_regs = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(adc->seq_regs)) {
> + ret = PTR_ERR(adc->seq_regs);
> + goto err_iio;
> + }
> +
> + mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "sample_store_csr");
> + adc->sample_regs = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(adc->sample_regs)) {
> + ret = PTR_ERR(adc->sample_regs);
> + goto err_iio;
> + }
> +
> + ret = alt_modular_adc_parse_dt(indio_dev, &pdev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to parse device tree\n");
> + goto err_iio;
> + }
> +
> + ret = alt_modular_adc_channel_init(indio_dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed initialize ADC channels\n");
> + goto err_iio;
> + }
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + indio_dev->name = dev_name(&pdev->dev);
> + indio_dev->dev.parent = &pdev->dev;
> + indio_dev->dev.of_node = pdev->dev.of_node;
> + indio_dev->info = &adc_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->num_channels = adc->slot_count;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret)
> + goto err_iio;
> +
> + /* Disable Interrupt */
> + writel_relaxed(0, (adc->sample_regs + ADC_IER_REG));
Given we don't seem to handle interrupts anywhere I'd expect the device
to have come up with this disabled, but if it doesn't, this definitely
belongs a lot earlier than here.
> +
> + /* Start Continuous Sampling */
> + writel_relaxed((ADC_RUN_MSK), (adc->seq_regs + ADC_CMD_REG));
I'm not sure it makes sense to do either of these last two steps
after the userspace interface is started by the iio_device_register
call.
> +
> + return 0;
> +
Only 1 blank line here please (pretty much always considered enough
in kernel coding style).
> +
> +err_iio:
> + iio_device_free(indio_dev);
> + return ret;
> +}
> +
> +static int alt_modular_adc_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct altera_adc *adc = iio_priv(indio_dev);
> +
If you don't unregister the iio_device first there is a clear
race condition with userspace requests hitting after you've
stopped the ADC but before the interfaces have been removed / marked
as going away by that unregister call.
> + /* Stop ADC */
> + writel((ADC_STOP_MSK), (adc->seq_regs + ADC_CMD_REG));
Still an excess of brackets. You don't need them around a lot
of these arguments. We can reasonably assume they don't contain
any malicious elements that could lead to it doing other than the
obvious.
writel(ADC_STOP_MSK, adc->seq_regs + ADC_CMD_REG);
will be fine (similar cases elsewhere in the driver).
> +
> + /* Unregister ADC */
> + iio_device_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver altr_modular_adc_driver = {
> + .probe = alt_modular_adc_probe,
> + .remove = alt_modular_adc_remove,
> + .driver = {
> + .name = "alt-modular-adc",
> + .owner = THIS_MODULE,
> + .of_match_table = alt_modular_adc_match,
> + },
> +};
> +
> +
> +module_platform_driver(altr_modular_adc_driver);
> +
> +MODULE_DESCRIPTION("Altera Modular ADC Driver");
> +MODULE_AUTHOR("Chee Nouk Phoon <cnphoon@...era.com>");
> +MODULE_LICENSE("GPL v2");
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists