Topcliff PHUB: Add The Packet Hub driver [1/2]
Subsystems (2)
PATCH 1/1 Topcliff PHUB: Add The Packet Hub driver [1/2]
Quoted 4 lines · Masayuki Ohtake
From: Masayuki Ohtake <[email protected]>
This patch adds the Packet Hub driver for Topcliff. Patch created against 2.6.33.1
Thank you for your submission. Since this is the first time you posted this patch, there is naturally a lot that needs to be improved on before the driver can be merged. I'll go through this in detail so you can improve it for the next submission.
First of all, you need to decide where you want the code to go. The quickest path is to get it into drivers/staging, which can accept code that does not yet follow the kernel coding style or may still need changes to its user interface, see http://www.kroah.com/log/linux/linux-staging-update.html
If you want to go straight into the regular supported driver directory, that is also ok but requires more upfront work on coding style and addressing review comments like the ones I make below. Addressing the comments typically means that you either change your code as suggested or that you argue that your version is actually correct.
The patch description above needs to tell the reader what the driver actually does and what the hardware is for.
Quoted 12 lines
Signed-off-by Masayuki Ohtake <[email protected]>---
drivers/char/Kconfig | 7++ drivers/char/Makefile | 2 drivers/char/pch_phub/Makefile | 9 drivers/char/pch_phub/pch_common.h | 147 drivers/char/pch_phub/pch_debug.h | 58 drivers/char/pch_phub/pch_phub.c | 375 drivers/char/pch_phub/pch_phub.h | 195 drivers/char/pch_phub/pch_phub_hal.c | 544 drivers/char/pch_phub/pch_phub_hal.h | 124 drivers/char/pch_phub/pch_phub_pci.c | 499
Quoted 1 line
+++++++++++++++++++++++++++++++ 10 files changed, 1960 insertions(+)
You have submitted this driver to the netdev list, but put the code into drivers/char. If this is really a network driver, it should probably go into drivers/net, otherwise it needs to be reviewed on the main linux-kernel mailing list.
If you want it to be applied as a staging driver first, change the code location to drivers/staging first a
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 6 lines
diff -urN linux-2.6.33.1/drivers/char/Kconfig
topcliff-2.6.33.1/drivers/char/Kconfigdrivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +1
-0+0900@@ -4,6 +4,13 @@
Evidently, your email client breaks line-wrapping. This means that it's not possibly to apply the patch. Please see Documentation/email-clients.txt on how to fix this.
To make sure this does not happen again, you can send the patch to yourself and try to apply it from the email as a test before you send it to a mailing list.
Quoted 9 lines
menu "Character devices"
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +7
-0+config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH Packet Hub Host controller. +
Quoted 3 lines
config VT
bool "Virtual terminal" if EMBEDDED
depends on !S390
This description also should be more helpful. This could be the same text that you will put in the patch description above.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 5
Quoted 12 lines
diff -urN
linux-2.6.33.1/drivers/char/pch_phub/pch_common.htopcliff-2.6.33.1/drivers/char/pch_phub/pch_common.h
---linux-2.6.33.1/drivers/char/pch_phub/pch_common.h1970-01-01 09:00:00.000000000 +0900
+++topcliff-2.6.33.1/drivers/char/pch_phub/pch_common.h2010-04-14 15:29:48.000000000 +0900drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0@@ -0,0 +1,147 @@ +/*! + * @file pch_common.h + * @brief Provides the macro definitions used by all files. + * @version 1.0.0.0 + * @section
You seem to use a tool for processing the comments into documentation, which is good. However, the syntax you use is incompatible with the one used in the kernel and should be changed to the 'kerneldoc' format, see Documentation/kernel-doc-nano-HOWTO.txt.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 18
Quoted 13 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +13
-0+/*! @ingroup Global +@def PCH_WRITE8 +@brief Macro for writing 8 bit data to an io/mem address +*/ +#define PCH_WRITE8(val, addr) iowrite8((val), (void __iomem *)(addr)) +/*! @ingroup Global +@def PCH_LOG +@brief Macro for writing 16 bit data to an io/mem address +*/ +#define PCH_WRITE16(val, addr) iowrite16((val), (void __iomem *)(addr)) +/*! @ingroup Global +@def PCH_LOG +@brief Macro for writing 32 bit data to an io/mem address
In general, wrapping kernel functions in a driver specific macro that does not do anything else is discouraged. It's best to delete these macros and change the code to use the underlying interfaces directly.
Quoted 16 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +16
-0+#ifndef __PCH_DEBUG_H__ +#define __PCH_DEBUG_H__ + +#ifdef MODULE +#define PCH_LOG(level, fmt, args...) printk(level "%s:" fmt "\n",\ + THIS_MODULE->name, ##args) +#else +#define PCH_LOG(level, fmt, args...) printk(level "%s:" fmt "\n" ,\ + __FILE__, ##args) +#endif + + +#ifdef DEBUG + #define PCH_DEBUG(fmt, args...) PCH_LOG(KERN_DEBUG, fmt, ##args) +#else + #define PCH_DEBUG(fmt, args...)
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 44
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +10
-0+#endif + +#ifdef PCH_TRACE_ENABLED + #define PCH_TRACE PCH_DEBUG +#else + #define PCH_TRACE(fmt, args...) +#endif + +#define PCH_TRACE_ENTER PCH_TRACE("Enter %s", __func__) +#define PCH_TRACE_EXIT PCH_TRACE("Exit %s", __func__)
For these macros, we already have existing interfaces in the kernel, you should remove yours and use dev_dbg, dev_info, pr_debug etc.
The tracing functions can probably just be removed here. If you feel that you need tracing, please take a look at the kernel tracing subsystem. There is an excellent series of articles about tracing at http://lwn.net/Articles/383362/.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 53
Quoted 9 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +9
-0+/** + * file_operations structure initialization + */ +const struct file_operations pch_phub_fops = { + .owner = THIS_MODULE, + .open = pch_phub_open, + .release = pch_phub_release, + .ioctl = pch_phub_ioctl, +};
New code should use the 'unlocked_ioctl' callback instead of 'ioctl'.
The whitespace in this patch is damaged: indentation of code should be done with tabs instead of spaces. It's not clear if the code is written like this, or it was damaged by your email client.
If the problem is part of your actual code, have a look at Documentation/CodingStyle for how it should look like.
Quoted 9 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0+/*function implementations*/ + +/*! @ingroup PHUB_InterfaceLayerAPI + @fn int pch_phub_open( struct inode *inode,struct file *file) + @remarks Implements the Initializing and opening of the Packet Hubmodule.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +3
-0+ @param inode [@ref INOUT] Contains the reference of the inode + structure + @param file [@ref INOUT] Contains the reference of the file structure
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +10
-0+ @retval returnvalue [@ref OUT] contains the result for the concerned + attempt. + The result would generally comprise of success code + or failure code. The failure code will indicate reason for + failure. + @see + EBUSY + */ +int pch_phub_open(struct inode *inode, struct file *file) +{
Since this is not an exported interface, it the function should probably be marked 'static'.
Quoted 5 lines
+ int
ret_value=PCH_PHUB_SUCCESS;
+ structpch_phub_reqt*p_pch_phub_reqt;
+ unsigned longaddr_offset;
+ unsigned long data;
+ unsigned long mask;
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 9 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +9
-0+ + do { + if (pch_phub_suspended == true) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : " + "suspend initiated returning =%d\n", + PCH_PHUB_FAIL); + ret_value = PCH_PHUB_FAIL; + break; + }
Using the do { ... } while (0) construct in this way is not wrong, but unconventional. The way this is normally done in Linux is to have a goto target at the end.
The macros PCH_PHUB_SUCCESS and PCH_PHUB_FAIL should probably be removed, because they are not standard error codes. By convention, you should return 0 for success in ioctl or one of the errno.h values for failure, as you do below.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 12 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +12
-0+ 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) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : " + "copy_from_user fail returning =%d\n", + -EFAULT); + ret_value = -EFAULT; + break; + }
It is much easier to use get_user() here than copy_from_user.
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.
Quoted 31 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +31
-0+ switch (cmd) { + case IOCTL_PHUB_READ_REG: + { + + pch_phub_read_reg(addr_offset, &data); + PCH_DEBUG("pch_phub_ioctl : Invoked " + "pch_phub_read_reg successfully\n"); + + ret_value = + copy_to_user((void *)&p_pch_phub_reqt-> + data, (void *)&data, + sizeof(data)); + if (ret_value) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : " + "copy_to_user fail returning =%d\n", + -EFAULT); + ret_value = -EFAULT; + break; + } + break; + } + + case IOCTL_PHUB_WRITE_REG: + { + + ret_value = + copy_from_user((void *)&data, + (void *)&p_pch_phub_reqt-> + data, sizeof(data)); + if (ret_value) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : "
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +10
-0+ "copy_from_user fail returning =%d\n", + -EFAULT); + ret_value = -EFAULT; + break; + } + pch_phub_write_reg(addr_offset, data); + PCH_DEBUG("pch_phub_ioctl : Invoked " + "pch_phub_write_reg successfully\n"); + break; + }
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.
I don't see any range cheching on the addr_offset argument, which means that a malicious user can use this function to access not only your device, but any data in the kernel address space.
Note that your open count does not protect the hardware from concurrent access, because a file descriptor can be shared by multiple user threads. You can probably safely drop that count.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 5 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0+/*! @ingroup PHUB_InterfaceLayer + @def IOCTL_PHUB_READ_REG + @brief Outlines the read register function signature. + */ +#define IOCTL_PHUB_READ_REG (_IOW(PHUB_IOCTL_MAGIC, 1, unsigned long))
If I read your code correctly, you actually pass a struct pch_phub_reqt argument, not an unsigned long argument, so this definition should be changed accordingly. The same applies to the other ioctl commands.
Your patch continues in a second email, which is not how you normally split submissions. When there is a logical separation between parts of the driver, make multiple patches, each with a separate description of what the patch does.
In case of this driver, that does not seem necessary. In fact, it seems to me like it is simple enough to become a single source file, which would simplify it even more because you no longer need header files defining the interface between parts of the driver.
Arnd
On: Fri, 23 Apr 2010 16:57:57 +0200, Arnd Bergmann <[email protected]> wrote:
Hi Arnd
Thank you for your review.
We will confirm them and update the new patch.
Our company have a vacation.
Therefore we will update patch after May 6.
Best regards,
Masayuki Ohtake <[email protected]>
Hi Arnd
We have added our comment for your email in line. If you have any question, let me know.
After modifying , we will resubmit our phub pach.(Today or tommorow)
Thanks,
Ohtake.
----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: "NETDEV" <[email protected]>; "Wang, Yong Y" <[email protected]>; "Wang, Qi" <[email protected]>; <[email protected]>
Sent: Friday, April 23, 2010 11:57 PM
Subject: Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2]
Quoted 7 lines · Masayuki Ohtake
You have submitted this driver to the netdev list, but put the code into
drivers/char. If this is really a network driver, it should probably go intodrivers/net, otherwise it needs to be reviewed on the main linux-kernel mailing list.
PHUB is not network driver.
Quoted 3 lines
If you want it to be applied as a staging driver first, change the code location to
drivers/stagingfirst a
We don't want it to be applied as a staging driver first.
Quoted 11 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 7 lines
diff -urN linux-2.6.33.1/drivers/char/Kconfig
topcliff-2.6.33.1/drivers/char/Kconfigdrivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +1
-0+0900@@ -4,6 +4,13 @@Evidently, your email client breaks line-wrapping. This means that it's not possibly to apply the patch. Please see
Documentation/email-clients.txton how to fix this.
We used outlook express.
But now, we use thunderbird.
Quoted 7 lines
To make sure this does not happen again, you can send the patch to yourself and try to apply it from the email as a test before you send it to a mailing list.
Quoted 2 lines
menu "Character devices"
Quoted 13 lines
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +7
-0+config PCH_PHUB + tristate "PCH PHUB" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH Packet Hub Host controller. +config VT
bool "Virtual terminal" if EMBEDDED
depends on !S390This description also should be more helpful. This could be the same text that you will put in the patch description above.
We agree.
We will add in detail.
Quoted 10 lines
Quoted 10 lines
diff -urN
linux-2.6.33.1/drivers/char/pch_phub/pch_common.htopcliff-2.6.33.1/drivers/char/pch_phub/pch_common.h
---linux-2.6.33.1/drivers/char/pch_phub/pch_common.h1970-01-01 09:00:00.000000000 +0900
+++topcliff-2.6.33.1/drivers/char/pch_phub/pch_common.h2010-04-14 15:29:48.000000000 +0900drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +2
-0@@ -0,0 +1,147 @@ +/*! + * @file pch_common.h
Quoted 8 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 5
Quoted 3 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +3
-0+ * @brief Provides the macro definitions used by all files. + * @version 1.0.0.0 + * @sectionYou seem to use a tool for processing the comments into documentation, which is good. However, the syntax you use is incompatible with the one used in the kernel and should be changed to the 'kerneldoc' format, see
Documentation/kernel-doc-nano-HOWTO.txt.
Referring 'kernel-doc-nano-HOWTO.txt', we will modify.
Quoted 10 lines
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +9
-0+/*! @ingroup Global +@def PCH_WRITE8 +@brief Macro for writing 8 bit data to an io/mem address +*/ +#define PCH_WRITE8(val, addr) iowrite8((val), (void __iomem *)(addr)) +/*! @ingroup Global +@def PCH_LOG +@brief Macro for writing 16 bit data to an io/mem address +*/
Quoted 8 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 18
Quoted 4 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +4
-0+#define PCH_WRITE16(val, addr) iowrite16((val), (void __iomem *)(addr)) +/*! @ingroup Global +@def PCH_LOG +@brief Macro for writing 32 bit data to an io/mem addressIn general, wrapping kernel functions in a driver specific macro that does not do anything else is discouraged. It's best to delete these macros and change the code to use the underlying interfaces directly.
We have deleted unnecessary wrapping.
Quoted 22 lines
Quoted 22 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +21
-0+#ifndef __PCH_DEBUG_H__ +#define __PCH_DEBUG_H__ + +#ifdef MODULE +#define PCH_LOG(level, fmt, args...) printk(level "%s:" fmt "\n",\ + THIS_MODULE->name, ##args) +#else +#define PCH_LOG(level, fmt, args...) printk(level "%s:" fmt "\n" ,\ + __FILE__, ##args) +#endif + + +#ifdef DEBUG + #define PCH_DEBUG(fmt, args...) PCH_LOG(KERN_DEBUG, fmt, ##args) +#else + #define PCH_DEBUG(fmt, args...) +#endif + +#ifdef PCH_TRACE_ENABLED + #define PCH_TRACE PCH_DEBUG +#else
Quoted 8 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 44
Quoted 5 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0+ #define PCH_TRACE(fmt, args...) +#endif + +#define PCH_TRACE_ENTER PCH_TRACE("Enter %s", __func__) +#define PCH_TRACE_EXIT PCH_TRACE("Exit %s", __func__)For these macros, we already have existing interfaces in the kernel, you should remove yours and use
dev_dbg,dev_info,pr_debugetc.
We have replaced our original function to linux function.
Quoted 5 lines
The tracing functions can probably just be removed here. If you feel that you need tracing, please take a look at the kernel tracing subsystem. There is an excellent series of articles about tracing at http://lwn.net/Articles/383362/.
We have deleted tracing function.
Quoted 12 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 line 53
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +9
-0+/** + * file_operations structure initialization + */ +const struct file_operations pch_phub_fops = { + .owner = THIS_MODULE, + .open = pch_phub_open, + .release = pch_phub_release, + .ioctl = pch_phub_ioctl, +};New code should use the '
unlocked_ioctl' callback instead of 'ioctl'.
We agree.
Quoted 7 lines
The whitespace in this patch is damaged: indentation of code should be done with tabs instead of spaces. It's not clear if the code is written like this, or it was damaged by your email client.
If the problem is part of your actual code, have a look at
Documentation/CodingStylefor how it should look like.
We use thunderbird now.
Thus, we think our patch is not damaged.
Quoted 15 lines
Quoted 15 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0+/*function implementations*/ + +/*! @ingroup PHUB_InterfaceLayerAPI + @fn int pch_phub_open( struct inode *inode,struct file *file) + @remarks Implements the Initializing and opening of the Packet Hubmodule.
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +8
-0+ @param inode [@ref INOUT] Contains the reference of the inode + structure + @param file [@ref INOUT] Contains the reference of the file structure + @retval returnvalue [@ref OUT] contains the result for the concerned + attempt. + The result would generally comprise of success code + or failure code. The failure code will indicate reason for + failure.
Quoted 8 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 5 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0+ @see + EBUSY + */ +int pch_phub_open(struct inode *inode, struct file *file) +{Since this is not an exported interface, it the function should probably be marked 'static'.
We have modified definition as 'static'.
Quoted 16 lines
Quoted 6 lines
+ int
ret_value=PCH_PHUB_SUCCESS;
+ structpch_phub_reqt*p_pch_phub_reqt;
+ unsigned longaddr_offset;
+ unsigned long data;
+ unsigned long mask;drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 9 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +9
-0+ + do { + if (pch_phub_suspended == true) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : " + "suspend initiated returning =%d\n", + PCH_PHUB_FAIL); + ret_value = PCH_PHUB_FAIL; + break; + }
Quoted 3 lines
Using the do { ... } while (0) construct in this way is not wrong, but unconventional. The way this is normally done in Linux is to have a goto target at the end.
We have deleted unnecessary '{ ...}' and while(0).
Quoted 5 lines
The macros
PCH_PHUB_SUCCESSandPCH_PHUB_FAILshould probably be removed, because they are not standard error codes. By convention, you should return 0 for success in ioctl or one of theerrno.hvalues for failure, as you do below.
We have replaced our original returned value to linux's returned value.
Quoted 14 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 13 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +12
-0+ 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) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : " + "copy_from_user fail returning =%d\n", + -EFAULT); + ret_value = -EFAULT; + break; + }
Quoted 1 line
It is much easier to use
get_user()here thancopy_from_user.
We have replaced copy_from_user to get_user.
Quoted 10 lines
The definition of struct
pch_phub_reqtis 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
__u32or__u64, not long or pointer.
We have defined as u32 type.
Quoted 50 lines
Quoted 32 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +31
-0+ switch (cmd) { + case IOCTL_PHUB_READ_REG: + { + + pch_phub_read_reg(addr_offset, &data); + PCH_DEBUG("pch_phub_ioctl : Invoked " + "pch_phub_read_reg successfully\n"); + + ret_value = + copy_to_user((void *)&p_pch_phub_reqt-> + data, (void *)&data, + sizeof(data)); + if (ret_value) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : " + "copy_to_user fail returning =%d\n", + -EFAULT); + ret_value = -EFAULT; + break; + } + break; + } + + case IOCTL_PHUB_WRITE_REG: + { + + ret_value = + copy_from_user((void *)&data, + (void *)&p_pch_phub_reqt-> + data, sizeof(data)); + if (ret_value) { + PCH_LOG(KERN_ERR, "pch_phub_ioctl : "drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +10
-0+ "copy_from_user fail returning =%d\n", + -EFAULT); + ret_value = -EFAULT; + break; + } + pch_phub_write_reg(addr_offset, data); + PCH_DEBUG("pch_phub_ioctl : Invoked " + "pch_phub_write_reg successfully\n"); + break; + }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.
Quoted 4 lines
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.
Quoted 5 lines
I don't see any range cheching on the
addr_offsetargument, which means that a malicious user can use this function to access not only your device, but any data in the kernel address space.
We have implemented range limitaion code.
Quoted 5 lines
Note that your open count does not protect the hardware from concurrent access, because a file descriptor can be shared by multiple user threads. You can probably safely drop that count.
We will implement using mutex.
Quoted 10 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000
Quoted 6 lines
drivers/char/Kconfig 2010-04-14 18:19:10.000000000 renamed from drivers/char/Kconfig 2010-03-16 01:09:39.000000000 +0900 +5
-0+/*! @ingroup PHUB_InterfaceLayer + @def IOCTL_PHUB_READ_REG + @brief Outlines the read register function signature. + */ +#define IOCTL_PHUB_READ_REG (_IOW(PHUB_IOCTL_MAGIC, 1, unsigned long))If I read your code correctly, you actually pass a struct
pch_phub_reqtargument, not an unsigned long argument, so this definition should be changed accordingly. The same applies to the other ioctl commands.
I have modified as 'u32'.
Quoted 11 lines
Your patch continues in a second email, which is not how you normally split submissions. When there is a logical separation between parts of the driver, make multiple patches, each with a separate description of what the patch does.
In case of this driver, that does not seem necessary. In fact, it seems to me like it is simple enough to become a single source file, which would simplify it even more because you no longer need header files defining the interface between parts of the driver.
We agree.
Quoted 7 lines
Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 1 line · Masayuki Ohtake
We have added our comment for your email in line.
ok, thanks for the update
Quoted 1 line
If you have any question, let me know.
One small comment from me, plus more background on the ioctl:
Quoted 10 lines
Quoted 9 lines
The definition of struct
pch_phub_reqtis 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
__u32or__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.
Quoted 7 lines
Quoted 7 lines
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.
Quoted 6 lines
Quoted 4 lines
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:
Quoted 16 lines
(quoted diff) +16
-0+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; + } +
Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
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;'.
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/
Review
Quoted 2 lines
(quoted diff) +2
-0+ dev_dbg(dev_dbg, "pch_phub_ioctl : " + "copy_from_user returns =%d\n", ret_value);
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.
Review
Quoted 6 lines
(quoted diff) +4
-0+ 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.
Quoted 11 lines
(quoted diff) +11
-0+ 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");
Review
Quoted 10 lines
(quoted diff) +10
-0+ } + 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.
Quoted 4 lines
(quoted diff) +4
-0+ 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");
Review
Quoted 10 lines
(quoted diff) +10
-0+ + 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.
Arnd
Hi Arnd
Thank you for your comment.
Please refer the following mail in line.
----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: "Wang, Yong Y" <[email protected]>; "Wang, Qi" <[email protected]>; <[email protected]>; "LKML" <[email protected]>; "Alan Cox" <[email protected]>
Sent: Wednesday, June 09, 2010 10:16 PM
Subject: Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2]
Quoted 19 lines · Masayuki Ohtake
Quoted 2 lines
We have added our comment for your email in line.
ok, thanks for the update
Quoted 1 line
If you have any question, let me know.
One small comment from me, plus more background on the ioctl:
Quoted 9 lines
Quoted 9 lines
The definition of struct
pch_phub_reqtis 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
__u32or__u64, not long or pointer.
Quoted 5 lines
Quoted 1 line
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 oflinux/types.h.
We will replace like below.
u32 --> __u32
s32 --> __s32
s8 --> __s8
Quoted 56 lines
Quoted 8 lines
Quoted 8 lines
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.
Quoted 6 lines
Quoted 4 lines
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:
Quoted 22 lines
(quoted diff) +22
-0+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 : "
Quoted 8 lines
Quoted 4 lines
+ "
copy_from_userfail returning =%d\n",-EFAULT);
+ret_value=-EFAULT;
+ gotoreturn_ioctrl;
+ }The
pch_phub_reqtdata structure does not really make sense for commands other thanIOCTL_PHUB_READ_MODIFY_WRITE_REGand 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.
Quoted 5 lines
Moreover, the type casts are incorrect because they are missing the
__useraddress space modifier. The casts can simply be removed, and the variable declared as e.g. 'structpch_phub_reqt__user*p_pch_phub_reqt;'.
We will add '__user'
Quoted 9 lines
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/Quoted 2 lines
+ dev_dbg(dev_dbg, "
pch_phub_ioctl: "
+ "copy_from_userreturns =%d\n",ret_value);
We will use 'sparse' tool .
Quoted 5 lines
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.
Quoted 4 lines
Quoted 3 lines
+ switch (cmd) {
+ caseIOCTL_PHUB_READ_REG:
Quoted 8 lines
Quoted 3 lines
+ 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.
Quoted 23 lines
Quoted 14 lines
+ 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_romsuccessfully\n");
+ }
+ret_value= copy_to_user((void *)&p_pch_phub_reqt->data,Quoted 8 lines
+ (void *)&data, sizeof(data));
+ if (ret_value) {
+ dev_dbg(dev_dbg, "pch_phub_ioctl: "
+ "copy_to_userfail returning =%d\n",-EFAULT);
+ret_value=-EFAULT;
+ break;
+ }
+ break;
Quoted 6 lines
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.
Quoted 16 lines
Quoted 15 lines
+ 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_addrsuccessfully\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_userfail "
+ "returning =%d\n",-EFAULT);
+ret_value=-EFAULT;
+ break;
+ }
+ break;
Quoted 3 lines
I believe this really belongs into the ethernet driver, using the
ethtool_ops->get_eepromoperation 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