Re: [PATCHv2 07/10] xshm: XSHM Configuration data handling

[Arnd Bergmann <arnd@arndb.de>]

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