On Mon, 2015-07-13 at 16:09 -0400, Gabriel L. Somlo wrote:
>
> 3. I'm currently only handling x86 and I/O ports. I could drop the
> fw_cfg_dmi_whitelist and just check the signature, using mmio where
> appropriate, but I don't have a handy-dandy set of VMs for those
> architectures on which I could test. Wondering if that's something
> we should have before I officially try to submit this to the kernel,
> or whether it could wait for a second iteration.
>
> Speaking of the kernel: My default plan is to subscribe to
> kernelnewb...@kernelnewbies.org and submit it there (this is after
> all baby's first kernel module :) but if there's a better route for
> pushing it upstream, please feel free to suggest it to me.
Congrats on your first kernel patch! Cc'ing kernelnewbies is OK but this
patches warrants a wider audience. Cc Greg Kroah-Hartman
(gre...@linuxfoundation.org) since he polices things related to sysfs,
the x86 maintainers (x...@kernel.org), and linux-ker...@vger.kernel.org.
The closest thing we have to a Linux system firmware list is probably
linux-...@vger.kernel.org.
By all means, mention that it's your first kernel patch when submitting
this.
Oh, you're also going to want to add documentation under
Documentation/ABI/testing/sysfs-firmware-fw_cfg as part of your patch
series.
> /* provide atomic read access to hardware fw_cfg device
> * (critical section involves potentially lengthy i/o, using mutex) */
> static DEFINE_MUTEX(fw_cfg_dev_lock);
Just a very small nitpick, but,
/*
* Multi-line comments should look like this
* in the kernel.
*/
> static int __init fw_cfg_sysfs_init(void)
> {
> int ret = 0;
> uint32_t count, i;
> char sig[FW_CFG_SIG_SIZE];
> struct fw_cfg_sysfs_entry *entry;
>
> /* only install on matching "hardware" */
> if (!dmi_check_system(fw_cfg_whitelist)) {
> pr_warn("Supported QEMU Standard PC hardware not found!\n");
> return -ENODEV;
> }
I would suggest deleting this warning because it's just noise if I build
this code into my kernel and boot it on non-qemu hardware.
> /* verify fw_cfg device signature */
> fw_cfg_read_blob(FW_CFG_SIGNATURE, sig, 0, FW_CFG_SIG_SIZE);
> if (memcmp(sig, "QEMU", FW_CFG_SIG_SIZE) != 0) {
> pr_warn("QEMU fw_cfg signature not found!\n");
> return -ENODEV;
> }
>
> /* create fw_cfg folder in sysfs */
> fw_cfg_kset = kset_create_and_add("fw_cfg", NULL, firmware_kobj);
> if (!fw_cfg_kset)
> return -ENOMEM;
>
> /* process fw_cfg file directory entry */
> mutex_lock(&fw_cfg_dev_lock);
> outw(FW_CFG_FILE_DIR, FW_CFG_PORT_CTL); /* select fw_cfg directory */
> insb(FW_CFG_PORT_DATA, &count, sizeof(count)); /* get file count */
> count = be32_to_cpu(count);
> /* create & register a sysfs node for each file in the directory */
> for (i = 0; i < count; i++) {
> /* allocate new entry */
> entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> if (!entry) {
> ret = -ENOMEM;
> break;
> }
>
> /* acquire file information from directory */
> insb(FW_CFG_PORT_DATA, &entry->f, sizeof(struct fw_cfg_file));
> entry->f.size = be32_to_cpu(entry->f.size);
> entry->f.select = be16_to_cpu(entry->f.select);
>
> /* register sysfs entry for this file */
> entry->kobj.kset = fw_cfg_kset;
> ret = kobject_init_and_add(&entry->kobj,
> &fw_cfg_sysfs_entry_ktype, NULL,
> /* NOTE: '/' represented as '!' */
> "%s", entry->f.name);
> if (ret)
> break;
If you take this error path, aren't you leaking 'entry'? It isn't yet on
the 'fw_cfg_entry_cache' list and so won't be freed in
fw_cfg_sysfs_cache_cleanup().
