I2C driver of Topcliff PCH
in [Kernel]
|
Prev: [PATCH 16/25] crypto: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
Next: Strange read data corruption on ext4/LVM/md
From: Arnd Bergmann on 20 Jul 2010 05:40 On Tuesday 20 July 2010, Masayuki Ohtake wrote: > > > @@ -147,6 +148,11 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count, > > > if (tmp == NULL) > > > return -ENOMEM; > > > > > > + if (copy_from_user(tmp, buf, count)) { > > > + kfree(tmp); > > > + return -EFAULT; > > > + } > > > + > > > pr_debug("i2c-dev: i2c-%d reading %zu bytes.\n", > > > iminor(file->f_path.dentry->d_inode), count); > > > > > > A read function should not do copy_from_user, only copy_to_user. > > If you are worried about returning invalid data from kernel space, > > better use kzalloc instead of kmalloc to get the buffer. > > Our I2C HW has special mode. > To control the mode, our i2c driver has copy_from_user. That is a highly unusual interface and I would definitely not recommend doing this. Please use an ioctl operation for anything that has both input and output data. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Masayuki Ohtake on 20 Jul 2010 08:40 Hi Arnd, > If you pass IRQF_SHARED, you can register any number of handlers > for the same IRQ number using different dev pointers. I will modify like above. Thanks, Ohtake. ----- Original Message ----- From: "Arnd Bergmann" <arnd(a)arndb.de> To: "Masayuki Ohtake" <masa-korg(a)dsn.okisemi.com> Cc: "Jean Delvare (PC drivers, core)" <khali(a)linux-fr.org>; "Ben Dooks (embedded platforms)" <ben-linux(a)fluff.org>; <linux-i2c(a)vger.kernel.org>; "LKML" <linux-kernel(a)vger.kernel.org>; <qi.wang(a)intel.com>; "Wang, Yong Y" <yong.y.wang(a)intel.com>; <joel.clark(a)intel.com>; <andrew.chih.howe.khor(a)intel.com> Sent: Tuesday, July 20, 2010 6:27 PM Subject: Re: [PATCH] I2C driver of Topcliff PCH > On Tuesday 20 July 2010, Masayuki Ohtake wrote: > > > > + > > > > + dev_dbg(&pdev->dev, > > > > + "i2c_add_adapter returns %d for channel-%d\n", ret, i); > > > > + pch_init(&adap_info->pch_data[i]); > > > > + dev_dbg(&pdev->dev, "pch_init invoked successfully\n"); > > > > + } > > > > + > > > > + ret = request_irq(pdev->irq, &pch_handler, IRQF_SHARED, > > > > + MODULE_NAME, adap_info); > > > > > > Similarly, you would create a new channel data structure for each channel here > > > and register it separately, and then request the interrupt with that > > > data structure as the info. > > > > With I2c multi-cahnnel IOH, IRQ number is in common. > > Thus, I think our PCH I2C driver can't be implemented like above. > > If you pass IRQF_SHARED, you can register any number of handlers > for the same IRQ number using different dev pointers. > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Masayuki Ohtake on 20 Jul 2010 08:50
Hi Arnd, > That is a highly unusual interface and I would definitely not recommend doing > this. Please use an ioctl operation for anything that has both input and output > data. I will delete the special interface from our i2c driver. Thanks, Ohtake ----- Original Message ----- From: "Arnd Bergmann" <arnd(a)arndb.de> To: "Masayuki Ohtake" <masa-korg(a)dsn.okisemi.com> Cc: <andrew.chih.howe.khor(a)intel.com>; <joel.clark(a)intel.com>; "Wang, Yong Y" <yong.y.wang(a)intel.com>; <qi.wang(a)intel.com>; "LKML" <linux-kernel(a)vger.kernel.org>; <linux-i2c(a)vger.kernel.org>; "Ben Dooks (embedded platforms)" <ben-linux(a)fluff.org>; "Jean Delvare (PC drivers, core)" <khali(a)linux-fr.org> Sent: Tuesday, July 20, 2010 6:29 PM Subject: Re: [PATCH] I2C driver of Topcliff PCH > On Tuesday 20 July 2010, Masayuki Ohtake wrote: > > > > @@ -147,6 +148,11 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count, > > > > if (tmp == NULL) > > > > return -ENOMEM; > > > > > > > > + if (copy_from_user(tmp, buf, count)) { > > > > + kfree(tmp); > > > > + return -EFAULT; > > > > + } > > > > + > > > > pr_debug("i2c-dev: i2c-%d reading %zu bytes.\n", > > > > iminor(file->f_path.dentry->d_inode), count); > > > > > > > > > A read function should not do copy_from_user, only copy_to_user. > > > If you are worried about returning invalid data from kernel space, > > > better use kzalloc instead of kmalloc to get the buffer. > > > > Our I2C HW has special mode. > > To control the mode, our i2c driver has copy_from_user. > > That is a highly unusual interface and I would definitely not recommend doing > this. Please use an ioctl operation for anything that has both input and output > data. > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |