Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

[Arnd Bergmann <arnd@arndb.de>]

On Monday 14 June 2010, Masayuki Ohtak wrote:
> Hi we have modified for your comments.
> Please Confirm below.

Looks much better. A few more comments about the new code:

> +#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.
> +
> +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.


> +/*--------------------------------------------
> +	internal function prototypes
> +--------------------------------------------*/
> +static __s32 __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 __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.

> +/** 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);
> +	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.

> +/** 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],
> +							(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.

> +
> +/** 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);
> +	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.

> +/** 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
> + */
> +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.

> +/** 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;
> +	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.

> +	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.

> +	/* 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++) {
> +			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));

> +#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))
> +
> +/*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.

> +/*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