Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

[Arnd Bergmann <arnd@arndb.de>]

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