| 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: Tue, 26 Apr 2011 12:23:38 +0100
From: Mark Brown <broonie@...nsource.wolfsonmicro.com>
To: Ashish Jangam <Ashish.Jangam@...tcummins.com>
Cc: "sameo@...nedhand.com" <sameo@...nedhand.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Dajun Chen <Dajun.Chen@...semi.com>
Subject: Re: [PATCHv1 -next] MFD: MFD module of DA9052 PMIC driver
On Tue, Apr 26, 2011 at 04:19:03PM +0530, Ashish Jangam wrote:
> +#define DA9052_SUBDEV(_name, _pdata, _pdata_sz, _res, _res_sz) \
> + { \
> + .name = "da9052-"#_name, \
> + .platform_data = _pdata, \
> + .data_size = _pdata_sz, \
> + .num_resources = _res_sz, \
> + .resources = _res, \
> + }
> +/* As the function does not use global variable and
You need more blank lines in this driver than you have.
> + da9052_reg_read() function has lock features an additional
> + lock is not provided to access 'da9052_adc_manual_read' function to
> + prevent concurrency issues */
This comment doesn't address the concurrency issue. The issue is that:
> + ret = da9052_reg_write(da9052, DA9052_ADC_MAN_REG,
> + mux_sel);
> + if (ret < 0)
> + return ret;
You write to select the input to read from here...
> +
> + do {
> + msleep(10);
> +
> + ret = da9052_reg_read(da9052, DA9052_ADC_MAN_REG);
> + if (ret < 0)
> + return ret;
...then wait for the result. If something else comes in and tries to do
another ADC reading simultaneously you're in trouble.
> +static int da9052_add_subdevs(struct da9052 *da9052)
> +{
> + struct da9052_pdata *pdata = da9052->dev->platform_data;
> + int ret = 0;
> +
> + struct mfd_cell da9052_subdev_info[] = {
> + DA9052_SUBDEV(onkey, NULL, 0, &da9052_onkey_resource, 1),
> + DA9052_SUBDEV(rtc, NULL, 0, &da9052_rtc_resource, 1),
> + DA9052_SUBDEV(gpio, NULL, 0, NULL, 0),
> + DA9052_SUBDEV(hwmon, NULL, 0, NULL, 0),
> + DA9052_SUBDEV(leds, pdata->pled,
> + sizeof(struct led_platform_data),
> + NULL, 0),
> + DA9052_SUBDEV(WLED1, NULL, 0, NULL, 0),
> + DA9052_SUBDEV(WLED2, NULL, 0, NULL, 0),
> + DA9052_SUBDEV(WLED3, NULL, 0, NULL, 0),
> + DA9052_SUBDEV(tsi, pdata->ptsi,
> + sizeof(struct da9052_tsi_platform_data),
> + da9052_tsi_resources,
> + ARRAY_SIZE(da9052_tsi_resources)),
> + DA9052_SUBDEV(bat, NULL, 0, da9052_power_resources,
> + ARRAY_SIZE(da9052_power_resources)),
> + DA9052_SUBDEV(watchdog, pdata->pwdt,
> + sizeof(struct da9052_wdt_platform_data), NULL, 0),
That's a large array to be allocating on the stack, and it's not obvious
why it isn't just static initdata anyway?
> +static struct i2c_device_id da9052_i2c_id[] = {
> + { "da9052_i2c"},
> +};
As previously pointed out this would generally just be "da9052", the
fact that it's an I2C device is obvious from the fact that this is an
i2c_device_id being used by an i2c_driver.
> +MODULE_AUTHOR("Dialog Semiconductor Ltd <dchen@...semi.com>");
> +MODULE_DESCRIPTION("I2C driver for Dialog DA9052 PMIC");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:da9052_i2c");
This isn't a platform device.
> +int da9052_read_events(struct da9052 *da9052, unsigned char reg ,
> + unsigned int *events)
> +{
> + uint8_t v[4] = {0, 0, 0, 0};
> + int ret;
> +
> + ret = da9052_group_read(da9052, reg, 4, v);
> + if (ret < 0)
> + return ret;
> +
> + *events = (v[3] << 24) | (v[2] << 16) | (v[1] << 8) | v[0];
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(da9052_read_events);
This looks *incredibly* suspicious, why would anything outside the
interrupt code be looking at this? There's also the fact that it's
separate to the set and clear functions in the main driver.
> +config PMIC_DA9052
> + tristate "Support Dialog DA9052 PMIC"
> + select MFD_CORE
> + help
> + Say yes here to support for Dialog semiconductor Da9052, Power
> + Management IC. This option enables the SSC communication type
> + needed to communicate with DA9052 PMIC.
> +choice
> + prompt "DA9052 SSC support"
> + depends on PMIC_DA9052
> +config MFD_DA9052_SPI
> + bool "SPI"
> + depends on SPI_MASTER=y
> + help
> + Say Y to select SPI serial interface for DA9052 chip
> +config MFD_DA9052_I2C
> + bool "I2C"
> + depends on I2C=y
> + help
> + Say Y to select I2C serial interface for DA9052 chip
> +endchoice
> +
This will prevent users building a kernel that supports both I2C and SPI
simultaneously. You also want some blank lines in here.
> +
> +ifeq ($(CONFIG_MFD_DA9052_SPI),y)
> +da9052-objs := da9052-spi.o da9052-core.o da9052-irq.o
> +obj-$(CONFIG_PMIC_DA9052) += da9052.o
> +endif
> +ifeq ($(CONFIG_MFD_DA9052_I2C),y)
> +da9052-objs := da9052-i2c.o da9052-core.o da9052-irq.o
> +obj-$(CONFIG_PMIC_DA9052) += da9052.o
> +endif
> +
Why combine these into a single module?
> +struct da9052_pdata {
> + struct led_platform_data *pled;
> + struct da9052_wdt_platform_data *pwdt;
> + struct da9052_tsi_platform_data *ptsi;
> + int (*init) (struct da9052 *da9052);
> + int irq;
> + int irq_base;
> + int num_subdevs;
Why is num_subdevs in the platform data? That looks wrong...
--
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