On Thu, Nov 22 2012, Andrzej Pietrasiewicz wrote:
> +#define _FSG_MODULE_PARAM_ARRAY(prefix, params, name, type, desc) \
> + module_param_array_named(prefix ## name, params.name, type, \
> + &prefix ## params.name ## _count, \
> + S_IRUGO); \
> + MODULE_PARM_DESC(prefix ## name, desc)
> +
> +#define _FSG_MODULE_PARAM(prefix, params, name, type, desc) \
> + module_param_named(prefix ## name, params.name, type, \
> + S_IRUGO); \
> + MODULE_PARM_DESC(prefix ## name, desc)
> +
> +#define FSG_MODULE_PARAMETERS(prefix, params) \
> + _FSG_MODULE_PARAM_ARRAY(prefix, params, file, charp, \
> + "names of backing files or devices"); \
> + _FSG_MODULE_PARAM_ARRAY(prefix, params, ro, bool, \
> + "true to force read-only"); \
> + _FSG_MODULE_PARAM_ARRAY(prefix, params, removable, bool, \
> + "true to simulate removable media"); \
> + _FSG_MODULE_PARAM_ARRAY(prefix, params, cdrom, bool, \
> + "true to simulate CD-ROM instead of disk"); \
> + _FSG_MODULE_PARAM_ARRAY(prefix, params, nofua, bool, \
> + "true to ignore SCSI WRITE(10,12) FUA bit"); \
> + _FSG_MODULE_PARAM(prefix, params, luns, uint, \
> + "number of LUNs"); \
> + _FSG_MODULE_PARAM(prefix, params, stall, bool, \
> + "false to prevent bulk stalls")
Since those will be used only in this wrapper, you can just drop the
macro and use the _FSG_MODULE_PARAM* macros directly. Furthermore, you
can drop the “prefix” argument.
> +
> +#define UFG_MODULE (UFG_SUBSYSTEM->subsys.su_group.cg_item.ci_type->ct_owner)
I cannot seem to find UFG_SUBSYSTEM anywhere.
> +#define LUN(i) config->luns[i]
This looks like just obfuscating things. At least, making it return
a pointer would be less weird IMO:
#define LUN(i) (config->luns + (i))
This is because since kernel is written in C, there is no concept of
a reference, and therefore “foo().bar” is not common.
> +
> +struct fsg_module_parameters {
> + char *file[FSG_MAX_LUNS];
> + bool ro[FSG_MAX_LUNS];
> + bool removable[FSG_MAX_LUNS];
> + bool cdrom[FSG_MAX_LUNS];
> + bool nofua[FSG_MAX_LUNS];
> +
> + unsigned int file_count, ro_count, removable_count, cdrom_count;
> + unsigned int nofua_count;
> + unsigned int luns; /* nluns */
> + bool stall; /* can_stall */
> +};
> +
> +struct fsg_config {
> + unsigned nluns;
> + unsigned configfs_nluns;
> + struct fsg_lun_config {
> + const char *filename;
> + char ro;
> + char removable;
> + char cdrom;
> + char nofua;
> + struct device dev;
> + struct dentry *dentry;
> + } luns[FSG_MAX_LUNS];
> +
> + char can_stall;
> +
> + struct dentry *root;
> + struct dentry *gadget;
> + struct dentry *conf;
> + struct dentry *func;
> + struct dentry *f;
> +};
Again, since the module is the only user of those, this cane be
*greatly* simplified.
> +
> +/*
> + * ATTENTION:
> + *
> + * struct configfs_dirent is "borrowed" from fs/configfs/configfs_internal.h.
Could we just include it? It sounds like a better idea to me.
> + *
> + * The adapters by default will be phased out, so this _should_ be acceptable.
> + */
> +struct configfs_dirent {
> + atomic_t s_count;
> + int s_dependent_count;
> + struct list_head s_sibling;
> + struct list_head s_children;
> + struct list_head s_links;
> + void * s_element;
> + int s_type;
> + umode_t s_mode;
> + struct dentry * s_dentry;
> + struct iattr * s_iattr;
> +#ifdef CONFIG_LOCKDEP
> + int s_depth;
> +#endif
> +};
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo--Attachment:
pgpvPAQqkSIAs.pgp
Description: PGP signature
