Topcliff PHUB: Generate PacketHub driver
PATCH 1/1 Topcliff PHUB: Generate PacketHub driver
Hi Alan,
We have modified for the following your indication. Please confirm.
Quoted 18 lines
Quoted 9 lines
+/*!
+ * @ingroup
PHUB_PCILayerAPI+ * @fn static void
__exitpch_phub_pci_exit(void)+ * @brief Provides the functionality of exiting the module
+ * */
Can we please do something about all this stuff first so the code is
easier to review by those not familiar with the kernel format.
Quoted 5 lines
I sent a bit of code out last time that did most of the work
Alan
Thanks.
Masayuki Ohtake
--- linux-2.6.33.1/drivers/char/Kconfig | 20 + linux-2.6.33.1/drivers/char/Makefile | 4 + linux-2.6.33.1/drivers/char/pch_phub/Makefile | 11 + linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c | 1232 +++++++++++++++++++++++ linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h | 96 ++ 5 files changed, 1363 insertions(+), 0 deletions(-) create mode 100644 linux-2.6.33.1/drivers/char/pch_phub/Makefile create mode 100755 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c create mode 100755 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h
drivers/char/Kconfig +20 -0
@@ -4,6 +4,26 @@ menu "Character devices" +config PCH_IEEE1588 + tristate "PCH IEEE1588" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH IEEE1588 Host controller. + +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 PCH_CAN_PCLK_50MHZ + bool "CAN PCLK 50MHz" + depends on PCH_PHUB + help + If you say yes to this option, clock is set to 50MHz.(For CAN control) + config VT bool "Virtual terminal" if EMBEDDED depends on !S390
drivers/char/Makefile +4 -0
@@ -111,6 +111,10 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_IEEE1588) += pch_ieee1588/ + +obj-$(CONFIG_PCH_PHUB) += pch_phub/ + # Files generated that shall be removed upon make clean clean-files := consolemap_deftbl.c defkeymap.c
drivers/char/pch_phub/Makefile new file +11 -0
@@ -0,0 +1,11 @@ +ifeq ($(CONFIG_PHUB_DEBUG),y) +EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_PCH_PHUB) += pch_phub_drv.o +#to set CAN clock to 50Mhz +ifdef CONFIG_PCH_CAN_PCLK_50MHZ +EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ +endif + +pch_phub_drv-objs := pch_phub.o
drivers/char/pch_phub/pch_phub.c new file +1232 -0
1,232 lines not on this page · raw message
drivers/char/pch_phub/pch_phub.h new file +96 -0
96 lines not on this page · raw message
Signature · 1 line
1.6.0.6
O> +/* Status Register offset */
Quoted 9 lines
drivers/char/pch_phub/pch_phub.c new file +9
-0+#define PHUB_STATUS (0x00) +/* Control Register offset */ +#define PHUB_CONTROL (0x04) +/* Time out value for Status Register */ +#define PHUB_TIMEOUT (0x05) +/* Enabling for writing ROM */ +#define PCH_PHUB_ROM_WRITE_ENABLE (0x01) +/* Disabling for writing ROM */ +#define PCH_PHUB_ROM_WRITE_DISABLE (0x00)
drivers/char/pch_phub/pch_phub.c line 69
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+/* ROM data area start address offset */ +#define PCH_PHUB_ROM_START_ADDR (0x14) +/* MAX number of INT_REDUCE_CONTROL registers */ +#define MAX_NUM_INT_REDUCE_CONTROL_REG (128) + +#define PCI_DEVICE_ID_PCH1_PHUB (0x8801) + +#define PCH_MINOR_NOS (1) +#define CLKCFG_CAN_50MHZ (0x12000000) +#define CLKCFG_CANCLK_MASK (0xFF000000)
Style: constants don't need brackets - might also look more Linux like tabbed out a bit
drivers/char/pch_phub/pch_phub.c line 75
Quoted 3 lines
drivers/char/pch_phub/pch_phub.c new file +3
-0+#define PCH_READ_REG (a) ioread32((pch_phub_base_address + a)) + +#define PCH_WRITE_REG (a, b) iowrite32(a, (pch_phub_base_address + b))
These on the other hand do - but not where they are now
iowrite32((a), pcb_phub_base_address + (b))
(so that if a or b are expressions they are evaluated first)
Quoted 8 lines
drivers/char/pch_phub/pch_phub.c new file +8
-0+struct pch_phub_reg { +u32 phub_id_reg;/* PHUB_ID register val */ +u32 q_pri_val_reg;/* QUEUE_PRI_VAL register val */ +u32 rc_q_maxsize_reg;/* RC_QUEUE_MAXSIZE register val */ +u32 bri_q_maxsize_reg;/* BRI_QUEUE_MAXSIZE register val */ +u32 comp_resp_timeout_reg;/* COMP_RESP_TIMEOUT register val */ +u32 bus_slave_control_reg;/* BUS_SLAVE_CONTROL_REG register val */ +u32 deadlock_avoid_type_reg;/* DEADLOCK_AVOID_TYPE register val */
drivers/char/pch_phub/pch_phub.c line 97
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+u32 intpin_reg_wpermit_reg0;/* INTPIN_REG_WPERMIT register 0 val */ +u32 intpin_reg_wpermit_reg1;/* INTPIN_REG_WPERMIT register 1 val */ +u32 intpin_reg_wpermit_reg2;/* INTPIN_REG_WPERMIT register 2 val */ +u32 intpin_reg_wpermit_reg3;/* INTPIN_REG_WPERMIT register 3 val */ +/* INT_REDUCE_CONTROL registers val */ +u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG ]; +#ifdef PCH_CAN_PCLK_50MHZ +u32 clkcfg_reg;/* CLK CFG register val */ +#endif +} g_pch_phub_reg;
Stylewise the kernel doesn't use the g_ convention that many Windows people do, so lose the g_
drivers/char/pch_phub/pch_phub.c line 102
Quoted 4 lines
drivers/char/pch_phub/pch_phub.c new file +4
-0+s32 pch_phub_opencount;/* check whether opened or not */ +u32 pch_phub_base_address; +u32 pch_phub_extrom_base_address; +s32 pch_phub_suspended;
Any reason these can't be in the struct ?
Review
Quoted 2 lines
(quoted diff) +2
-0+ +struct device *dev_dbg;
Not a good name for a global variable as it will clash with others
Quoted 1 line
+DEFINE_SPINLOCK(pch_phub_lock); /* for spin lock */
Can this be static or in the struct ?
drivers/char/pch_phub/pch_phub.c line 116
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-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, +};
static const ?
drivers/char/pch_phub/pch_phub.c line 126
Quoted 3 lines
drivers/char/pch_phub/pch_phub.c new file +3
-0+/*-------------------------------------------- +exported function prototypes +--------------------------------------------*/
They start 'static' I don't think they are exportdd !
Quoted 15 lines
drivers/char/pch_phub/pch_phub.c new file +14
-0+static int __devinit pch_phub_probe (struct pci_dev *pdev,const +struct pci_device_id *id); +static void __devexit pch_phub_remove(struct pci_dev *pdev); +static int pch_phub_suspend (struct pci_dev *pdev,pm_message_t state); +static int pch_phub_resume (struct pci_dev *pdev); +static struct pci_driver pch_phub_driver = { + .name ="pch_phub" , + .id_table = pch_phub_pcidev_id, + .probe = pch_phub_probe, + .remove =__devexit_p (pch_phub_remove), +#ifdef CONFIG_PM + .suspend = pch_phub_suspend, + .resume = pch_phub_resume +#endif
drivers/char/pch_phub/pch_phub.c line 161
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+}; + +static int __init pch_phub_pci_init (void ); +static void __exit pch_phub_pci_exit(void); + +MODULE_DESCRIPTION ("PCH PACKET HUB PCI Driver" ); +MODULE_LICENSE ("GPL" ); +module_init (pch_phub_pci_init); +module_exit (pch_phub_pci_exit); +module_param (pch_phub_major_no, int,S_IRUSR |S_IWUSR );
If you migrated the module registration/PCI registration to the bottom of the file you could avoid the forward declations and make the code easier to follow ?
drivers/char/pch_phub/pch_phub.c line 178
Quoted 6 lines
drivers/char/pch_phub/pch_phub.c new file +6
-0+void pch_phub_read_modify_write_reg (unsigned long reg_addr_offset, +unsigned long data,unsigned long mask) +{ +unsigned long reg_addr = pch_phub_base_address + reg_addr_offset; +iowrite32 (((ioread32 ((void __iomem *)reg_addr) & ~mask)) | data, + (void __iomem *)reg_addr);
When you have that many casts in a line it's perhaps a hint you have the types wrong initially. At the very least reg_addr should be void __iomem, and probably pch_phub_base_address should be
It would probably make sense to pass a pointer to your struct pch_hub_reg and use that to hold the base address. Then if you ever get a box with two in some future design it won't be a disaster
Quoted 1 line
+void
pch_phub_save_reg_conf(void)
Ditto
drivers/char/pch_phub/pch_phub.c line 256
Quoted 4 lines
drivers/char/pch_phub/pch_phub.c new file +4
-0+#ifdef PCH_CAN_PCLK_50MHZ +/* save clk cfg register */ + g_pch_phub_reg.clkcfg_reg =PCH_READ_REG (CLKCFG_REG_OFFSET ); +#endif
This makes it hard to build one kernel for everything
drivers/char/pch_phub/pch_phub.c line 265
Quoted 6 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+return ; +} +void pch_phub_restore_reg_conf (void ) +{ +u32 i =0 ;
Why = 0, if you do that here you may hide initialisation errors from future changes ?
drivers/char/pch_phub/pch_phub.c line 350
Quoted 12 lines
drivers/char/pch_phub/pch_phub.c new file +12
-0+/** pch_phub_read_serial_rom - Implements the functionality of reading Serial +* ROM. +* @offset_address: Contains the Serial ROM address offset value +* @data: Contains the Serial ROM value +*/ +int pch_phub_read_serial_rom (unsigned long offset_address,unsigned char *data) +{ +unsigned long mem_addr = pch_phub_extrom_base_address + offset_address; + +dev_dbg (dev_dbg, +"pch_phub_read_serial_rom:mem_addr=0x%08x\n" , (u32 )mem_addr); + *data =ioread8 ((void __iomem *)mem_addr);
Same comments about casts
drivers/char/pch_phub/pch_phub.c line 577
Quoted 6 lines
drivers/char/pch_phub/pch_phub.c new file +6
-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;
This will change size on 32/64bit boxes makign your copy a bit problematic
drivers/char/pch_phub/pch_phub.c line 838
Quoted 7 lines
drivers/char/pch_phub/pch_phub.c new file +6
-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) { +/* End of Access area check */
Remember here ioctl isn't single threaded so you may want to double check some of these
drivers/char/pch_phub/pch_phub.h line 80
Quoted 5 lines
drivers/char/pch_phub/pch_phub.h new file +5
-0+struct pch_phub_reqt { +unsigned long addr_offset;/*specifies the register address +offset */ +unsigned long data;/*specifies the data */ +unsigned long mask;/*specifies the mask */
If they may need to be long make them u64. That way 32 and 64bit systems get the same ioctl structure and life is good.
drivers/char/pch_phub/pch_phub.h line 87
Quoted 7 lines
drivers/char/pch_phub/pch_phub.h new file +7
-0+}; + +/* exported function prototypes */ +int pch_phub_open (struct inode *inode,struct file *file); +int pch_phub_release (struct inode *inode,struct file *file); +int pch_phub_ioctl (struct inode *inode,struct file *file,unsigned int cmd, +unsigned long arg);
You have various other functions that are not static - if they should be then make them static
Alan
Hi Alan,
Thank you for your comments.
We will integrate your comments to our PHUB by today or tomorrow at the latest.
Ohtake.
----- Original Message -----
From: "Alan Cox" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Tuesday, June 08, 2010 12:05 AM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver
Quoted 217 lines
O> +/* Status Register offset */
Quoted 9 lines
drivers/char/pch_phub/pch_phub.c new file +9
-0+#define PHUB_STATUS (0x00) +/* Control Register offset */ +#define PHUB_CONTROL (0x04) +/* Time out value for Status Register */ +#define PHUB_TIMEOUT (0x05) +/* Enabling for writing ROM */ +#define PCH_PHUB_ROM_WRITE_ENABLE (0x01) +/* Disabling for writing ROM */ +#define PCH_PHUB_ROM_WRITE_DISABLE (0x00)drivers/char/pch_phub/pch_phub.c line 69
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+/* ROM data area start address offset */ +#define PCH_PHUB_ROM_START_ADDR (0x14) +/* MAX number of INT_REDUCE_CONTROL registers */ +#define MAX_NUM_INT_REDUCE_CONTROL_REG (128) + +#define PCI_DEVICE_ID_PCH1_PHUB (0x8801) + +#define PCH_MINOR_NOS (1) +#define CLKCFG_CAN_50MHZ (0x12000000) +#define CLKCFG_CANCLK_MASK (0xFF000000)Style: constants don't need brackets - might also look more Linux like tabbed out a bit
drivers/char/pch_phub/pch_phub.c line 75
Quoted 3 lines
drivers/char/pch_phub/pch_phub.c new file +3
-0+#define PCH_READ_REG (a) ioread32((pch_phub_base_address + a)) + +#define PCH_WRITE_REG (a, b) iowrite32(a, (pch_phub_base_address + b))These on the other hand do - but not where they are now
iowrite32((a), pcb_phub_base_address + (b))(so that if a or b are expressions they are evaluated first)
Quoted 8 lines
drivers/char/pch_phub/pch_phub.c new file +8
-0+struct pch_phub_reg { +u32 phub_id_reg;/* PHUB_ID register val */ +u32 q_pri_val_reg;/* QUEUE_PRI_VAL register val */ +u32 rc_q_maxsize_reg;/* RC_QUEUE_MAXSIZE register val */ +u32 bri_q_maxsize_reg;/* BRI_QUEUE_MAXSIZE register val */ +u32 comp_resp_timeout_reg;/* COMP_RESP_TIMEOUT register val */ +u32 bus_slave_control_reg;/* BUS_SLAVE_CONTROL_REG register val */ +u32 deadlock_avoid_type_reg;/* DEADLOCK_AVOID_TYPE register val */ drivers/char/pch_phub/pch_phub.c line 97
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+u32 intpin_reg_wpermit_reg0;/* INTPIN_REG_WPERMIT register 0 val */ +u32 intpin_reg_wpermit_reg1;/* INTPIN_REG_WPERMIT register 1 val */ +u32 intpin_reg_wpermit_reg2;/* INTPIN_REG_WPERMIT register 2 val */ +u32 intpin_reg_wpermit_reg3;/* INTPIN_REG_WPERMIT register 3 val */ +/* INT_REDUCE_CONTROL registers val */ +u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG ]; +#ifdef PCH_CAN_PCLK_50MHZ +u32 clkcfg_reg;/* CLK CFG register val */ +#endif +} g_pch_phub_reg;Stylewise the kernel doesn't use the g_ convention that many Windows people do, so lose the g_
drivers/char/pch_phub/pch_phub.c line 102
Quoted 4 lines
drivers/char/pch_phub/pch_phub.c new file +4
-0+s32 pch_phub_opencount;/* check whether opened or not */ +u32 pch_phub_base_address; +u32 pch_phub_extrom_base_address; +s32 pch_phub_suspended;Any reason these can't be in the struct ?
Review
Quoted 2 lines
(quoted diff) +2
-0+ +struct device *dev_dbg;Not a good name for a global variable as it will clash with others
Quoted 1 line
+DEFINE_SPINLOCK(pch_phub_lock); /* for spin lock */
Can this be static or in the struct ?
drivers/char/pch_phub/pch_phub.c line 116
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-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, +};static const ?
Quoted 3 lines
+/*--------------------------------------------
+ exported function prototypes
+--------------------------------------------*/They start 'static' I don't think they are exportdd !
Quoted 15 lines
drivers/char/pch_phub/pch_phub.c new file +14
-0+static int __devinit pch_phub_probe (struct pci_dev *pdev,const +struct pci_device_id *id); +static void __devexit pch_phub_remove(struct pci_dev *pdev); +static int pch_phub_suspend (struct pci_dev *pdev,pm_message_t state); +static int pch_phub_resume (struct pci_dev *pdev); +static struct pci_driver pch_phub_driver = { + .name ="pch_phub" , + .id_table = pch_phub_pcidev_id, + .probe = pch_phub_probe, + .remove =__devexit_p (pch_phub_remove), +#ifdef CONFIG_PM + .suspend = pch_phub_suspend, + .resume = pch_phub_resume +#endifdrivers/char/pch_phub/pch_phub.c line 161
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+}; + +static int __init pch_phub_pci_init (void ); +static void __exit pch_phub_pci_exit(void); + +MODULE_DESCRIPTION ("PCH PACKET HUB PCI Driver" ); +MODULE_LICENSE ("GPL" ); +module_init (pch_phub_pci_init); +module_exit (pch_phub_pci_exit); +module_param (pch_phub_major_no, int,S_IRUSR |S_IWUSR );If you migrated the module registration/PCI registration to the bottom of the file you could avoid the forward declations and make the code easier to follow ?
drivers/char/pch_phub/pch_phub.c line 178
Quoted 6 lines
drivers/char/pch_phub/pch_phub.c new file +6
-0+void pch_phub_read_modify_write_reg (unsigned long reg_addr_offset, +unsigned long data,unsigned long mask) +{ +unsigned long reg_addr = pch_phub_base_address + reg_addr_offset; +iowrite32 (((ioread32 ((void __iomem *)reg_addr) & ~mask)) | data, + (void __iomem *)reg_addr);When you have that many casts in a line it's perhaps a hint you have the types wrong initially. At the very least
reg_addrshould be void__iomem, and probablypch_phub_base_addressshould beIt would probably make sense to pass a pointer to your struct
pch_hub_regand use that to hold the base address. Then if you ever get a box with two in some future design it won't be a disasterQuoted 1 line
+void
pch_phub_save_reg_conf(void)Ditto
drivers/char/pch_phub/pch_phub.c line 256
Quoted 4 lines
drivers/char/pch_phub/pch_phub.c new file +4
-0+#ifdef PCH_CAN_PCLK_50MHZ +/* save clk cfg register */ + g_pch_phub_reg.clkcfg_reg =PCH_READ_REG (CLKCFG_REG_OFFSET ); +#endif This makes it hard to build one kernel for everything
drivers/char/pch_phub/pch_phub.c line 265
Quoted 6 lines
+ return;
drivers/char/pch_phub/pch_phub.c new file +4
-0+} +void pch_phub_restore_reg_conf (void ) +{ +u32 i =0 ;Why = 0, if you do that here you may hide initialisation errors from future changes ?
drivers/char/pch_phub/pch_phub.c line 350
Quoted 12 lines
drivers/char/pch_phub/pch_phub.c new file +12
-0+/** pch_phub_read_serial_rom - Implements the functionality of reading Serial +* ROM. +* @offset_address: Contains the Serial ROM address offset value +* @data: Contains the Serial ROM value +*/ +int pch_phub_read_serial_rom (unsigned long offset_address,unsigned char *data) +{ +unsigned long mem_addr = pch_phub_extrom_base_address + offset_address; + +dev_dbg (dev_dbg, +"pch_phub_read_serial_rom:mem_addr=0x%08x\n" , (u32 )mem_addr); + *data =ioread8 ((void __iomem *)mem_addr);Same comments about casts
drivers/char/pch_phub/pch_phub.c line 577
Quoted 6 lines
drivers/char/pch_phub/pch_phub.c new file +6
-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;This will change size on 32/64bit boxes makign your copy a bit problematic
Quoted 7 lines
+
p_pch_phub_reqt= (structpch_phub_reqt*)arg;
+ret_value= copy_from_user((void *)&addr_offset,
+ (void *)&p_pch_phub_reqt->addr_offset,
+sizeof(addr_offset));
+ if (ret_value) {+ /* End of Access area check */
Remember here ioctl isn't single threaded so you may want to double check some of these
Quoted 5 lines
+struct
pch_phub_reqt{
+ unsigned longaddr_offset; /*specifies the register address
+ offset */
+ unsigned long data; /*specifies the data */
+ unsigned long mask; /*specifies the mask */If they may need to be long make them u64. That way 32 and 64bit systems get the same ioctl structure and life is good.
drivers/char/pch_phub/pch_phub.h line 87
Quoted 7 lines
drivers/char/pch_phub/pch_phub.h new file +7
-0+}; + +/* exported function prototypes */ +int pch_phub_open (struct inode *inode,struct file *file); +int pch_phub_release (struct inode *inode,struct file *file); +int pch_phub_ioctl (struct inode *inode,struct file *file,unsigned int cmd, +unsigned long arg);You have various other functions that are not static - if they should be then make them static
Alan
Hi we have modified for your comments.
Please Confirm below.
Thanks.
Ohtake.
--- linux-2.6.33.1/drivers/char/Kconfig | 25 + linux-2.6.33.1/drivers/char/Makefile | 4 + linux-2.6.33.1/drivers/char/pch_phub/Makefile | 11 + linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c | 1000 +++++++++++++++++++++++ linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h | 87 ++ 5 files changed, 1127 insertions(+), 0 deletions(-) create mode 100644 linux-2.6.33.1/drivers/char/pch_phub/Makefile create mode 100755 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.c create mode 100755 linux-2.6.33.1/drivers/char/pch_phub/pch_phub.h
drivers/char/Kconfig +25 -0
@@ -4,6 +4,31 @@ menu "Character devices" +config PCH_IEEE1588 + tristate "PCH IEEE1588" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH IEEE1588 Host controller. + +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. + This driver is for PCH Packet hub driver for Topcliff. + This driver is integrated as built-in. + This driver can access GbE MAC address. + This driver can access HW register. + You can also be integrated as module. + +config PCH_CAN_PCLK_50MHZ + bool "CAN PCLK 50MHz" + depends on PCH_PHUB + help + If you say yes to this option, clock is set to 50MHz.(For CAN control) + config VT bool "Virtual terminal" if EMBEDDED depends on !S390
drivers/char/Makefile +4 -0
@@ -111,6 +111,10 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_IEEE1588) += pch_ieee1588/ + +obj-$(CONFIG_PCH_PHUB) += pch_phub/ + # Files generated that shall be removed upon make clean clean-files := consolemap_deftbl.c defkeymap.c
drivers/char/pch_phub/Makefile new file +11 -0
@@ -0,0 +1,11 @@ +ifeq ($(CONFIG_PHUB_DEBUG),y) +EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_PCH_PHUB) += pch_phub_drv.o +#to set CAN clock to 50Mhz +ifdef CONFIG_PCH_CAN_PCLK_50MHZ +EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ +endif + +pch_phub_drv-objs := pch_phub.o
drivers/char/pch_phub/pch_phub.c new file +1000 -0
1,000 lines not on this page · raw message
drivers/char/pch_phub/pch_phub.h new file +87 -0
87 lines not on this page · raw message
Signature · 1 line
1.6.0.6
Arnd Bergmann
maintainerQuoted 2 lines · Masayuki Ohtak
Hi we have modified for your comments.
Please Confirm below.
Looks much better. A few more comments about the new code:
drivers/char/pch_phub/Makefile line 9
Quoted 4 lines
drivers/char/pch_phub/Makefile new file +4
-0+#to set CAN clock to 50Mhz +ifdef CONFIG_PCH_CAN_PCLK_50MHZ +EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ +endif
This should not be necessary. Just use CONFIG_PCH_CAN_PCLK_50MHZ directly in the code instead of the extra PCH_CAN_PCLK_50MHZ macro.
Review
Quoted 2 lines
(quoted diff) +2
-0+ +DEFINE_MUTEX(pch_phub_ioctl_mutex);
This should probable be 'static DEFINE_MUTEX', since the symbol does not need to be visible in the entire kernel.
Quoted 5 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+/*-------------------------------------------- +internal function prototypes +--------------------------------------------*/ +static __s32 __devinit pch_phub_probe (struct pci_dev *pdev,const +struct pci_device_id *id);
drivers/char/pch_phub/pch_phub.c line 147
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+static void __devexit pch_phub_remove(struct pci_dev *pdev); +static __s32 pch_phub_suspend (struct pci_dev *pdev,pm_message_t state); +static __s32 pch_phub_resume (struct pci_dev *pdev); +static __s32 pch_phub_open (struct inode *inode,struct file *file); +static __s32 pch_phub_release (struct inode *inode,struct file *file); +static long pch_phub_ioctl (struct file *file,unsigned int cmd, +unsigned long arg); +static ssize_t pch_phub_read (struct file *,char __user *,size_t ,loff_t *); +static ssize_t pch_phub_write (struct file *,const char __user *, +size_t ,loff_t *);
My general recommendation would be to reorder all the function definitions so that you don't need any of these forward declarations. That is the order used in most parts of the kernel (so you start reading at the bottom), and it makes it easier to understand the structure of the code IMHO.
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+/** pch_phub_open - Implements the Initializing and opening of the Packet Hub +module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +*/ +static __s32 pch_phub_open (struct inode *inode,struct file *file) +{ +__s32 ret; + +spin_lock (&pch_phub_lock);
drivers/char/pch_phub/pch_phub.c line 535
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (pch_phub_reg.pch_phub_opencount) { + ret = -EBUSY ; + }else { + pch_phub_reg.pch_phub_opencount++; + ret =0 ; + } +spin_unlock (&pch_phub_lock); + +return ret; +}
As far as I can tell, there is no longer a reason to prevent multiple openers. Besides, even if there is only a single user, you might still have concurrency problems, so the lock does not help and you could remove the open function entirely.
Quoted 31 lines
drivers/char/pch_phub/pch_phub.c new file +31
-0+/** pch_phub_read - Implements the read functionality of the Packet Hub module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @pos: Contains the reference of the file structure +*/ + +static ssize_t pch_phub_read (struct file *file,char __user *buf,size_t size, +loff_t *pos) +{ +__u32 rom_signature =0 ; +__u8 rom_length; +__s32 ret_value; +__u32 tmp; +__u8 data; +__u32 addr_offset =0 ; + +/*Get Rom signature*/ +pch_phub_read_serial_rom (0x80 , (__u8 *)&rom_signature); +pch_phub_read_serial_rom (0x81 , (__u8 *)&tmp); + rom_signature |= (tmp &0xff ) <<8 ; +if (rom_signature ==0xAA55 ) { +pch_phub_read_serial_rom (0x82 , &rom_length); +if (size > (rom_length *512 )+1 ) +return -ENOMEM ; + +for (addr_offset =0 ; + addr_offset <= ((__u32 )rom_length *512 ); + addr_offset++) { +pch_phub_read_serial_rom (0x80 + addr_offset, &data); + ret_value = copy_to_user((void *)&buf[addr_offset],
drivers/char/pch_phub/pch_phub.c line 576
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+ (void *)&data, 1); +if (ret_value) +return -EFAULT ; + } + }else { +return -ENOEXEC ; + } + +return rom_length *512 +1 ; +}
This function has multiple problems:
- If the size argument is less than rom_length*512, you access past the user-provided buffer.
- When the user does an llseek or pread, the *pos argument is not zero, so you should return data from the middle, but you still return data from the beginning.
- You don't update the *pos argument with the new position, so you cannot continue the read where the first call left.
- Instead of returning -ENOMEM, you should just the data you have (or 0 for end-of-file).
- ENOEXEC does not seem appropriate either: The user can just check these buffer for the signature here, so you just as well return whatever you find in the ROM.
Quoted 19 lines
drivers/char/pch_phub/pch_phub.c new file +19
-0+ +/** pch_phub_write - Implements the write functionality of the Packet Hub +* module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @pos: Contains the reference of the file structure +*/ +static ssize_t pch_phub_write (struct file *file,const char __user *buf, +size_t size,loff_t *pos) +{ +static __u8 data[PCH_PHUB_OROM_SIZE ]; +__s32 ret_value; +__u32 addr_offset =0 ; + +if (size >PCH_PHUB_OROM_SIZE ) + size =PCH_PHUB_OROM_SIZE ; + + ret_value =copy_from_user (data, buf, size);
drivers/char/pch_phub/pch_phub.c line 605
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (ret_value) +return -EFAULT ; + +for (addr_offset =0 ; addr_offset < size; addr_offset++) { + ret_value =pch_phub_write_serial_rom (0x80 + addr_offset, + data[addr_offset]); + } + +return size; +}
This has the same problems, plus a buffer overflow: You must never have an array of multiple kilobytes on the stack (data[PCH_PHUB_OROM_SIZE]), because the stack itself is only a few kilobytes in the kernel. Better use a loop with copy_from_user like the read function does.
Quoted 5 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+/** pch_phub_release - Implements the release functionality of the Packet Hub +* module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +*/
drivers/char/pch_phub/pch_phub.c line 622
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+static __s32 pch_phub_release (struct inode *inode,struct file *file) +{ +spin_lock (&pch_phub_lock); + +if (pch_phub_reg.pch_phub_opencount >0 ) + pch_phub_reg.pch_phub_opencount--; +spin_unlock (&pch_phub_lock); + +return 0 ; +}
When you remove the open function, this one can go away as well.
Quoted 13 lines
drivers/char/pch_phub/pch_phub.c new file +13
-0+/** pch_phub_ioctl - Implements the various ioctl functionalities of the Packet +* Hub module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +* @cmd: Contains the command value +* @arg: Contains the command argument value +*/ + + +static long pch_phub_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{ +__s32 ret_value =0 ;
drivers/char/pch_phub/pch_phub.c line 646
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+struct pch_phub_reqt __user *p_pch_phub_reqt; +__u32 data; +__u32 mac_addr[2 ]; +__s32 ret; +__u32 i; +void __user *varg = (void __user *)arg; + + ret =mutex_trylock (&pch_phub_ioctl_mutex); +if (ret ==0 ) + goto return_ioctrl;/*Can't get mutex lock*/
mutex_trylock is probably not what you want, it returns immediately when there is another function in the kernel. mutex_lock_interruptible seems more appropriate, it will block until the mutex is free or the process gets sent a signal, which you should handle by returning -ERESTARTSYS.
In either case, you must not jump to return_ioctrl here, because that will try to release the mutex that you do not hold here, causing a hang the next time you enter the function.
drivers/char/pch_phub/pch_phub.c line 656
Quoted 9 lines
drivers/char/pch_phub/pch_phub.c new file +9
-0+if (pch_phub_reg.pch_phub_suspended == true) { + ret_value = -EPERM ; + goto return_ioctrl; + } + + p_pch_phub_reqt = (struct pch_phub_reqt *)varg; + +if (ret_value) + goto return_ioctrl;
is always zero here.
Quoted 9 lines
drivers/char/pch_phub/pch_phub.c new file +9
-0+/* End of Access area check */ + + +switch (cmd) { + +case IOCTL_PHUB_READ_MAC_ADDR : + mac_addr[0 ] =0 ; + mac_addr[1 ] =0 ; +for (i =0 ; i <4 ; i++) {
drivers/char/pch_phub/pch_phub.c line 676
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+pch_phub_read_gbe_mac_addr (i, (__u8 *)&data); + mac_addr[0 ] |= data << i*8 ; + } +for (; i <6 ; i++) { +pch_phub_read_gbe_mac_addr (i, (__u8 *)&data); + mac_addr[1 ] |= data << (i-4 )*8 ; + } + + ret_value =copy_to_user ((void *)&p_pch_phub_reqt->data, + (void *)mac_addr,sizeof (mac_addr));
p_pch_phub_reqt->data has multiple problems:
- You have the typecast to (void *), which is wrong and unneeded.
- The data structure has no point at all any more when you use only one field.
Just make this
u8 mac_addr[6]; for (i = 0; i < 4; i++) pch_phub_read_gbe_mac_addr(i, &mac_addr[i]); ret_value = copy_to_user(varg, mac_addr, sizeof(mac_addr));
Quoted 7 lines
drivers/char/pch_phub/pch_phub.h new file +7
-0+#define PHUB_IOCTL_MAGIC (0xf7) + +/*Outlines the read register function signature. */ +#define IOCTL_PHUB_READ_REG (_IOW(PHUB_IOCTL_MAGIC, 1, __u32)) + +/*Outlines the write register function signature. */ +#define IOCTL_PHUB_WRITE_REG (_IOW(PHUB_IOCTL_MAGIC, 2, __u32))
drivers/char/pch_phub/pch_phub.h line 48
Quoted 10 lines
drivers/char/pch_phub/pch_phub.h new file +10
-0+ +/*Outlines the read, modify and write register function signature. */ +#define IOCTL_PHUB_READ_MODIFY_WRITE_REG (_IOW(PHUB_IOCTL_MAGIC, 3,\ + __u32)) + +/*Outlines the read option rom function signature. */ +#define IOCTL_PHUB_READ_OROM (_IOW(PHUB_IOCTL_MAGIC, 4, __u32)) + +/*Outlines the write option rom function signature. */ +#define IOCTL_PHUB_WRITE_OROM (_IOW(PHUB_IOCTL_MAGIC, 5, __u32))
These should all get removed now.
drivers/char/pch_phub/pch_phub.h line 54
Quoted 5 lines
drivers/char/pch_phub/pch_phub.h new file +5
-0+/*Outlines the read mac address function signature. */ +#define IOCTL_PHUB_READ_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 6, __u32)) + +/*brief Outlines the write mac address function signature. */ +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 7, __u32))
IOCTL_PHUB_READ_MAC_ADDR needs _IOR instead of _IOW, and the type is still wrong here. Your code currently has struct pch_phub_reqt instead of __u32, and if you change the ioctl function as I explained above, it should become
#define IOCTL_PHUB_READ_MAC_ADDR (_IOR(PHUB_IOCTL_MAGIC, 6, __u8[6])) #define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 7, __u8[6]))
Arnd
Hi Arnd
Thank you for your quick comments.
After modifying, we will resubmit the patch.
Thanks, Ohtake.
----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: "Alan Cox" <[email protected]>; "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Monday, June 14, 2010 9:50 PM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver
Quoted 293 lines · Masayuki Ohtak
Quoted 2 lines
Hi we have modified for your comments.
Please Confirm below.Looks much better. A few more comments about the new code:
drivers/char/pch_phub/Makefile line 9
Quoted 4 lines
drivers/char/pch_phub/Makefile new file +4
-0+#to set CAN clock to 50Mhz +ifdef CONFIG_PCH_CAN_PCLK_50MHZ +EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ +endifThis should not be necessary. Just use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the code instead of the extraPCH_CAN_PCLK_50MHZmacro.Review
Quoted 2 lines
(quoted diff) +2
-0+ +DEFINE_MUTEX(pch_phub_ioctl_mutex);This should probable be 'static
DEFINE_MUTEX', since the symbol does not need to be visible in the entire kernel.Quoted 5 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+/*-------------------------------------------- +internal function prototypes +--------------------------------------------*/ +static __s32 __devinit pch_phub_probe (struct pci_dev *pdev,const +struct pci_device_id *id);drivers/char/pch_phub/pch_phub.c line 147
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+static void __devexit pch_phub_remove(struct pci_dev *pdev); +static __s32 pch_phub_suspend (struct pci_dev *pdev,pm_message_t state); +static __s32 pch_phub_resume (struct pci_dev *pdev); +static __s32 pch_phub_open (struct inode *inode,struct file *file); +static __s32 pch_phub_release (struct inode *inode,struct file *file); +static long pch_phub_ioctl (struct file *file,unsigned int cmd, +unsigned long arg); +static ssize_t pch_phub_read (struct file *,char __user *,size_t ,loff_t *); +static ssize_t pch_phub_write (struct file *,const char __user *, +size_t ,loff_t *);My general recommendation would be to reorder all the function definitions so that you don't need any of these forward declarations. That is the order used in most parts of the kernel (so you start reading at the bottom), and it makes it easier to understand the structure of the code IMHO.
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+/** pch_phub_open - Implements the Initializing and opening of the Packet Hub +module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +*/ +static __s32 pch_phub_open (struct inode *inode,struct file *file) +{ +__s32 ret; + +spin_lock (&pch_phub_lock);drivers/char/pch_phub/pch_phub.c line 535
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (pch_phub_reg.pch_phub_opencount) { + ret = -EBUSY ; + }else { + pch_phub_reg.pch_phub_opencount++; + ret =0 ; + } +spin_unlock (&pch_phub_lock); + +return ret; +}As far as I can tell, there is no longer a reason to prevent multiple openers. Besides, even if there is only a single user, you might still have concurrency problems, so the lock does not help and you could remove the open function entirely.
Quoted 31 lines
drivers/char/pch_phub/pch_phub.c new file +31
-0+/** pch_phub_read - Implements the read functionality of the Packet Hub module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @pos: Contains the reference of the file structure +*/ + +static ssize_t pch_phub_read (struct file *file,char __user *buf,size_t size, +loff_t *pos) +{ +__u32 rom_signature =0 ; +__u8 rom_length; +__s32 ret_value; +__u32 tmp; +__u8 data; +__u32 addr_offset =0 ; + +/*Get Rom signature*/ +pch_phub_read_serial_rom (0x80 , (__u8 *)&rom_signature); +pch_phub_read_serial_rom (0x81 , (__u8 *)&tmp); + rom_signature |= (tmp &0xff ) <<8 ; +if (rom_signature ==0xAA55 ) { +pch_phub_read_serial_rom (0x82 , &rom_length); +if (size > (rom_length *512 )+1 ) +return -ENOMEM ; + +for (addr_offset =0 ; + addr_offset <= ((__u32 )rom_length *512 ); + addr_offset++) { +pch_phub_read_serial_rom (0x80 + addr_offset, &data); + ret_value = copy_to_user((void *)&buf[addr_offset],drivers/char/pch_phub/pch_phub.c line 576
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+ (void *)&data, 1); +if (ret_value) +return -EFAULT ; + } + }else { +return -ENOEXEC ; + } + +return rom_length *512 +1 ; +}This function has multiple problems:
- If the size argument is less than rom_length*512, you access past the user-provided buffer.
- When the user does an llseek or pread, the *pos argument is not zero, so you should return data from the middle, but you still return data from the beginning.
- You don't update the *pos argument with the new position, so you cannot continue the read where the first call left.
- Instead of returning-ENOMEM, you should just the data you have (or 0 for end-of-file).
- ENOEXEC does not seem appropriate either: The user can just check these buffer for the signature here, so you just as well return whatever you find in the ROM.Quoted 19 lines
drivers/char/pch_phub/pch_phub.c new file +19
-0+ +/** pch_phub_write - Implements the write functionality of the Packet Hub +* module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @pos: Contains the reference of the file structure +*/ +static ssize_t pch_phub_write (struct file *file,const char __user *buf, +size_t size,loff_t *pos) +{ +static __u8 data[PCH_PHUB_OROM_SIZE ]; +__s32 ret_value; +__u32 addr_offset =0 ; + +if (size >PCH_PHUB_OROM_SIZE ) + size =PCH_PHUB_OROM_SIZE ; + + ret_value =copy_from_user (data, buf, size);drivers/char/pch_phub/pch_phub.c line 605
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (ret_value) +return -EFAULT ; + +for (addr_offset =0 ; addr_offset < size; addr_offset++) { + ret_value =pch_phub_write_serial_rom (0x80 + addr_offset, + data[addr_offset]); + } + +return size; +}This has the same problems, plus a buffer overflow: You must never have an array of multiple kilobytes on the stack (data[PCH_PHUB_OROM_SIZE]), because the stack itself is only a few kilobytes in the kernel. Better use a loop with
copy_from_userlike the read function does.Quoted 5 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+/** pch_phub_release - Implements the release functionality of the Packet Hub +* module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +*/ drivers/char/pch_phub/pch_phub.c line 622
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+static __s32 pch_phub_release (struct inode *inode,struct file *file) +{ +spin_lock (&pch_phub_lock); + +if (pch_phub_reg.pch_phub_opencount >0 ) + pch_phub_reg.pch_phub_opencount--; +spin_unlock (&pch_phub_lock); + +return 0 ; +}When you remove the open function, this one can go away as well.
Quoted 13 lines
drivers/char/pch_phub/pch_phub.c new file +13
-0+/** pch_phub_ioctl - Implements the various ioctl functionalities of the Packet +* Hub module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +* @cmd: Contains the command value +* @arg: Contains the command argument value +*/ + + +static long pch_phub_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{ +__s32 ret_value =0 ;drivers/char/pch_phub/pch_phub.c line 646
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+struct pch_phub_reqt __user *p_pch_phub_reqt; +__u32 data; +__u32 mac_addr[2 ]; +__s32 ret; +__u32 i; +void __user *varg = (void __user *)arg; + + ret =mutex_trylock (&pch_phub_ioctl_mutex); +if (ret ==0 ) + goto return_ioctrl;/*Can't get mutex lock*/
mutex_trylockis probably not what you want, it returns immediately when there is another function in the kernel.mutex_lock_interruptibleseems more appropriate, it will block until the mutex is free or the process gets sent a signal, which you should handle by returning-ERESTARTSYS.In either case, you must not jump to
return_ioctrlhere, because that will try to release the mutex that you do not hold here, causing a hang the next time you enter the function.drivers/char/pch_phub/pch_phub.c line 656
Quoted 9 lines
+ if (pch_phub_reg.pch_phub_suspended == true) {
+ret_value=-EPERM;
+ gotoreturn_ioctrl;
+ }drivers/char/pch_phub/pch_phub.c new file +5
-0+ + p_pch_phub_reqt = (struct pch_phub_reqt *)varg; + +if (ret_value) + goto return_ioctrl;is always zero here.
Quoted 9 lines
+ /* End of Access area check */
drivers/char/pch_phub/pch_phub.c new file +8
-0+ + +switch (cmd) { + +case IOCTL_PHUB_READ_MAC_ADDR : + mac_addr[0 ] =0 ; + mac_addr[1 ] =0 ; +for (i =0 ; i <4 ; i++) {drivers/char/pch_phub/pch_phub.c line 676
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+pch_phub_read_gbe_mac_addr (i, (__u8 *)&data); + mac_addr[0 ] |= data << i*8 ; + } +for (; i <6 ; i++) { +pch_phub_read_gbe_mac_addr (i, (__u8 *)&data); + mac_addr[1 ] |= data << (i-4 )*8 ; + } + + ret_value =copy_to_user ((void *)&p_pch_phub_reqt->data, + (void *)mac_addr,sizeof (mac_addr));
p_pch_phub_reqt->datahas multiple problems:- You have the typecast to (void *), which is wrong and unneeded.
- The data structure has no point at all any more when you use only one field.Just make this
u8 mac_addr[6];
for (i = 0; i < 4; i++)pch_phub_read_gbe_mac_addr(i, &mac_addr[i]);ret_value=copy_to_user(varg, mac_addr, sizeof(mac_addr));Quoted 7 lines
drivers/char/pch_phub/pch_phub.h new file +7
-0+#define PHUB_IOCTL_MAGIC (0xf7) + +/*Outlines the read register function signature. */ +#define IOCTL_PHUB_READ_REG (_IOW(PHUB_IOCTL_MAGIC, 1, __u32)) + +/*Outlines the write register function signature. */ +#define IOCTL_PHUB_WRITE_REG (_IOW(PHUB_IOCTL_MAGIC, 2, __u32))drivers/char/pch_phub/pch_phub.h line 48
Quoted 10 lines
drivers/char/pch_phub/pch_phub.h new file +10
-0+ +/*Outlines the read, modify and write register function signature. */ +#define IOCTL_PHUB_READ_MODIFY_WRITE_REG (_IOW(PHUB_IOCTL_MAGIC, 3,\ + __u32)) + +/*Outlines the read option rom function signature. */ +#define IOCTL_PHUB_READ_OROM (_IOW(PHUB_IOCTL_MAGIC, 4, __u32)) + +/*Outlines the write option rom function signature. */ +#define IOCTL_PHUB_WRITE_OROM (_IOW(PHUB_IOCTL_MAGIC, 5, __u32))These should all get removed now.
drivers/char/pch_phub/pch_phub.h line 54
Quoted 5 lines
drivers/char/pch_phub/pch_phub.h new file +5
-0+/*Outlines the read mac address function signature. */ +#define IOCTL_PHUB_READ_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 6, __u32)) + +/*brief Outlines the write mac address function signature. */ +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 7, __u32))
IOCTL_PHUB_READ_MAC_ADDRneeds_IORinstead of_IOW, and the type is still wrong here. Your code currently has structpch_phub_reqtinstead of__u32, and if you change the ioctl function as I explained above, it should become#define
IOCTL_PHUB_READ_MAC_ADDR(_IOR(PHUB_IOCTL_MAGIC, 6, __u8[6])) #defineIOCTL_PHUB_WRITE_MAC_ADDR(_IOW(PHUB_IOCTL_MAGIC, 7, __u8[6]))Arnd
Hi Arnd,
Quoted 7 lines
drivers/char/pch_phub/Makefile line 9
Quoted 4 lines
drivers/char/pch_phub/Makefile new file +4
-0+#to set CAN clock to 50Mhz +ifdef CONFIG_PCH_CAN_PCLK_50MHZ +EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ +endifThis should not be necessary. Just use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the code instead of the extraPCH_CAN_PCLK_50MHZmacro.
I have a question. I show the above reason.
In case CAN is integrated as MODULE, macro name is CONFIG_PCH_CAN_PCLK_50MHZ_MODULE. On the other hand, integrated as built-in, CONFIG_PCH_CAN_PCLK_50MHZ. To prevent PHUB source code from integrated as MODULE or BUILT-IN,
we re-define macro name in Makefile.
If use CONFIG_PCH_CAN_PCLK_50MHZ directly in the source code, in case buit-in, behavior is not correct.
But in case module, behavior is not correct.
Please give us your opinion
Thanks,
Ohtake.
----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: "Alan Cox" <[email protected]>; "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Monday, June 14, 2010 9:50 PM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver
Quoted 293 lines · Masayuki Ohtak
Quoted 2 lines
Hi we have modified for your comments.
Please Confirm below.Looks much better. A few more comments about the new code:
drivers/char/pch_phub/Makefile line 9
Quoted 4 lines
drivers/char/pch_phub/Makefile new file +4
-0+#to set CAN clock to 50Mhz +ifdef CONFIG_PCH_CAN_PCLK_50MHZ +EXTRA_CFLAGS +=-DPCH_CAN_PCLK_50MHZ +endifThis should not be necessary. Just use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the code instead of the extraPCH_CAN_PCLK_50MHZmacro.Review
Quoted 2 lines
(quoted diff) +2
-0+ +DEFINE_MUTEX(pch_phub_ioctl_mutex);This should probable be 'static
DEFINE_MUTEX', since the symbol does not need to be visible in the entire kernel.Quoted 5 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+/*-------------------------------------------- +internal function prototypes +--------------------------------------------*/ +static __s32 __devinit pch_phub_probe (struct pci_dev *pdev,const +struct pci_device_id *id);drivers/char/pch_phub/pch_phub.c line 147
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+static void __devexit pch_phub_remove(struct pci_dev *pdev); +static __s32 pch_phub_suspend (struct pci_dev *pdev,pm_message_t state); +static __s32 pch_phub_resume (struct pci_dev *pdev); +static __s32 pch_phub_open (struct inode *inode,struct file *file); +static __s32 pch_phub_release (struct inode *inode,struct file *file); +static long pch_phub_ioctl (struct file *file,unsigned int cmd, +unsigned long arg); +static ssize_t pch_phub_read (struct file *,char __user *,size_t ,loff_t *); +static ssize_t pch_phub_write (struct file *,const char __user *, +size_t ,loff_t *);My general recommendation would be to reorder all the function definitions so that you don't need any of these forward declarations. That is the order used in most parts of the kernel (so you start reading at the bottom), and it makes it easier to understand the structure of the code IMHO.
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+/** pch_phub_open - Implements the Initializing and opening of the Packet Hub +module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +*/ +static __s32 pch_phub_open (struct inode *inode,struct file *file) +{ +__s32 ret; + +spin_lock (&pch_phub_lock);drivers/char/pch_phub/pch_phub.c line 535
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (pch_phub_reg.pch_phub_opencount) { + ret = -EBUSY ; + }else { + pch_phub_reg.pch_phub_opencount++; + ret =0 ; + } +spin_unlock (&pch_phub_lock); + +return ret; +}As far as I can tell, there is no longer a reason to prevent multiple openers. Besides, even if there is only a single user, you might still have concurrency problems, so the lock does not help and you could remove the open function entirely.
Quoted 31 lines
drivers/char/pch_phub/pch_phub.c new file +31
-0+/** pch_phub_read - Implements the read functionality of the Packet Hub module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @pos: Contains the reference of the file structure +*/ + +static ssize_t pch_phub_read (struct file *file,char __user *buf,size_t size, +loff_t *pos) +{ +__u32 rom_signature =0 ; +__u8 rom_length; +__s32 ret_value; +__u32 tmp; +__u8 data; +__u32 addr_offset =0 ; + +/*Get Rom signature*/ +pch_phub_read_serial_rom (0x80 , (__u8 *)&rom_signature); +pch_phub_read_serial_rom (0x81 , (__u8 *)&tmp); + rom_signature |= (tmp &0xff ) <<8 ; +if (rom_signature ==0xAA55 ) { +pch_phub_read_serial_rom (0x82 , &rom_length); +if (size > (rom_length *512 )+1 ) +return -ENOMEM ; + +for (addr_offset =0 ; + addr_offset <= ((__u32 )rom_length *512 ); + addr_offset++) { +pch_phub_read_serial_rom (0x80 + addr_offset, &data); + ret_value = copy_to_user((void *)&buf[addr_offset],drivers/char/pch_phub/pch_phub.c line 576
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+ (void *)&data, 1); +if (ret_value) +return -EFAULT ; + } + }else { +return -ENOEXEC ; + } + +return rom_length *512 +1 ; +}This function has multiple problems:
- If the size argument is less than rom_length*512, you access past the user-provided buffer.
- When the user does an llseek or pread, the *pos argument is not zero, so you should return data from the middle, but you still return data from the beginning.
- You don't update the *pos argument with the new position, so you cannot continue the read where the first call left.
- Instead of returning-ENOMEM, you should just the data you have (or 0 for end-of-file).
- ENOEXEC does not seem appropriate either: The user can just check these buffer for the signature here, so you just as well return whatever you find in the ROM.Quoted 19 lines
drivers/char/pch_phub/pch_phub.c new file +19
-0+ +/** pch_phub_write - Implements the write functionality of the Packet Hub +* module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @pos: Contains the reference of the file structure +*/ +static ssize_t pch_phub_write (struct file *file,const char __user *buf, +size_t size,loff_t *pos) +{ +static __u8 data[PCH_PHUB_OROM_SIZE ]; +__s32 ret_value; +__u32 addr_offset =0 ; + +if (size >PCH_PHUB_OROM_SIZE ) + size =PCH_PHUB_OROM_SIZE ; + + ret_value =copy_from_user (data, buf, size);drivers/char/pch_phub/pch_phub.c line 605
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (ret_value) +return -EFAULT ; + +for (addr_offset =0 ; addr_offset < size; addr_offset++) { + ret_value =pch_phub_write_serial_rom (0x80 + addr_offset, + data[addr_offset]); + } + +return size; +}This has the same problems, plus a buffer overflow: You must never have an array of multiple kilobytes on the stack (data[PCH_PHUB_OROM_SIZE]), because the stack itself is only a few kilobytes in the kernel. Better use a loop with
copy_from_userlike the read function does.Quoted 5 lines
drivers/char/pch_phub/pch_phub.c new file +5
-0+/** pch_phub_release - Implements the release functionality of the Packet Hub +* module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +*/ drivers/char/pch_phub/pch_phub.c line 622
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+static __s32 pch_phub_release (struct inode *inode,struct file *file) +{ +spin_lock (&pch_phub_lock); + +if (pch_phub_reg.pch_phub_opencount >0 ) + pch_phub_reg.pch_phub_opencount--; +spin_unlock (&pch_phub_lock); + +return 0 ; +}When you remove the open function, this one can go away as well.
Quoted 13 lines
drivers/char/pch_phub/pch_phub.c new file +13
-0+/** pch_phub_ioctl - Implements the various ioctl functionalities of the Packet +* Hub module. +* @inode: Contains the reference of the inode structure +* @file: Contains the reference of the file structure +* @cmd: Contains the command value +* @arg: Contains the command argument value +*/ + + +static long pch_phub_ioctl (struct file *file,unsigned int cmd, +unsigned long arg) +{ +__s32 ret_value =0 ;drivers/char/pch_phub/pch_phub.c line 646
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+struct pch_phub_reqt __user *p_pch_phub_reqt; +__u32 data; +__u32 mac_addr[2 ]; +__s32 ret; +__u32 i; +void __user *varg = (void __user *)arg; + + ret =mutex_trylock (&pch_phub_ioctl_mutex); +if (ret ==0 ) + goto return_ioctrl;/*Can't get mutex lock*/
mutex_trylockis probably not what you want, it returns immediately when there is another function in the kernel.mutex_lock_interruptibleseems more appropriate, it will block until the mutex is free or the process gets sent a signal, which you should handle by returning-ERESTARTSYS.In either case, you must not jump to
return_ioctrlhere, because that will try to release the mutex that you do not hold here, causing a hang the next time you enter the function.drivers/char/pch_phub/pch_phub.c line 656
Quoted 9 lines
+ if (pch_phub_reg.pch_phub_suspended == true) {
+ret_value=-EPERM;
+ gotoreturn_ioctrl;
+ }drivers/char/pch_phub/pch_phub.c new file +5
-0+ + p_pch_phub_reqt = (struct pch_phub_reqt *)varg; + +if (ret_value) + goto return_ioctrl;is always zero here.
Quoted 9 lines
+ /* End of Access area check */
drivers/char/pch_phub/pch_phub.c new file +8
-0+ + +switch (cmd) { + +case IOCTL_PHUB_READ_MAC_ADDR : + mac_addr[0 ] =0 ; + mac_addr[1 ] =0 ; +for (i =0 ; i <4 ; i++) {drivers/char/pch_phub/pch_phub.c line 676
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+pch_phub_read_gbe_mac_addr (i, (__u8 *)&data); + mac_addr[0 ] |= data << i*8 ; + } +for (; i <6 ; i++) { +pch_phub_read_gbe_mac_addr (i, (__u8 *)&data); + mac_addr[1 ] |= data << (i-4 )*8 ; + } + + ret_value =copy_to_user ((void *)&p_pch_phub_reqt->data, + (void *)mac_addr,sizeof (mac_addr));
p_pch_phub_reqt->datahas multiple problems:- You have the typecast to (void *), which is wrong and unneeded.
- The data structure has no point at all any more when you use only one field.Just make this
u8 mac_addr[6];
for (i = 0; i < 4; i++)pch_phub_read_gbe_mac_addr(i, &mac_addr[i]);ret_value=copy_to_user(varg, mac_addr, sizeof(mac_addr));Quoted 7 lines
drivers/char/pch_phub/pch_phub.h new file +7
-0+#define PHUB_IOCTL_MAGIC (0xf7) + +/*Outlines the read register function signature. */ +#define IOCTL_PHUB_READ_REG (_IOW(PHUB_IOCTL_MAGIC, 1, __u32)) + +/*Outlines the write register function signature. */ +#define IOCTL_PHUB_WRITE_REG (_IOW(PHUB_IOCTL_MAGIC, 2, __u32))drivers/char/pch_phub/pch_phub.h line 48
Quoted 10 lines
drivers/char/pch_phub/pch_phub.h new file +10
-0+ +/*Outlines the read, modify and write register function signature. */ +#define IOCTL_PHUB_READ_MODIFY_WRITE_REG (_IOW(PHUB_IOCTL_MAGIC, 3,\ + __u32)) + +/*Outlines the read option rom function signature. */ +#define IOCTL_PHUB_READ_OROM (_IOW(PHUB_IOCTL_MAGIC, 4, __u32)) + +/*Outlines the write option rom function signature. */ +#define IOCTL_PHUB_WRITE_OROM (_IOW(PHUB_IOCTL_MAGIC, 5, __u32))These should all get removed now.
drivers/char/pch_phub/pch_phub.h line 54
Quoted 5 lines
drivers/char/pch_phub/pch_phub.h new file +5
-0+/*Outlines the read mac address function signature. */ +#define IOCTL_PHUB_READ_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 6, __u32)) + +/*brief Outlines the write mac address function signature. */ +#define IOCTL_PHUB_WRITE_MAC_ADDR (_IOW(PHUB_IOCTL_MAGIC, 7, __u32))
IOCTL_PHUB_READ_MAC_ADDRneeds_IORinstead of_IOW, and the type is still wrong here. Your code currently has structpch_phub_reqtinstead of__u32, and if you change the ioctl function as I explained above, it should become#define
IOCTL_PHUB_READ_MAC_ADDR(_IOR(PHUB_IOCTL_MAGIC, 6, __u8[6])) #defineIOCTL_PHUB_WRITE_MAC_ADDR(_IOW(PHUB_IOCTL_MAGIC, 7, __u8[6]))Arnd
Arnd Bergmann
maintainerQuoted 9 lines · Masayuki Ohtake
Quoted 2 lines
This should not be necessary. Just use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the code instead of the extraPCH_CAN_PCLK_50MHZmacro.I have a question. I show the above reason.
In case CAN is integrated as MODULE, macro name isCONFIG_PCH_CAN_PCLK_50MHZ_MODULE. On the other hand, integrated as built-in,CONFIG_PCH_CAN_PCLK_50MHZ. To prevent PHUB source code from integrated as MODULE or BUILT-IN,
we re-define macro name inMakefile.
Quoted 3 lines
If use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the source code, in case buit-in, behavior is not correct.
But in case module, behavior is not correct.
I don't understand the problem, because you have the definition
config PCH_CAN_PCLK_50MHZ
bool "CAN PCLK 50MHz"
depends on PCH_PHUB
which is 'bool', not 'tristate', so it can never be a module. If you are referring to a dependency on the CAN code that is not part of this patch, you can express this as
config PCH_CAN_PCLK_50MHZ
bool "CAN PCLK 50MHz"
depends on PCH_PHUB || CAN != "n"
This will leave CONFIG_PCH_CAN_PCLK_50MHZ as bool and let it only get enabled if CONFIG_CAN is either "y" or "m".
Does that answer your question?
Arnd
Hi Arnd,
Thank you for your comments.
I can understand your intention.
I misunderstood about Kconfig behavior.
Thanks,
Ohtake.
----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtake" <[email protected]>
Cc: "Alan Cox" <[email protected]>; "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Tuesday, June 15, 2010 7:42 PM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver
Quoted 34 lines · Masayuki Ohtake
Quoted 9 lines
Quoted 2 lines
This should not be necessary. Just use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the code instead of the extraPCH_CAN_PCLK_50MHZmacro.I have a question. I show the above reason.
In case CAN is integrated as MODULE, macro name isCONFIG_PCH_CAN_PCLK_50MHZ_MODULE. On the other hand, integrated as built-in,CONFIG_PCH_CAN_PCLK_50MHZ. To prevent PHUB source code from integrated as MODULE or BUILT-IN,
we re-define macro name inMakefile.Quoted 3 lines
If use
CONFIG_PCH_CAN_PCLK_50MHZdirectly in the source code, in case buit-in, behavior is not correct.
But in case module, behavior is not correct.I don't understand the problem, because you have the definition
config
PCH_CAN_PCLK_50MHZ
bool "CAN PCLK 50MHz"
depends onPCH_PHUBwhich is 'bool', not 'tristate', so it can never be a module. If you are referring to a dependency on the CAN code that is not part of this patch, you can express this as
config
PCH_CAN_PCLK_50MHZ
bool "CAN PCLK 50MHz"
depends onPCH_PHUB|| CAN != "n"This will leave
CONFIG_PCH_CAN_PCLK_50MHZas bool and let it only get enabled ifCONFIG_CANis either "y" or "m".
Does that answer your question?Arnd
Hi Alan,
We have modified for your comments.
Please Confirm below.
Thanks.
Ohtake.
--- drivers/char/Kconfig | 25 + drivers/char/Makefile | 4 + drivers/char/pch_phub/Makefile | 7 + drivers/char/pch_phub/pch_phub.c | 940 +++++++++++++++++++++++ drivers/char/pch_phub/pch_phub.h | 58 ++ 5 files changed, 1034 insertions(+), 0 deletions(-) create mode 100644 drivers/char/pch_phub/Makefile create mode 100755 drivers/char/pch_phub/pch_phub.c create mode 100755 drivers/char/pch_phub/pch_phub.h
drivers/char/Kconfig +25 -0
@@ -4,6 +4,31 @@ menu "Character devices" +config PCH_IEEE1588 + tristate "PCH IEEE1588" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH IEEE1588 Host controller. + +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. + This driver is for PCH Packet hub driver for Topcliff. + This driver is integrated as built-in. + This driver can access GbE MAC address. + This driver can access HW register. + You can also be integrated as module. + +config PCH_CAN_PCLK_50MHZ + bool "CAN PCLK 50MHz" + depends on PCH_PHUB + help + If you say yes to this option, clock is set to 50MHz.(For CAN control) + config VT bool "Virtual terminal" if EMBEDDED depends on !S390
drivers/char/Makefile +4 -0
@@ -111,6 +111,10 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_IEEE1588) += pch_ieee1588/ + +obj-$(CONFIG_PCH_PHUB) += pch_phub/ + # Files generated that shall be removed upon make clean clean-files := consolemap_deftbl.c defkeymap.c
drivers/char/pch_phub/Makefile new file +7 -0
@@ -0,0 +1,7 @@ +ifeq ($(CONFIG_PHUB_DEBUG),y) +EXTRA_CFLAGS += -DDEBUG +endif + +obj-$(CONFIG_PCH_PHUB) += pch_phub_drv.o + +pch_phub_drv-objs := pch_phub.o
drivers/char/pch_phub/pch_phub.c new file +940 -0
940 lines not on this page · raw message
drivers/char/pch_phub/pch_phub.h new file +58 -0
58 lines not on this page · raw message
Signature · 1 line
1.6.0.6
Arnd Bergmann
maintainerLooks almost good to me now. Some more details (only the -EFAULT return code is a real issue, the other ones are cosmetic changes):
drivers/char/pch_phub/pch_phub.c line 77
Quoted 5 lines · Masayuki Ohtak
drivers/char/pch_phub/pch_phub.c new file +5
-0+ +#define PCH_READ_REG (a) ioread32(pch_phub_reg.pch_phub_base_address + (a)) + +#define PCH_WRITE_REG (a, b) iowrite32((a), pch_phub_reg.pch_phub_base_address +\ + (b))
I'd recommend just getting rid of these abstractions. You only use them in one or two functions each, so you can just as well add a local variable there and do
void __iomem *p = pch_phub_reg.pch_phub_base_address; foo = ioread32(p + foo_offset); bar = ioread32(p + bar_offset); ...
Not really important, but this way it would be more conventional without having to write extra code.
Quoted 13 lines
drivers/char/pch_phub/pch_phub.c new file +13
-0+/*-------------------------------------------- +global variables +--------------------------------------------*/ +/** +* struct pch_phub_reg_t - PHUB register structure +* @phub_id_reg: PHUB_ID register val +* @q_pri_val_reg: QUEUE_PRI_VAL register val +* @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val +* @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val +* @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val +* @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val +* @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val +* @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val
drivers/char/pch_phub/pch_phub.c line 100
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+* @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val +* @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val +* @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val +* @int_reduce_control_reg: INT_REDUCE_CONTROL registers val +* @clkcfg_reg: CLK CFG register val +* @pch_phub_base_address: register base address +* @pch_phub_extrom_base_address: external rom base address +* @pch_phub_suspended: PHUB status val +*/ +struct pch_phub_reg_t {
It would be better to drop the _t postfix on the type name. By convention, we use this only for typedefs on simple data types like off_t but not for structures.
drivers/char/pch_phub/pch_phub.c line 103
Quoted 3 lines
drivers/char/pch_phub/pch_phub.c new file +3
-0+__u32 phub_id_reg; +__u32 q_pri_val_reg; +__u32 rc_q_maxsize_reg;
When I told you to change the ioctl arguments to use __u32 instead of u32, I was only referring to those parts that are in the user-visible section of the header file. While it does not hurt to use __u32 in the kernel, you should understand the distinction.
drivers/char/pch_phub/pch_phub.c line 545
Quoted 11 lines
drivers/char/pch_phub/pch_phub.c new file +11
-0+/** pch_phub_write - Implements the write functionality of the Packet Hub +* module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @ppos: Contains the reference of the file structure +*/ +static ssize_t pch_phub_write (struct file *file,const char __user *buf, +size_t size,loff_t *ppos) +{ +static __u32 data;
No need to make data static.
Quoted 4 lines
drivers/char/pch_phub/pch_phub.c new file +4
-0+__s32 ret_value; +__u32 addr_offset =0 ; +loff_t pos = *ppos; +
drivers/char/pch_phub/pch_phub.c line 559
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (pos <0 ) +return -EINVAL ; + +for (addr_offset =0 ; addr_offset < size; addr_offset++) { +get_user (data, &buf[addr_offset]); + + ret_value =pch_phub_write_serial_rom (0x80 + addr_offset + pos, + data); +if (ret_value) +return -EFAULT ;
You should return -EFAULT if the get_user() call fails, otherwise you have a possible security hole. If pch_phub_write_serial_rom fails, the correct return code would be -EIO.
drivers/char/pch_phub/pch_phub.c line 645
Quoted 9 lines
drivers/char/pch_phub/pch_phub.c new file +9
-0+/** +* file_operations structure initialization +*/ +static const struct file_operations pch_phub_fops = { + .owner =THIS_MODULE , + .read = pch_phub_read, + .write = pch_phub_write, + .unlocked_ioctl = pch_phub_ioctl, +};
If would be good to add a line of
.llseek = default_llseek,
here. I have a patch to change the default llseek method to one that disallows seeking, so if you add this line, there is one less driver for me to patch.
Arnd
Hi Arnd,
I will modify for your comments by today or tomorrow.
Thanks,
Ohtake
----- Original Message -----
From: "Arnd Bergmann" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: "Alan Cox" <[email protected]>; "LKML" <[email protected]>; "Andrew" <[email protected]>; "Intel OTC" <[email protected]>; "Wang, Qi" <[email protected]>; "Wang, Yong Y" <[email protected]>
Sent: Thursday, June 17, 2010 8:59 PM
Subject: Re: [PATCH] Topcliff PHUB: Generate PacketHub driver
Quoted 110 lines
Looks almost good to me now. Some more details (only the
-EFAULTreturn code is a real issue, the other ones are cosmetic changes):drivers/char/pch_phub/pch_phub.c line 77
Quoted 5 lines · Masayuki Ohtak
drivers/char/pch_phub/pch_phub.c new file +5
-0+ +#define PCH_READ_REG (a) ioread32(pch_phub_reg.pch_phub_base_address + (a)) + +#define PCH_WRITE_REG (a, b) iowrite32((a), pch_phub_reg.pch_phub_base_address +\ + (b))I'd recommend just getting rid of these abstractions. You only use them in one or two functions each, so you can just as well add a local variable there and do
void
__iomem*p = pch_phub_reg.pch_phub_base_address; foo =ioread32(p + foo_offset);
bar =ioread32(p + bar_offset);
...Not really important, but this way it would be more conventional without having to write extra code.
Quoted 13 lines
drivers/char/pch_phub/pch_phub.c new file +13
-0+/*-------------------------------------------- +global variables +--------------------------------------------*/ +/** +* struct pch_phub_reg_t - PHUB register structure +* @phub_id_reg: PHUB_ID register val +* @q_pri_val_reg: QUEUE_PRI_VAL register val +* @rc_q_maxsize_reg: RC_QUEUE_MAXSIZE register val +* @bri_q_maxsize_reg: BRI_QUEUE_MAXSIZE register val +* @comp_resp_timeout_reg: COMP_RESP_TIMEOUT register val +* @bus_slave_control_reg: BUS_SLAVE_CONTROL_REG register val +* @deadlock_avoid_type_reg: DEADLOCK_AVOID_TYPE register val +* @intpin_reg_wpermit_reg0: INTPIN_REG_WPERMIT register 0 val drivers/char/pch_phub/pch_phub.c line 100
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+* @intpin_reg_wpermit_reg1: INTPIN_REG_WPERMIT register 1 val +* @intpin_reg_wpermit_reg2: INTPIN_REG_WPERMIT register 2 val +* @intpin_reg_wpermit_reg3: INTPIN_REG_WPERMIT register 3 val +* @int_reduce_control_reg: INT_REDUCE_CONTROL registers val +* @clkcfg_reg: CLK CFG register val +* @pch_phub_base_address: register base address +* @pch_phub_extrom_base_address: external rom base address +* @pch_phub_suspended: PHUB status val +*/ +struct pch_phub_reg_t {It would be better to drop the _t postfix on the type name. By convention, we use this only for typedefs on simple data types like
off_tbut not for structures.Quoted 3 lines
+
__u32phub_id_reg;
+__u32q_pri_val_reg;
+__u32rc_q_maxsize_reg;When I told you to change the ioctl arguments to use
__u32instead of u32, I was only referring to those parts that are in the user-visible section of the header file. While it does not hurt to use__u32in the kernel, you should understand the distinction.drivers/char/pch_phub/pch_phub.c line 545
Quoted 11 lines
drivers/char/pch_phub/pch_phub.c new file +11
-0+/** pch_phub_write - Implements the write functionality of the Packet Hub +* module. +* @file: Contains the reference of the file structure +* @buf: Usermode buffer pointer +* @size: Usermode buffer size +* @ppos: Contains the reference of the file structure +*/ +static ssize_t pch_phub_write (struct file *file,const char __user *buf, +size_t size,loff_t *ppos) +{ +static __u32 data;No need to make data static.
Quoted 4 lines
+
__s32ret_value;
+__u32addr_offset= 0;
+loff_tpos = *ppos;drivers/char/pch_phub/pch_phub.c new file +1
-0+drivers/char/pch_phub/pch_phub.c line 559
Quoted 10 lines
drivers/char/pch_phub/pch_phub.c new file +10
-0+if (pos <0 ) +return -EINVAL ; + +for (addr_offset =0 ; addr_offset < size; addr_offset++) { +get_user (data, &buf[addr_offset]); + + ret_value =pch_phub_write_serial_rom (0x80 + addr_offset + pos, + data); +if (ret_value) +return -EFAULT ;You should return
-EFAULTif theget_user()call fails, otherwise you have a possible security hole. Ifpch_phub_write_serial_romfails, the correct return code would be-EIO.drivers/char/pch_phub/pch_phub.c line 645
Quoted 9 lines
drivers/char/pch_phub/pch_phub.c new file +9
-0+/** +* file_operations structure initialization +*/ +static const struct file_operations pch_phub_fops = { + .owner =THIS_MODULE , + .read = pch_phub_read, + .write = pch_phub_write, + .unlocked_ioctl = pch_phub_ioctl, +};If would be good to add a line of
.llseek =default_llseek,
here. I have a patch to change the default llseek method to one that disallows seeking, so if you add this line, there is one less driver for me to patch.Arnd
drivers/char/Kconfig line 13
Quoted 7 lines
drivers/char/Kconfig +7
-0+config PCH_IEEE1588 + tristate "PCH IEEE1588" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH IEEE1588 Host controller. +
Is this really supposed to be part of this patch?
Quoted 17 lines
drivers/char/Kconfig +17
-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. + This driver is for PCH Packet hub driver for Topcliff. + This driver is integrated as built-in. + This driver can access GbE MAC address. + This driver can access HW register. + You can also be integrated as module. + +config PCH_CAN_PCLK_50MHZ + bool "CAN PCLK 50MHz" + depends on PCH_PHUB + help + If you say yes to this option, clock is set to 50MHz.(For CAN
Quoted 2 lines
control)
+
Ohtak-san, I remember you said PCH_CAN_PCLK_50MHZ was in the patch because you used to have a hw bug. If that hw bug has been fixed, this config option shall also be removed. Otherwise, you have to build multiple kernel binaries for different hardware configurations as Alan pointed out.
drivers/char/Makefile obj-$(CONFIG_PS3_FLASH) += ps3flash.o · line 115
Quoted 10 lines
drivers/char/Makefile +2
-0@@ -111,6 +111,10 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_IEEE1588) += pch_ieee1588/ +
Ditto
Hi Yong Y
Review
Quoted 12 lines
drivers/char/Kconfig line 13
Quoted 7 lines
drivers/char/Kconfig +7
-0+config PCH_IEEE1588 + tristate "PCH IEEE1588" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH IEEE1588 Host controller. +Is this really supposed to be part of this patch?
(quoted diff) +2
-0+obj-$(CONFIG_PCH_IEEE1588) += pch_ieee1588/ +
Yes, the above is true.
Our driver under 'drivers/char' has patch order restriction. Firstly, ieee1588 must be patched.
After the above, Phub is able to be patched.
Thus, our Phub patch includes ieee1588 code.
Quoted 4 lines
Ohtak-san, I remember you said
PCH_CAN_PCLK_50MHZwas in the patch because you used to have a hw bug. If that hw bug has been fixed, this config option shall also be removed. Otherwise, you have to build multiple kernel binaries for different hardware configurations as Alan pointed out.
Sorry, I gave wrong information to you.
Our CAN HW spec is that works only 50MHz.(Not HW bug) According to our CAN hw spec, we will delete 'PCH_CAN_PCLK_50MHZ'.
Thanks,
Ohtake
----- Original Message -----
From: "Wang, Yong Y" <[email protected]>
To: "Masayuki Ohtak" <[email protected]>
Cc: "LKML" <[email protected]>; "Khor, Andrew Chih Howe" <[email protected]>; "Clark, Joel" <[email protected]>; "Wang, Qi" <[email protected]>
Sent: Friday, June 18, 2010 5:08 PM
Subject: RE: [PATCH] Topcliff PHUB: Generate PacketHub driver
drivers/char/Kconfig line 13
Quoted 7 lines
drivers/char/Kconfig +7
-0+config PCH_IEEE1588 + tristate "PCH IEEE1588" + depends on PCI + help + If you say yes to this option, support will be included for the + PCH IEEE1588 Host controller. +
Is this really supposed to be part of this patch?
Quoted 17 lines
drivers/char/Kconfig +17
-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. + This driver is for PCH Packet hub driver for Topcliff. + This driver is integrated as built-in. + This driver can access GbE MAC address. + This driver can access HW register. + You can also be integrated as module. + +config PCH_CAN_PCLK_50MHZ + bool "CAN PCLK 50MHz" + depends on PCH_PHUB + help + If you say yes to this option, clock is set to 50MHz.(For CAN
Quoted 2 lines
control)
+
Ohtak-san, I remember you said PCH_CAN_PCLK_50MHZ was in the patch because you used to have a hw bug. If that hw bug has been fixed, this config option shall also be removed. Otherwise, you have to build multiple kernel binaries for different hardware configurations as Alan pointed out.
drivers/char/Makefile obj-$(CONFIG_PS3_FLASH) += ps3flash.o · line 115
Quoted 10 lines
drivers/char/Makefile +2
-0@@ -111,6 +111,10 @@ obj-$(CONFIG_PS3_FLASH) += ps3flash.o obj-$(CONFIG_JS_RTC) += js-rtc.o js-rtc-y = rtc.o +obj-$(CONFIG_PCH_IEEE1588) += pch_ieee1588/ +
Ditto