Messages in this thread | | | From | Arnd Bergmann <> | Subject | Re: [PATCHv2 07/10] xshm: XSHM Configuration data handling | Date | Fri, 9 Dec 2011 15:09:40 +0000 |
| |
On Friday 09 December 2011, Sjur Brændeland wrote: > This patch implements the configuration handling: > - reception of gen-netlink requests from user-space. > - calculating address of where to put information in shared memory, i.e > the offset of: IPC-TOC, channel-descriptors, read/write indexes for each > directional channel, and data area of each channel. > - formatting of the shared memory area, so modem can read out configuration > data. > - Creation of configuration data given to the XSHM drivers: xshm_chr and > caif_xshm.
Hmm, from what I read here, you rely on the user to create the channels that correspond to the ones that the modem side has configured.
Why is that? Can't you query from the modem what channels it needs and then automatically create those?
> +static bool config_error; > +static bool commited; > +static bool registered; > +static bool addr_set; > +static u32 modem_bootimg_size; > +static void *shm_start; > +static u32 xshm_channels; > +static struct xshm_dev *xshmdevs[XSHM_MAX_CHANNELS + 1]; > +static struct xshm_ipctoc *ipctoc;
Global variables like this are a bit problematic. What would happen if you ever get a system that has access to two devices? I think you can remove most of these easily. Anything that is specific to the xshm modem should just go into the device private data of the modem.
Do not hold your own list of devices, that's what the generic device handling is for. You can e.g. use device_for_each_child on the modem device to iterate the channels.
> +static unsigned long xshm_start; > +module_param(xshm_start, ulong, S_IRUGO | S_IWUSR); > +MODULE_PARM_DESC(xshm_start, "Address for memory shared by host/modem."); > + > +static unsigned long xshm_c2c_bootaddr; > +module_param(xshm_c2c_bootaddr, ulong, S_IRUGO | S_IWUSR); > +MODULE_PARM_DESC(xshm_c2c_bootaddr, "Address given to modem (through GENI register)"); > + > +static long xshm_size; > +module_param(xshm_size, long, S_IRUGO | S_IWUSR); > +MODULE_PARM_DESC(xshm_size, "Size of SHM area");
That looks fragile, you should never have addresses as module arguments. Either the kernel should be free to pick a reasonable address and tell it to the modem, or the modem should pick an address and then the kernel needs a reliable way to find out what it is.
> +/* sysfs: Write the modem firmware including TOC */ > +static ssize_t modemfw_write(struct file *f, struct kobject *kobj, > + struct bin_attribute *attr, > + char *buf, loff_t off, size_t count) > +{ > + if (commited) > + return -EBUSY; > + > + if (off + count > xshm_size) > + return -ENOSPC; > + memcpy(shm_start + off, buf, count); > + modem_bootimg_size = off + count; > + return count; > +} > + > +/* sysfs: Modem firmware attribute */ > +static struct bin_attribute modemfw_attr = { > + .attr = { > + .name = "bootimg", > + .mode = S_IRUGO | S_IWUSR, > + }, > + .size = IMG_MAX_SZ, > + .read = modemfw_read, > + .write = modemfw_write > +};
Why don't you use the regular request_firmware() function here?
Arnd -- 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/
| |