RE: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver

quoted

+#define SIZE_OF_FIFO (512*1024)
+
+static u8 message_fifo[ISA_DEVICES][SIZE_OF_FIFO];

Thats a huge amount of static memory that gets allocated regardless of
whether the device is open or being used ?

Yes, but all of these will be used, in any system which includes APE-Modem
communication.

quoted

+static int major;
+module_param(major, int, 0);
+MODULE_PARM_DESC(major, "Major device number");

Is this really needed still - it looks like an escaped debugging aid

Yup, will remove this:-)

quoted

+/* global fops mutex */
+static DEFINE_MUTEX(isa_lock);
+
+/**
+ * shrm_get_cdev_index() - return the index mapped to l2 header
+ * @l2_header:     L2 header
+ *
+ * struct map_device maps the index(count) with the device L2

header.

quoted

+ * This function returns the index for the provided L2 header in

case

quoted

+ * of success else -ve value.
+ */
+int shrm_get_cdev_index(u8 l2_header)
+{
+   u8 cnt;
+   for (cnt = 0; cnt < ISA_DEVICES; cnt++) {
+           if (map_dev[cnt].l2_header == l2_header)
+                   return map_dev[cnt].idx;
+   }
+   return -EINVAL;
+}

Would be good to document the locking assumptions on this lot

Sure will add a note on this locking mechanism.

quoted

+           /* reset the msg queue pointers */
+           q->size = SIZE_OF_FIFO;
+           q->readptr = 0;
+           q->writeptr = 0;

Is there a reason for not using the existing kfifo layer for this work
or
does it need to handle other things kfifo cannot.

Basically it doesn't suit our protocol of having base addr, read/write
pointer, locking etc as the same set of structures and protocol will be
used on the modem side implementation.

quoted

