Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756090Ab2HHDEp (ORCPT ); Tue, 7 Aug 2012 23:04:45 -0400 Received: from eu1sys200aog118.obsmtp.com ([207.126.144.145]:38264 "EHLO eu1sys200aog118.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751970Ab2HHDEl convert rfc822-to-8bit (ORCPT ); Tue, 7 Aug 2012 23:04:41 -0400 From: Arun MURTHY To: Alan Cox Cc: "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" , "linux-doc@vger.kernel.org" , "gregkh@linuxfoundation.org" , Sjur BRENDELAND Date: Wed, 8 Aug 2012 05:03:57 +0200 Subject: RE: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver Thread-Topic: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver Thread-Index: Ac10gwvjbIYyXX8wT9awuXZJac4r0QAC3R8g Message-ID: References: <1344322471-3640-1-git-send-email-arun.murthy@stericsson.com> <1344322471-3640-4-git-send-email-arun.murthy@stericsson.com> <20120807110156.4d0c2571@pyramind.ukuu.org.uk> In-Reply-To: <20120807110156.4d0c2571@pyramind.ukuu.org.uk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: acceptlanguage: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 17654 Lines: 565 > > +#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. > > > +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:-) > > > +/* 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. > > + * This function returns the index for the provided L2 header in > case > > + * 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. > > > + /* 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. > > > +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; > > + 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 > > + 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. > > > + > > + 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. > > + 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. > > +/** > > + * 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 > > + * inside queue. If there is no space, it blocks until space becomes > available. > > + * It also schedules transfer thread to transmit the newly added > message. > > + */ > > +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. > > > + 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. > > > + 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. > > > > +/** > > + * isa_ioctl() - To handle different ioctl commands supported by > driver. > > + * @inode: structure is used by the kernel internally to > represent files > > + * @filp: file descriptor pointer > > + * @cmd: ioctl command > > + * @arg: input param > > This appears incomplete - it's just debug statements Yes, will remove this. > > > > +} > > +/** > > + * 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. > > + 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. > > + > > + 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"); > > + break; > > + case AUDIO_LOOPBACK_MESSAGING: > > + kfree(isadev->addr); > > + dev_info(shrm->dev, "Close AUDIO_LOOPBACK_MESSAGING > Device\n"); > > + 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- > >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! > > > + 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. > > > + 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 > > > + } > > + isadev = &isa_context->isadev[idx]; > > + if (filp != NULL) > > + filp->private_data = isadev; > > How can filp be NULL ? :-) just a error condition check > > > + > > + 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"); > > + 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"); > > + 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! > > > +const struct file_operations isa_fops = { > > static > > > > + 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. > > > + > > + 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. > > > > diff --git a/drivers/modem_shm/u8500_shm/shrm_fifo.c > b/drivers/modem_shm/u8500_shm/shrm_fifo.c > > 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 > > + * Kumar Sanghvi for ST-Ericsson > > + * > > + * License terms: GNU General Public License (GPL) version 2 > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#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! > > +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. > > + 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. > > > > + 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 > > > +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 -------------- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/