From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755549AbbFOO4r (ORCPT <rfc822;w@1wt.eu>);
	Mon, 15 Jun 2015 10:56:47 -0400
Received: from mail-wi0-f174.google.com ([209.85.212.174]:35258 "EHLO
	mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754918AbbFOO4l convert rfc822-to-8bit (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 15 Jun 2015 10:56:41 -0400
Subject: Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT)
Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\))
Content-Type: text/plain; charset=utf-8
From: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
In-Reply-To: <1434379385.2069.33.camel@x220>
Date: Mon, 15 Jun 2015 17:56:35 +0300
Cc: Rob Herring <robherring2@gmail.com>,
        Grant Likely <grant.likely@secretlab.ca>,
        Matt Porter <mporter@konsulko.com>,
        Koen Kooi <koen@dominion.thruhere.net>,
        Guenter Roeck <linux@roeck-us.net>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <7B4810B3-61BE-4510-A884-3BC02D786A41@konsulko.com>
References: <1434139460-16194-1-git-send-email-pantelis.antoniou@konsulko.com> <1434379385.2069.33.camel@x220>
To: Paul Bolle <pebolle@tiscali.nl>
X-Mailer: Apple Mail (2.2098)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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