Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCH] Topcliff PHUB: Generate PacketHub driver | Date | Thu, 17 Jun 2010 13:59:37 +0200 |
| |
Looks almost good to me now. Some more details (only the -EFAULT return code is a real issue, the other ones are cosmetic changes):
On Thursday 17 June 2010, Masayuki Ohtak wrote: > + > +#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.
> +/*-------------------------------------------- > + 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 > + * @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.
> + __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.
> +/** 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.
> + __s32 ret_value; > + __u32 addr_offset = 0; > + loff_t pos = *ppos; > + > + 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.
> +/** > + * 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
| |