Alan,
>Ok we now have two devices and they have differing APIs and
>own device names and both from the same company. This has to
>stop right now and the existing one wants sorting out
>accordingly (while you are it fix the fact any user can blow
>the kernel log away in that one by issuing bogus ioctls at it,
>thats not a good thing)
>
>NAK to this in its current form.
>
>If we are going to have multiple nfc devices (and we are) then
>we need a /dev/nfc%d device range to dump them in and we need
>some API commonality.
>
>Your API seems fairly sane (except your nfc-microread.txt
>needs to document or point properly to the HCI messages in question
>
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset
>the hardware.
>
>And a reset is a generic sort of interface so we should
>probably have NFC_IOC_RESET to go with /dev/nfc%d naming.
I see your point and agree with your and Arnd's opinion on that concept. Will, then, think how to approach to this and try to propose a resonable skeleton first.
>> + if (microread_is_busy(info)) {
>> + dev_err(&client->dev, "%s: info %p: device is
>busy.", __func__,
>> + info);
>
>So a process spinning trying to open it can spew all over the
>log - looks bogus to me (similar problems in the existing driver)
All potential spewing logs have been removed.
>
>What is the intended behaviour on a reset while I am polling ?
Good question :|. I will answare soon.
>
>Ermm nope.. why do we have do nothing ioctls ?
>
onfc stack requires those ones, but they are only valid for a specific test enviroment.
This should not be a case for driver and the stack should care about it if it needs this. Then will remove it.
>> + if (irq != client->irq)
>> + return IRQ_NONE;
>
>How can this occur - why is this test needed ????
It's not needed. Removed.
>> +
>> + mutex_lock(&info->rx_mutex);
>> + info->irq_state = 1;
>> + mutex_unlock(&info->rx_mutex);
>
>Would it not be lighter to use atomic bit ops ?
Do you mean in order to remove rx_mutex?
mutex_lock(&info->rx_mutex);
atomic_set(info->irq_state ,1);
mutex_unlock(&info->rx_mutex);
looks a bit strange. I still need the rx_mutex to protect irq_state while reading i2c.
mutex_lock(&info->rx_mutex);
ret = i2c_master_recv(client, info->buf, info->buflen);
info->irq_state = 0;
mutex_unlock(&info->rx_mutex);
>> + info->mdev.minor = MISC_DYNAMIC_MINOR;
>> + info->mdev.name = MICROREAD_DEV_NAME;
>> + info->mdev.fops = µread_fops;
>> + info->mdev.parent = &client->dev;
>> +
>> + ret = misc_register(&info->mdev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "%s: register chr dev
>failed (ret %d)",
>> + __func__, ret);
>> + goto free_irq;
>> + }
>
>And at this point you want a thing to hand out nfc%d names not
>to use misc device with random per device API. The same app
>ought to be able to work with many nfc readers.
Sure. Will fix this as well.
>> +static int microread_suspend(struct i2c_client *client,
>pm_message_t
>> +mesg) {
>> + return -ENOSYS;
>> +}
>> +
>> +static int microread_resume(struct i2c_client *client) {
>> + return -ENOSYS;
>> +}
>
>So why provide them ??
I've supposed to implement later on (need more hw specific input on that topic). Anyway...
I can add this when it's completed at all. Removed for now.
>-----Original Message-----
>From: Alan Cox [mailto:alan@xxxxxxxxxxxxxxxxxxx]
>Sent: Friday, March 18, 2011 12:04 PM
>To: Rymarkiewicz Waldemar
>Cc: linux-i2c@xxxxxxxxxxxxxxx; arnd@xxxxxxxx;
>sameo@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>hthebaud@xxxxxxxxxxxx; matti.j.aaltonen@xxxxxxxxx
>Subject: Re: [PATCH] NFC: Driver for Inside Secure MicroRead NFC chip
>
>On Fri, 18 Mar 2011 11:40:24 +0100
>Waldemar Rymarkiewicz <waldemar.rymarkiewicz@xxxxxxxxx> wrote:
>
>> Add new driver for MicroRead NFC chip connected to i2c bus.
>
>Ok we now have two devices and they have differing APIs and
>own device names and both from the same company. This has to
>stop right now and the existing one wants sorting out
>accordingly (while you are it fix the fact any user can blow
>the kernel log away in that one by issuing bogus ioctls at it,
>thats not a good thing)
>
>NAK to this in its current form.
>
>If we are going to have multiple nfc devices (and we are) then
>we need a /dev/nfc%d device range to dump them in and we need
>some API commonality.
>
>Your API seems fairly sane (except your nfc-microread.txt
>needs to document or point properly to the HCI messages in question
>
>> +The application can use ioctl(MICROREAD_IOC_RESET)to reset
>the hardware.
>
>And a reset is a generic sort of interface so we should
>probably have NFC_IOC_RESET to go with /dev/nfc%d naming.
>
>> +static int microread_open(struct inode *inode, struct file *file) {
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + int ret = 0;
>> +
>> + dev_vdbg(&client->dev, "%s: info: %p", __func__, info);
>> +
>> + mutex_lock(&info->mutex);
>> +
>> + if (microread_is_busy(info)) {
>> + dev_err(&client->dev, "%s: info %p: device is
>busy.", __func__,
>> + info);
>
>So a process spinning trying to open it can spew all over the
>log - looks bogus to me (similar problems in the existing driver)
>
>
>
>> + if (count > info->buflen) {
>> + dev_err(&client->dev, "%s: no enough space in
>read buffer.",
>> + __func__);
>> + ret = -ENOMEM;
>> + goto done;
>
>More bogus log spewing and an odd error code for good measure
>
>> + lrc = calc_lrc(info->buf, len + 1);
>> + if (lrc != info->buf[len + 1]) {
>> + dev_err(&client->dev, "%s: incorrect i2c
>frame.", __func__);
>> + ret = -EFAULT;
>> + goto done;
>
>So I can also spew all over the log by putting a deliberately
>busted sender next to it.
>
>> + }
>> +
>> + ret = len + 2;
>> +
>> + if (copy_to_user(buf, info->buf, len + 2)) {
>> + dev_err(&client->dev, "%s: error copying to
>user.", __func__);
>> + ret = -EFAULT;
>
>And another one.
>
>> +static ssize_t microread_write(struct file *file, const
>char __user *buf,
>> + size_t count, loff_t *f_pos)
>> +{
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + int ret;
>> + u16 len;
>> +
>> + dev_dbg(&client->dev, "%s: info: %p, size %d (bytes).",
>__func__,
>> + info, count);
>> +
>> + if (count > info->buflen) {
>> + dev_err(&client->dev, "%s: no enought space in
>TX buffer.",
>> + __func__);
>> + return -EINVAL;
>> + }
>
>And another
>
>> +
>> + len = min_t(u16, count, info->buflen);
>> +
>> + mutex_lock(&info->mutex);
>> + if (copy_from_user(info->buf, buf, len)) {
>> + dev_err(&client->dev, "%s: error copying from user.",
>> + __func__);
>
>Etc - these all want cleaning up
>
>
>> +static unsigned int microread_poll(struct file *file, poll_table
>> +*wait) {
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + int ret = (POLLOUT | POLLWRNORM);
>> +
>> + dev_vdbg(&client->dev, "%s: info: %p client %p", __func__, info,
>> + client);
>> +
>> + mutex_lock(&info->mutex);
>> + poll_wait(file, &info->rx_waitq, wait);
>> +
>> + mutex_lock(&info->rx_mutex);
>> + if (info->irq_state)
>> + ret |= (POLLIN | POLLRDNORM);
>> + mutex_unlock(&info->rx_mutex);
>> + mutex_unlock(&info->mutex);
>
>What is the intended behaviour on a reset while I am polling ?
>
>> +static long microread_ioctl(struct file *file, unsigned int cmd,
>> +
>unsigned long arg)
>> +{
>> + struct microread_info *info = container_of(file->private_data,
>> + struct microread_info, mdev);
>> + struct i2c_client *client = info->i2c_dev;
>> + struct microread_nfc_platform_data *pdata =
>> + dev_get_platdata(&client->dev);
>> + int ret = 0;
>> +
>> + dev_dbg(&client->dev, "%s: info: %p cmd %d", __func__,
>info, cmd);
>> +
>> + mutex_lock(&info->mutex);
>> +
>> + switch (cmd) {
>> + case MICROREAD_IOC_CONFIGURE:
>> + case MICROREAD_IOC_CONNECT:
>> + goto done;
>
>Ermm nope.. why do we have do nothing ioctls ?
>
>> + case MICROREAD_IOC_RESET:
>> + microread_reset_hw(pdata);
>
>> + default:
>> + dev_err(&client->dev, "%s; not supported ioctl
>0x%x", __func__,
>> + cmd);
>
>And more spewage
>
>
>> +static irqreturn_t microread_irq(int irq, void *dev) {
>> + struct microread_info *info = dev;
>> + struct i2c_client *client = info->i2c_dev;
>> +
>> + dev_dbg(&client->dev, "irq: info %p client %p ", info, client);
>> +
>> + if (irq != client->irq)
>> + return IRQ_NONE;
>
>How can this occur - why is this test needed ????
>
>> +
>> + mutex_lock(&info->rx_mutex);
>> + info->irq_state = 1;
>> + mutex_unlock(&info->rx_mutex);
>
>Would it not be lighter to use atomic bit ops ?
>
>
>> + info->mdev.minor = MISC_DYNAMIC_MINOR;
>> + info->mdev.name = MICROREAD_DEV_NAME;
>> + info->mdev.fops = µread_fops;
>> + info->mdev.parent = &client->dev;
>> +
>> + ret = misc_register(&info->mdev);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "%s: register chr dev
>failed (ret %d)",
>> + __func__, ret);
>> + goto free_irq;
>> + }
>
>And at this point you want a thing to hand out nfc%d names not
>to use misc device with random per device API. The same app
>ought to be able to work with many nfc readers.
>
>> +static int microread_suspend(struct i2c_client *client,
>pm_message_t
>> +mesg) {
>> + return -ENOSYS;
>> +}
>> +
>> +static int microread_resume(struct i2c_client *client) {
>> + return -ENOSYS;
>> +}
>
>So why provide them ??
>
>
>Alan
>--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
