| 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: <d120d5000710110627x625476ccl6802cf5e97aa8533@mail.gmail.com>
Date: Thu, 11 Oct 2007 09:27:55 -0400
From: "Dmitry Torokhov" <dmitry.torokhov@...il.com>
To: "Bryan Wu" <bryan.wu@...log.com>,
"Michael Hennerich" <michael.hennerich@...log.com>
Cc: linux-input@...ey.karlin.mff.cuni.cz,
linux-joystick@...ey.karlin.mff.cuni.cz,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
akpm@...ux-foundation.org
Subject: Re: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver
Hi Bryan, Michael,
On 10/11/07, Bryan Wu <bryan.wu@...log.com> wrote:
> +
> +static int gpio3 = 0;
No need to initialize.
> +
> +static int ad7877_read(struct device *dev, u16 reg)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct ser_req *req = kzalloc(sizeof *req, GFP_KERNEL);
How many reads can happen at once? Maybe allocate 1 ser_req per
touchcsreen when creating it?
> +
> + if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
> + /* compute touch pressure resistance using equation #2 */
> + Rt = z2;
> + Rt -= z1;
> + Rt *= x;
> + Rt *= ts->x_plate_ohms;
> + Rt /= z1;
> + Rt = (Rt + 2047) >> 12;
> + } else
> + Rt = 0;
> +
> + if (Rt) {
> + input_report_abs(input_dev, ABS_X, x);
> + input_report_abs(input_dev, ABS_Y, y);
> + sync = 1;
> + }
> +
> + if (sync) {
> + input_report_abs(input_dev, ABS_PRESSURE, Rt);
> + input_sync(input_dev);
> + }
Confused about the logic - you set sync only if Rt != 0 so why don't
fold second "if" into the first one?
> +/* Must be called with ts->lock held */
> +static void ad7877_disable(struct ad7877 *ts)
> +{
> + if (ts->disabled)
> + return;
> +
> + ts->disabled = 1;
> +
> + if (!ts->pending) {
> + ts->irq_disabled = 1;
> + disable_irq(ts->spi->irq);
> + } else {
> + /* the kthread will run at least once more, and
> + * leave everything in a clean state, IRQ disabled
> + */
> + while (ts->pending) {
> + spin_unlock_irq(&ts->lock);
> + msleep(1);
> + spin_lock_irq(&ts->lock);
> + }
> + }
> +
> + /* we know the chip's in lowpower mode since we always
> + * leave it that way after every request
> + */
> +
> +}
This looks scary. Can you try moving locking inside ad7877_enable and
ad7877_disable?
> +
> +static int __devinit ad7877_probe(struct spi_device *spi)
> +{
> + struct ad7877 *ts;
> + struct input_dev *input_dev;
> + struct ad7877_platform_data *pdata = spi->dev.platform_data;
> + struct spi_message *m;
> + int err;
> + u16 verify;
> +
> +
> + if (!spi->irq) {
> + dev_dbg(&spi->dev, "no IRQ?\n");
> + return -ENODEV;
> + }
> +
> +
> + if (!pdata) {
> + dev_dbg(&spi->dev, "no platform data?\n");
> + return -ENODEV;
> + }
> +
> +
> + /* don't exceed max specified SPI CLK frequency */
> + if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) {
> + dev_dbg(&spi->dev, "SPI CLK %d Hz?\n",spi->max_speed_hz);
> + return -EINVAL;
> + }
> +
> + ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
> + input_dev = input_allocate_device();
> + if (!ts || !input_dev) {
> + err = -ENOMEM;
> + goto err_free_mem;
> + }
> +
> +
> + dev_set_drvdata(&spi->dev, ts);
> + spi->dev.power.power_state = PMSG_ON;
> +
> + ts->spi = spi;
> + ts->input = input_dev;
> + ts->intr_flag = 0;
> + init_timer(&ts->timer);
> + ts->timer.data = (unsigned long) ts;
> + ts->timer.function = ad7877_timer;
> +
> + spin_lock_init(&ts->lock);
> +
> + ts->model = pdata->model ? : 7877;
> + ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
> + ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
> + ts->pressure_max = pdata->pressure_max ? : ~0;
> +
> +
> + ts->stopacq_polarity = pdata->stopacq_polarity;
> + ts->first_conversion_delay = pdata->first_conversion_delay;
> + ts->acquisition_time = pdata->acquisition_time;
> + ts->averaging = pdata->averaging;
> + ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
> +
> + snprintf(ts->phys, sizeof(ts->phys), "%s/input0", spi->dev.bus_id);
> +
> + input_dev->name = "AD7877 Touchscreen";
> + input_dev->phys = ts->phys;
> + input_dev->cdev.dev = &spi->dev;
Please use input_dev->dev.parent, i will kill 'cdev' soon.
> +
> + err = input_register_device(input_dev);
> + if (err)
> + goto err_remove_attr;
> +
> + ts->intr_flag = 0;
> +
> + ad7877_task = kthread_run(ad7877_thread, ts, "ad7877_ktsd");
> +
> + if (IS_ERR(ad7877_task)) {
> + printk(KERN_ERR "ts: Failed to start ad7877_task\n");
> + goto err_remove_attr;
You shoudl use input_unregister_device() once it was registered. So
you need something like
goto err_unregister_idev;
...
err_unregister_idev:
input_unregister_device(input_dev);
input-dve = NULL; /* so we don't try to free it later */
err_remove_attr:
...
> + }
> +
> + return 0;
> +
> + err_remove_attr:
> +
> + sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
> +
> + if(gpio3)
> + device_remove_file(&spi->dev, &dev_attr_gpio3);
> + else
> + device_remove_file(&spi->dev, &dev_attr_aux3);
> +
> +
> + free_irq(spi->irq, ts);
> + err_free_mem:
> + input_free_device(input_dev);
> + kfree(ts);
> + return err;
> +}
> +
> +static int __devexit ad7877_remove(struct spi_device *spi)
> +{
> + struct ad7877 *ts = dev_get_drvdata(&spi->dev);
> +
> + input_unregister_device(ts->input);
> +
> + ad7877_suspend(spi, PMSG_SUSPEND);
> +
> + kthread_stop(ad7877_task);
You don't want to unregister device before stopping
interrupts/kthread. Otherwise there is a chance you will try to use
just freed device to send event through.
The driver also contains some indenting damage, please re-check.
Thanks!
--
Dmitry
-
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