Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT)

[Paul Bolle <pebolle@tiscali.nl>]

Some remarks (that might not touch the subjects you want to get feedback
on for an RFC).

On Fri, 2015-06-12 at 23:04 +0300, Pantelis Antoniou wrote:
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
 
> +config DEV_OVERLAYMGR
> +	tristate "Device overlay manager"
> +	depends on OF
> +	select OF_OVERLAY
> +	default n

Why bother with "default n"? 

> +	help
> +	  Say Y here to include support for the automagical dev
> +	  overlay manager.

> --- /dev/null
> +++ b/drivers/misc/devovmgr.c

> +#include <linux/ctype.h>
> +#include <linux/cpu.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/spinlock.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/proc_fs.h>
> +#include <linux/configfs.h>
> +#include <linux/types.h>
> +#include <linux/stat.h>
> +#include <linux/limits.h>
> +#include <linux/file.h>
> +#include <linux/vmalloc.h>
> +#include <linux/firmware.h>
> +#include <linux/pci.h>
> +#include <linux/usb.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/workqueue.h>
> +#include <linux/firmware.h>

You're including <linux/firmware.h> twice.

> +/* copy of drivers/pci/pci.h */

Because?

> +static inline const struct pci_device_id *
> +pci_match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
> +{
> +	if ((id->vendor == PCI_ANY_ID || id->vendor == dev->vendor) &&
> +	    (id->device == PCI_ANY_ID || id->device == dev->device) &&
> +	    (id->subvendor == PCI_ANY_ID ||
> +		id->subvendor == dev->subsystem_vendor) &&
> +	    (id->subdevice == PCI_ANY_ID ||
> +		id->subdevice == dev->subsystem_device) &&
> +	    !((id->class ^ dev->class) & id->class_mask))
> +		return id;
> +	return NULL;
> +}

> +#if IS_ENABLED(CONFIG_USB)
> +/* in drivers/usb/core/driver.c */
> +extern int usb_match_device(struct usb_device *dev,
> +		const struct usb_device_id *id);

And that's an internal function of the usb core, isn't it?

> +static int __init dovmgr_init(void)
> +{
> +	int ret;
> +
> +	config_group_init(&dovmgr_subsys.su_group);
> +
> +#if IS_ENABLED(CONFIG_PCI)
> +	ret = dovmgr_pci_init();
> +	if (ret != 0)
> +		goto err_no_pci_init;
> +#endif
> +#if IS_ENABLED(CONFIG_USB)
> +	ret = dovmgr_usb_init();
> +	if (ret != 0)
> +		goto err_no_usb_init;
> +#endif
> +
> +	ret = configfs_register_subsystem(&dovmgr_subsys);
> +	if (ret != 0) {
> +		pr_err("%s: failed to register subsys\n", __func__);
> +		goto err_no_configfs;
> +	}
> +	pr_info("%s: OK\n", __func__);
> +	return 0;
> +
> +err_no_configfs:
> +#if IS_ENABLED(CONFIG_USB)
> +	dovmgr_usb_cleanup();
> +err_no_usb_init:
> +#endif
> +#if IS_ENABLED(CONFIG_PCI)
> +	dovmgr_pci_cleanup();
> +err_no_pci_init:
> +#endif
> +	return ret;
> +}
> +late_initcall(dovmgr_init);

Lot's of "#if IS_ENABLED(CONFIG_USB)" and "#if IS_ENABLED(CONFIG_PCI)"
in the code. The above function is a rather ugly example.

Is there a point to all this if neither PCI nor USB is enabled?

USB can be 'm'. Does this build and work in that case?

There's no MODULE_LICENSE() macro. If this is a module then loading it
will taint the kernel.

There's also no function that is, well, called by module_exit() to allow
(easy) unloading (and do the needed cleaning up on unload). Did you
intend DEV_OVERLAYMGR to be bool instead?

Thanks,


Paul Bolle