Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760103Ab0FKDSR (ORCPT ); Thu, 10 Jun 2010 23:18:17 -0400 Received: from sm-d311v.smileserver.ne.jp ([203.211.202.206]:35385 "EHLO sm-d311v.smileserver.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752902Ab0FKDSP (ORCPT ); Thu, 10 Jun 2010 23:18:15 -0400 Message-ID: <001a01cb0914$ba4ee680$66f8800a@maildom.okisemi.com> From: "Masayuki Ohtake" To: "Arnd Bergmann" Cc: "Alan Cox" , "LKML" , , "Wang, Qi" , "Wang, Yong Y" References: <000501cae2eb$de97e950$66f8800a@maildom.okisemi.com> <201004231657.57466.arnd@arndb.de> <000901cb07ad$234e6cf0$66f8800a@maildom.okisemi.com> <201006091516.13246.arnd@arndb.de> Subject: Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2] Date: Fri, 11 Jun 2010 12:18:10 +0900 X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 6.00.2800.1983 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2800.1983 X-Hosting-Pf: 0 X-NAI-Spam-Score: 1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7542 Lines: 215 Hi Arnd Thank you for your comment. Please refer the following mail in line. ----- Original Message ----- From: "Arnd Bergmann" To: "Masayuki Ohtake" Cc: "Wang, Yong Y" ; "Wang, Qi" ; ; "LKML" ; "Alan Cox" Sent: Wednesday, June 09, 2010 10:16 PM Subject: Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2] > On Wednesday 09 June 2010, Masayuki Ohtake wrote: > > > We have added our comment for your email in line. > > ok, thanks for the update > > > If you have any question, let me know. > > One small comment from me, plus more background on the ioctl: > > > > The definition of struct pch_phub_reqt is problematic because > > > it contains members of type 'unsigned long'. This means that > > > a 32 bit user process uses a different data structure than a > > > 64 bit kernel. > > > > > > Ideally, you only pass a single integer as an ioctl argument. > > > There are cases where it needs to be a data structure, but > > > if that happens, you should only use members types like __u32 > > > or __u64, not long or pointer. > > We have defined as u32 type. > > Note that public data headers should use '__u32' instead of 'u32', > because the 'u32' type is not available in the exported version > of linux/types.h. We will replace like below. u32 --> __u32 s32 --> __s32 s8 --> __s8 > > > > My feeling is that this ioctl interface is too > > > low-level in general. You only export access to specific > > > registers, not to functionality exposed by them. > > > The best kernel interfaces are defined in a way that > > > is independent of the underlying hardware and > > > convert generic commands into device specific commands. > > > > > > If you really want to allow direct register access, > > > a simpler way would be to map the memory into the user > > > address space using the mmap() operation and not > > > provide any ioctl commands. > > Packet Hub has function accessing many resisters. > > Thus, we think current spec is better. > > Getting the interface right is by far the most important > part in a driver submission, and often the hardest part. > > Giving register-level hardware access to random user space > applications for the PHUB essentially means that you > are forcing the hardware design to become stable for all > eternity because all tools that use this interface (whether > written by you or by other people) need to keep working on > future generations of the hardware as well. > > This is generally considered a very bad idea, so I think > you should drop the IOCTL_PHUB_{READ,WRITE,MODIFY}_REG > ioctl commands for now if you want to get your driver > merged quickly, and then we can find a way to do what > it's meant for in a better way. > > Regarding the other ioctl commands, I still have a few > comments that I did not mention at first: > > >+int pch_phub_ioctl(struct inode *inode, struct file *file, > >+ unsigned int cmd, unsigned long arg) > >+{ > >+ int ret_value = 0; > >+ struct pch_phub_reqt *p_pch_phub_reqt; > >+ unsigned long addr_offset; > >+ unsigned long data; > >+ unsigned long mask; > >+ > >+ if (pch_phub_suspended == true) { > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : " > >+ "suspend initiated returning =%d\n", -EPERM); > >+ ret_value = -EPERM; > >+ goto return_ioctrl; > >+ } > >+ > >+ p_pch_phub_reqt = (struct pch_phub_reqt *)arg; > >+ ret_value = copy_from_user((void *)&addr_offset, > >+ (void *)&p_pch_phub_reqt->addr_offset, > >+ sizeof(addr_offset)); > >+ if (ret_value) { > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : " > >+ "copy_from_user fail returning =%d\n", -EFAULT); > >+ ret_value = -EFAULT; > >+ goto return_ioctrl; > >+ } > > The pch_phub_reqt data structure does not really make sense for > commands other than IOCTL_PHUB_READ_MODIFY_WRITE_REG and causes > more problems, see below. We will delete almost ioctl functions. Phub ioctl has only MAC Address access. (OROM access will be moved to read/write method.) And we will change ioctl I/F like below. Usermode application doesn't only set HW register offset value but also set function. > > Moreover, the type casts are incorrect because they are > missing the __user address space modifier. The casts can > simply be removed, and the variable declared as > e.g. 'struct pch_phub_reqt __user *p_pch_phub_reqt;'. We will add '__user' > > I'd recommend using the 'sparse' tool to check your > source code to find problems like this. Simply install > sparse and run > > make C=1 drivers/char/pch_phub/ > > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : " > >+ "copy_from_user returns =%d\n", ret_value); We will use 'sparse' tool . > > The 'dev_dbg' variable does not seem to get initialized anywhere > in the code. In a clean driver, it should not be a global variable > anyway but refer to the object that you are operating on, e.g. > the pci device that has been opened. We will modify 'dev_dbg' is passed as a function parameter. > > >+ switch (cmd) { > >+ case IOCTL_PHUB_READ_REG: > > >+ case IOCTL_PHUB_WRITE_REG: > > >+ case IOCTL_PHUB_READ_MODIFY_WRITE_REG: > > > As mentioned, I'd just remove these commands for now. When a specific > functionality is needed, you can always add another ioctl command > or find another way to do it. Yes, we will delete the above. > > >+ case IOCTL_PHUB_READ_OROM: > >+ ret_value = pch_phub_read_serial_rom(addr_offset, > >+ (unsigned char *)&data); > >+ if (ret_value) { > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : Invoked " > >+ "pch_phub_read_serial_rom =%d\n", -EFAULT); > >+ ret_value = -EFAULT; > >+ break; > >+ } else { > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : Invoked " > >+ "pch_phub_read_serial_rom successfully\n"); > >+ } > >+ ret_value = copy_to_user((void *)&p_pch_phub_reqt->data, > >+ (void *)&data, sizeof(data)); > >+ if (ret_value) { > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : " > >+ "copy_to_user fail returning =%d\n", -EFAULT); > >+ ret_value = -EFAULT; > >+ break; > >+ } > >+ break; > > This is a very indirect way to access a rom. I'd recommend doing > read and write file operations for this instead of going through the > ioctl. When you do that, you can simply do 'cat /dev/topcliff-phub > romdump' > to read the entire contents, or have other code access the contents > bytewise. If teh OROM contents are specific to the ethernet function, > you may also just implement the ethtool operations for it, as shown below. We will try implement as read/write method. > > >+ case IOCTL_PHUB_READ_MAC_ADDR: > >+ pch_phub_read_gbe_mac_addr(addr_offset, (unsigned char *)&data); > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : Invoked " > >+ "pch_phub_read_gbe_mac_addr successfully\n"); > >+ > >+ ret_value = copy_to_user((void *)&p_pch_phub_reqt->data, > >+ (void *)&data, sizeof(data)); > >+ if (ret_value) { > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : copy_to_user fail " > >+ "returning =%d\n", -EFAULT); > >+ ret_value = -EFAULT; > >+ break; > >+ } > >+ break; > > I believe this really belongs into the ethernet driver, using the > ethtool_ops->get_eeprom operation as other ethernet drivers do the > same thing. MAC address is under Phub HW. To keep driver dependency, I think, Phub should take charge of MAC address access. Thanks, Ohtake -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/