+static u32 isa_select(struct file *filp,
+                           struct poll_table_struct *wait)
+{
+   struct isadev_context *isadev = filp->private_data;
+   struct shrm_dev *shrm = isadev->dl_queue.shrm;
+   struct message_queue *q;
+   u32 mask = 0;
+   u32 m = iminor(filp->f_path.dentry->d_inode);
+   u8 idx = shrm_get_cdev_index(m);
+
+   dev_dbg(shrm->dev, "%s IN\n", __func__);
+
+   if (shrm->msr_flag)
+           return -ENODEV;

This test has no associated locking. Please explain what stop sit
becoming true during the rest of this function ?

Locking in this file has been used only for protecting the handling
of queues.
When modem is reset, due to some crash or some unknown event, in
that case, this flag will be set and we are not suppose to communicate
any further as modem is in reset state.

+     struct shrm_dev *shrm = isadev->dl_queue.shrm;

quoted

+   struct message_queue *q;
+   u32 msgsize;
+
+   dev_dbg(shrm->dev, "%s IN\n", __func__);
+
+   if (len <= 0)
+           return -EFAULT;

How can this occur ?

Check for error condition

quoted

+   spin_lock_bh(&q->update_lock);
+   if (list_empty(&q->msg_list)) {
+           spin_unlock_bh(&q->update_lock);
+           dev_dbg(shrm->dev, "Waiting for Data\n");
+           if (wait_event_interruptible(q->wq_readable,
+                           atomic_read(&q->q_rp) == 1))
+                   return -ERESTARTSYS;
+   } else
+           spin_unlock_bh(&q->update_lock);
+
+   if (shrm->msr_flag) {
+           atomic_set(&q->q_rp, 0);
+           return -ENODEV;
+   }
+
+   msgsize = get_size_of_new_msg(q);
+
+   if (len < msgsize)
+           return -EINVAL;

What happens with two parallel reads - I don't see what prevents
corruption if that occurs or one racing read freeing the message before
another has finished processing it.

Two parallel reads for different L2 headers can happen, but within the
same L2 header is out of the scope. Since the client using this in
user space will not know about the message. i.e which msg is for which
client. Hence so that scenario is not considered.

quoted

+
+   if ((q->readptr+msgsize) >= q->size) {

Minor style comment - you seen to randomly switch between a+b and a + b
styles 8)

Will correct it.

quoted

+           size = (q->size-q->readptr);
+           /* Copy First Part of msg */
+           if (copy_to_user(psrc,
+                           (u8 *)(q->fifo_base+q->readptr),
+                           size)) {
+                   dev_err(shrm->dev, "copy_to_user failed\n");
+                   return -EFAULT;
+           }
+           psrc += size;
+           /* Copy Second Part of msg at the top of fifo */
+           if (copy_to_user(psrc,
+                           (u8 *)(q->fifo_base),
+                           (msgsize-size))) {
+                   dev_err(shrm->dev, "copy_to_user failed\n");
+                   return -EFAULT;
+           }
+   } else {
+           if (copy_to_user(buf,
+                           (u8 *)(q->fifo_base + q->readptr),
+                           msgsize)) {
+                   dev_err(shrm->dev, "copy_to_user failed\n");
+                   return -EFAULT;
+           }

Pedantically you should return the number of bytes successfully copied
if
you consume data. As this code stands it's not a big deal as far as I
can
see since you don't remove the message in this situation. I suspect
that
may need to change ?

In error case, if the number of messages copied are returned, there is
no way to communicate the error and hence only that partial message
will be considered and on decoding that message, will be a flaw.

quoted

+/**
+ * isa_write() - Write to shrm char device
+ * @filp:  file descriptor
+ * @buf:   user buffer pointer
+ * @len:   size of requested data transfer
+ * @ppos:  not used
+ *
+ * It checks if there is space available in queue, and copies the

message

quoted

+ * inside queue. If there is no space, it blocks until space becomes

available.

quoted

+ * It also schedules transfer thread to transmit the newly added

message.

quoted

+ */
+ssize_t isa_write(struct file *filp, const char __user *buf,
+                            size_t len, loff_t *ppos)
+{
+   struct isadev_context *isadev = filp->private_data;
+   struct shrm_dev *shrm = isadev->dl_queue.shrm;
+   struct message_queue *q;
+   void *addr = 0;
+   int err, l2_header;
+   int ret = 0;
+
+   dev_dbg(shrm->dev, "%s IN\n", __func__);
+
+   if (len <= 0 || buf == NULL)
+           return -EFAULT;

len < 0 cannot occur, buf == NULL is not an error

Error handling is for what which is not expected.

quoted

+   q = &isadev->dl_queue;
+   l2_header = shrm_get_cdev_l2header(isadev->device_id);
+   if (l2_header < 0) {
+           dev_err(shrm->dev, "failed to get L2 header\n");
+           return l2_header;
+   }
+
+   switch (l2_header) {

Why keep going through switches and lookup tables - can't you cache
this
in the isadev or in some file private data ?

This is a write request coming from user space, hence the identification
has to be done based on the write request.

quoted

+   if (copy_from_user(addr, buf, len)) {
+           dev_err(shrm->dev, "copy_from_user failed\n");

This allows an user to fill the logs with crap - should be dev_dbg

Also what happens on two parallel writers ?

Explained above.

quoted

+/**
+ * isa_ioctl() - To handle different ioctl commands supported by

driver.

quoted

+ * @inode: structure is used by the kernel internally to

represent files

quoted

+ * @filp:  file descriptor pointer
+ * @cmd:   ioctl command
+ * @arg:   input param

This appears incomplete - it's just debug statements

Yes, will remove this.

quoted

+}
+/**
+ * isa_mmap() - Maps kernel queue memory to user space.
+ * @filp:  file descriptor pointer
+ * @vma:   virtual area memory structure.
+ *
+ * This function maps kernel FIFO into user space. This function
+ * shall be called twice to map both uplink and downlink buffers.

Again this appears incomplete

Will work on this.

quoted

+           dev_err(shrm->dev, "Device not opened yet\n");
+           mutex_unlock(&isa_lock);
+           return -ENODEV;
+   }
+   atomic_set(&isa_context->is_open[idx], 1);

How do you know it will always be one. Also given it's within the mutex
in all uses I can see why is it an atomic ?

As per our assumptions/protocol only one client per L2 header.

quoted

+
+   switch (m) {
+   case RPC_MESSAGING:
+           dev_info(shrm->dev, "Close RPC_MESSAGING Device\n");
+           break;
+   case AUDIO_MESSAGING:
+           dev_info(shrm->dev, "Close AUDIO_MESSAGING Device\n");
+           break;
+   case SECURITY_MESSAGING:
+           dev_info(shrm->dev, "CLose SECURITY_MESSAGING Device\n");
+           break;
+   case COMMON_LOOPBACK_MESSAGING:
+           kfree(isadev->addr);
+           dev_info(shrm->dev, "Close COMMON_LOOPBACK_MESSAGING

Device\n");

quoted

+           break;
+   case AUDIO_LOOPBACK_MESSAGING:
+           kfree(isadev->addr);
+           dev_info(shrm->dev, "Close AUDIO_LOOPBACK_MESSAGING

Device\n");

quoted

+           break;
+   case CIQ_MESSAGING:
+           kfree(isadev->addr);
+           dev_info(shrm->dev, "Close CIQ_MESSAGING Device\n");
+           break;
+   case RTC_CAL_MESSAGING:
+           dev_info(shrm->dev, "Close RTC_CAL_MESSAGING Device\n");
+           break;

kfree(NULL) is a no-op and guaranteed so, providing you see isadev-

quoted

addr

= NULL properly in the open path you can delete all of this code and
just
do
kfree(isadev->addr);
mutex_unlock(...)
return 0;

Ok, sounds good!

quoted

+   if (get_boot_state() != BOOT_DONE) {
+           dev_err(shrm->dev, "Boot is not done\n");
+           return -EBUSY;
+   }

Is it guaranteed that this is a one way path - ie a device never goes
back into BOOT state ?

No, on modem reset, everything happens from first.

quoted

+   if (!atomic_dec_and_test(&isa_context->is_open[idx])) {
+           atomic_inc(&isa_context->is_open[idx]);
+           dev_err(shrm->dev, "Device already opened\n");
+           mutex_unlock(&isa_lock);
+           return -EBUSY;

See comments earlier about the atomics

Replied above

quoted

+   }
+   isadev = &isa_context->isadev[idx];
+   if (filp != NULL)
+           filp->private_data = isadev;

How can filp be NULL ?

:-) just a error condition check

quoted

+
+   switch (m) {
+   case RPC_MESSAGING:
+           dev_info(shrm->dev, "Open RPC_MESSAGING Device\n");
+           break;
+   case AUDIO_MESSAGING:
+           dev_info(shrm->dev, "Open AUDIO_MESSAGING Device\n");
+           break;
+   case SECURITY_MESSAGING:
+           dev_info(shrm->dev, "Open SECURITY_MESSAGING Device\n");
+           break;
+   case COMMON_LOOPBACK_MESSAGING:
+           isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
+           if (!isadev->addr) {
+                   mutex_unlock(&isa_lock);
+                   return -ENOMEM;
+           }
+           dev_info(shrm->dev, "Open COMMON_LOOPBACK_MESSAGING

Device\n");

quoted

+           break;
+   case AUDIO_LOOPBACK_MESSAGING:
+           isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
+           if (!isadev->addr) {
+                   mutex_unlock(&isa_lock);
+                   return -ENOMEM;
+           }
+           dev_info(shrm->dev, "Open AUDIO_LOOPBACK_MESSAGING

Device\n");

quoted

+           break;
+   case CIQ_MESSAGING:
+           isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
+           if (!isadev->addr) {
+                   mutex_unlock(&isa_lock);
+                   return -ENOMEM;
+           }
+           dev_info(shrm->dev, "Open CIQ_MESSAGING Device\n");
+           break;
+   case RTC_CAL_MESSAGING:
+           dev_info(shrm->dev, "Open RTC_CAL_MESSAGING Device\n");
+           break;
+   };

So all of this could just be

      dev_dbg("Open device %d\n", m);
      switch (m) {
      case a
      case b
      case c
              isadev->addr = ....
              break;
      case e
      case f
              isadev->addr = NULL;
              break;
      default:
              error path

also in your error case right now if you run out of memory you don't
fix
up the open counters.

Ok sounds good!

quoted

+const struct file_operations isa_fops = {

static

quoted

+   for (no_dev = 0; no_dev < ISA_DEVICES; no_dev++) {
+           atomic_set(&isa_context->is_open[no_dev], 1);
+           device_create(isa_context->shm_class, NULL,
+                           MKDEV(MAJOR(dev_id),
+                           map_dev[no_dev].l2_header), NULL,
+                           map_dev[no_dev].name);
+   }

What happens if I open the device right here... ?

It can be opened, but nothing thereafter, since modem is not booted.

quoted

+
+   isa_context->isadev = kzalloc(sizeof
+                           (struct isadev_context)*ISA_DEVICES,
+                           GFP_KERNEL);
+   if (isa_context->isadev == NULL) {
+           dev_err(shrm->dev, "Failed to alloc memory\n");
+           return -ENOMEM;

You just leaked all the other stuff you created, left device pointers
to
invalid memory registered.

There is a reason kernel drivers use the goto type error handling - it
avoids missing these things.

Oops missed it will correct.

quoted

diff --git a/drivers/modem_shm/u8500_shm/shrm_fifo.c

b/drivers/modem_shm/u8500_shm/shrm_fifo.c

quoted

new file mode 100644
index 0000000..ad63cc4

--- /dev/null
+++ b/drivers/modem_shm/u8500_shm/shrm_fifo.c

@@ -0,0 +1,837 @@
+/*
+ * Copyright (C) ST-Ericsson SA 2010
+ *
+ * Author: Arun Murthy <arun.murthy@stericsson.com>
+ * Kumar Sanghvi for ST-Ericsson
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/modem_shm/u8500_shm/shrm.h>
+#include <linux/modem_shm/u8500_shm/shrm_driver.h>
+#include <linux/modem_shm/u8500_shm/shrm_private.h>
+#include <linux/modem_shm/u8500_shm/shrm_net.h>
+#include <linux/mfd/dbx500-prcmu.h>
+
+#define L1_BOOT_INFO_REQ   1
+#define L1_BOOT_INFO_RESP  2
+#define L1_NORMAL_MSG              3
+#define L1_HEADER_MASK             28
+#define L1_MAPID_MASK              0xF0000000
+#define CONFIG_OFFSET              8
+#define COUNTER_OFFSET             20
+#define L2_HEADER_SIZE             4
+#define L2_HEADER_OFFSET   24
+#define MASK_0_15_BIT              0xFF
+#define MASK_16_31_BIT             0xFF00
+#define MASK_16_27_BIT             0xFFF0000
+#define MASK_0_39_BIT              0xFFFFF
+#define MASK_40_55_BIT             0xFF00000
+#define MASK_8_16_BIT           0x0000FF00
+#define MSG_LEN_OFFSET          16
+#define SHRM_VER                2
+#define ca_ist_inactivity_timer 25 /*25ms */
+#define ca_csc_inactivity_timer 25 /*25ms */
+
+static u8 msg_audio_counter;
+static u8 msg_common_counter;
+
+struct fifo_write_params ape_shm_fifo_0;
+struct fifo_write_params ape_shm_fifo_1;
+struct fifo_read_params cmt_shm_fifo_0;
+struct fifo_read_params cmt_shm_fifo_1;

static or fix the naming

Done!

quoted

+struct sock *shrm_nl_sk;

Better to make globals visible than bury them in a mass of statics

(actually all of this raises a big question - what happens if you ever
need to have two modems - you are basically hardcoding one per system.
That's fine if you are *very* sure it will remain true)

Atleast as per the present protocol, two modem is not supported, and if
so in future, then the protocol will have to be affected. The interrupts
part is the major part.

quoted

+   spin_lock_irqsave(&boot_lock, flags);
+   boot_state = BOOT_INIT;

Your open method locking earlier assumes this cannot go backwards...

This can go backward, locking is done only while modifying this flag.

quoted

+           dev_info(shm_dev->dev, "Initiating Modem silent reset\n");

Seems an odd use of dev_info.. if its a bug workaround then perhaps
dev_warn, and if its not important dev_dbg ?

Would prefer dev_dbg

quoted

+DECLARE_TASKLET(shrm_sw_reset_callback, shrm_modem_reset_callback,
+           IRQ_PRCMU_MODEM_SW_RESET_REQ);
+

static

Lots of other stuff here that should be shrm_ or static...

There also seems to be places where you copy or touch the fifo memory
which was ioremapped but don't use readb/writeb/memcpy_toio etc, or
handle endianness. While endianness might be the kind of thing you can
be
sure of in some embedded platforms not using readb and the like means
the
compiler can do interesting optimisations you won't want and is
something
that will break on some architectures and potentially on others in
future.

Sure will look into that again and implement this in the next version
of the patch.

Thanks and Regards,
Arun R Murthy
--------------