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

[Pantelis Antoniou <pantelis.antoniou@konsulko.com>]

Hi Paul,

> On Jun 15, 2015, at 17:43 , Paul Bolle <pebolle@tiscali.nl> wrote:
> 
> 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.
> 

Right,

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

Cause it’s not exported out of PCI core.

>> +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?
> 

Yes.

>> +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?
> 

Note this is an RFC after all, it’s not intended for inclusion as is. I just
present the technique and how it works. Yes, all those defines are probably
not good, but I’d like some remarks on the implementation first.


> Thanks,
> 
> 
> Paul Bolle
> 

Regards

— Pantelis