Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2]

[Masayuki Ohtake]

Hi Arnd

I have found the following my english includes mistake.
> Usermode application doesn't only set HW register offset value but also set function.

I show corrected sentence.
 Usermode application doesn't set HW register offset value but set function.

Thanks,
Ohtake

----- Original Message ----- 
From: "Masayuki Ohtake" <masa-korg@xxxxxxxxxxxxxxx>
To: "Arnd Bergmann" <arnd@xxxxxxxx>
Cc: "Alan Cox" <alan@xxxxxxxxxxxxxxxxxxx>; "LKML" <linux-kernel@xxxxxxxxxxxxxxx>; <andrew.chih.howe.khor@xxxxxxxxx>;
"Wang, Qi" <qi.wang@xxxxxxxxx>; "Wang, Yong Y" <yong.y.wang@xxxxxxxxx>
Sent: Friday, June 11, 2010 12:18 PM
Subject: Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2]


> Hi Arnd
>
> Thank you for your comment.
> Please refer the following mail in line.
> ----- Original Message ----- 
> From: "Arnd Bergmann" <arnd@xxxxxxxx>
> To: "Masayuki Ohtake" <masa-korg@xxxxxxxxxxxxxxx>
> Cc: "Wang, Yong Y" <yong.y.wang@xxxxxxxxx>; "Wang, Qi" <qi.wang@xxxxxxxxx>; <andrew.chih.howe.khor@xxxxxxxxx>; "LKML"
> <linux-kernel@xxxxxxxxxxxxxxx>; "Alan Cox" <alan@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, June 09, 2010 10:16 PM
> Subject: Re: [PATCH] Topcliff PHUB: Add The Packet Hub driver [1/2]
>
>
> > On Wednesday 09 June 2010, Masayuki Ohtake wrote:
> >
> > > We have added our comment for your email in line.
> >
> > ok, thanks for the update
> >
> > > If you have any question, let me know.
> >
> > One small comment from me, plus more background on the ioctl:
> >
> > > > The definition of struct pch_phub_reqt is problematic because
> > > > it contains members of type 'unsigned long'. This means that
> > > > a 32 bit user process uses a different data structure than a
> > > > 64 bit kernel.
> > > >
> > > > Ideally, you only pass a single integer as an ioctl argument.
> > > > There are cases where it needs to be a data structure, but
> > > > if that happens, you should only use members types like __u32
> > > > or __u64, not long or pointer.
> > > We have defined as u32 type.
> >
> > Note that public data headers should use '__u32' instead of 'u32',
> > because the 'u32' type is not available in the exported version
> > of linux/types.h.
>
> We will replace like below.
> u32 --> __u32
> s32 --> __s32
> s8 --> __s8
>
> >
> > > > My feeling is that this ioctl interface is too
> > > > low-level in general. You only export access to specific
> > > > registers, not to functionality exposed by them.
> > > > The best kernel interfaces are defined in a way that
> > > > is independent of the underlying hardware and
> > > > convert generic commands into device specific commands.
> > > >
> > > > If you really want to allow direct register access,
> > > > a simpler way would be to map the memory into the user
> > > > address space using the mmap() operation and not
> > > > provide any ioctl commands.
> > > Packet Hub has function accessing many resisters.
> > > Thus, we think current spec is better.
> >
> > Getting the interface right is by far the most important
> > part in a driver submission, and often the hardest part.
> >
> > Giving register-level hardware access to random user space
> > applications for the PHUB essentially means that you
> > are forcing the hardware design to become stable for all
> > eternity because all tools that use this interface (whether
> > written by you or by other people) need to keep working on
> > future generations of the hardware as well.
> >
> > This is generally considered a very bad idea, so I think
> > you should drop the IOCTL_PHUB_{READ,WRITE,MODIFY}_REG
> > ioctl commands for now if you want to get your driver
> > merged quickly, and then we can find a way to do what
> > it's meant for in a better way.
> >
> > Regarding the other ioctl commands, I still have a few
> > comments that I did not mention at first:
> >
> > >+int pch_phub_ioctl(struct inode *inode, struct file *file,
> > >+ unsigned int cmd, unsigned long arg)
> > >+{
> > >+ int ret_value = 0;
> > >+ struct pch_phub_reqt *p_pch_phub_reqt;
> > >+ unsigned long addr_offset;
> > >+ unsigned long data;
> > >+ unsigned long mask;
> > >+
> > >+ if (pch_phub_suspended == true) {
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : "
> > >+ "suspend initiated returning =%d\n", -EPERM);
> > >+ ret_value = -EPERM;
> > >+ goto return_ioctrl;
> > >+ }
> > >+
> > >+ p_pch_phub_reqt = (struct pch_phub_reqt *)arg;
> > >+ ret_value = copy_from_user((void *)&addr_offset,
> > >+ (void *)&p_pch_phub_reqt->addr_offset,
> > >+ sizeof(addr_offset));
> > >+ if (ret_value) {
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : "
> > >+ "copy_from_user fail returning =%d\n", -EFAULT);
> > >+ ret_value = -EFAULT;
> > >+ goto return_ioctrl;
> > >+ }
> >
> > The pch_phub_reqt data structure does not really make sense for
> > commands other than IOCTL_PHUB_READ_MODIFY_WRITE_REG and causes
> > more problems, see below.
> We will delete almost ioctl functions.
> Phub ioctl has only MAC Address access.
> (OROM access will be moved to read/write method.)
>
> And we will change ioctl I/F like below.
> Usermode application doesn't only set HW register offset value but also set function.
>
> >
> > Moreover, the type casts are incorrect because they are
> > missing the __user address space modifier. The casts can
> > simply be removed, and the variable declared as
> > e.g. 'struct pch_phub_reqt __user *p_pch_phub_reqt;'.
> We will add '__user'
>
> >
> > I'd recommend using the 'sparse' tool to check your
> > source code to find problems like this. Simply install
> > sparse and run
> >
> > make C=1 drivers/char/pch_phub/
> >
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : "
> > >+ "copy_from_user returns =%d\n", ret_value);
> We will use  'sparse' tool .
>
> >
> > The 'dev_dbg' variable does not seem to get initialized anywhere
> > in the code. In a clean driver, it should not be a global variable
> > anyway but refer to the object that you are operating on, e.g.
> > the pci device that has been opened.
> We will modify 'dev_dbg' is passed as a function parameter.
>
> >
> > >+ switch (cmd) {
> > >+ case IOCTL_PHUB_READ_REG:
> >
> > >+ case IOCTL_PHUB_WRITE_REG:
> >
> > >+ case IOCTL_PHUB_READ_MODIFY_WRITE_REG:
> >
> >
> > As mentioned, I'd just remove these commands for now. When a specific
> > functionality is needed, you can always add another ioctl command
> > or find another way to do it.
> Yes, we will delete the above.
>
> >
> > >+ case IOCTL_PHUB_READ_OROM:
> > >+ ret_value = pch_phub_read_serial_rom(addr_offset,
> > >+ (unsigned char *)&data);
> > >+ if (ret_value) {
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : Invoked "
> > >+ "pch_phub_read_serial_rom =%d\n", -EFAULT);
> > >+ ret_value = -EFAULT;
> > >+ break;
> > >+ } else {
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : Invoked "
> > >+ "pch_phub_read_serial_rom successfully\n");
> > >+ }
> > >+ ret_value = copy_to_user((void *)&p_pch_phub_reqt->data,
> > >+ (void *)&data, sizeof(data));
> > >+ if (ret_value) {
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : "
> > >+ "copy_to_user fail returning =%d\n", -EFAULT);
> > >+ ret_value = -EFAULT;
> > >+ break;
> > >+ }
> > >+ break;
> >
> > This is a very indirect way to access a rom. I'd recommend doing
> > read and write file operations for this instead of going through the
> > ioctl. When you do that, you can simply do 'cat /dev/topcliff-phub > romdump'
> > to read the entire contents, or have other code access the contents
> > bytewise. If teh OROM contents are specific to the ethernet function,
> > you may also just implement the ethtool operations for it, as shown below.
> We will try implement as read/write method.
>
> >
> > >+ case IOCTL_PHUB_READ_MAC_ADDR:
> > >+ pch_phub_read_gbe_mac_addr(addr_offset, (unsigned char *)&data);
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : Invoked "
> > >+ "pch_phub_read_gbe_mac_addr successfully\n");
> > >+
> > >+ ret_value = copy_to_user((void *)&p_pch_phub_reqt->data,
> > >+ (void *)&data, sizeof(data));
> > >+ if (ret_value) {
> > >+ dev_dbg(dev_dbg, "pch_phub_ioctl : copy_to_user fail "
> > >+ "returning =%d\n", -EFAULT);
> > >+ ret_value = -EFAULT;
> > >+ break;
> > >+ }
> > >+ break;
> >
> > I believe this really belongs into the ethernet driver, using the
> > ethtool_ops->get_eeprom operation as other ethernet drivers do the
> > same thing.
> MAC address is under Phub HW.
> To keep driver dependency, I think, Phub should take charge of MAC address access.
>
> Thanks,
> Ohtake
>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/