From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755902AbbFOOnX (ORCPT ); Mon, 15 Jun 2015 10:43:23 -0400 Received: from lb3-smtp-cloud6.xs4all.net ([194.109.24.31]:56647 "EHLO lb3-smtp-cloud6.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755745AbbFOOnK (ORCPT ); Mon, 15 Jun 2015 10:43:10 -0400 Message-ID: <1434379385.2069.33.camel@x220> Subject: Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT) From: Paul Bolle To: Pantelis Antoniou Cc: Rob Herring , Grant Likely , Matt Porter , Koen Kooi , Guenter Roeck , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Pantelis Antoniou Date: Mon, 15 Jun 2015 16:43:05 +0200 In-Reply-To: <1434139460-16194-1-git-send-email-pantelis.antoniou@konsulko.com> References: <1434139460-16194-1-git-send-email-pantelis.antoniou@konsulko.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4 (3.10.4-4.fc20) Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include You're including 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Bolle Subject: Re: [PATCH] RFC: Device overlay manager (PCI/USB + DT) Date: Mon, 15 Jun 2015 16:43:05 +0200 Message-ID: <1434379385.2069.33.camel@x220> References: <1434139460-16194-1-git-send-email-pantelis.antoniou@konsulko.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1434139460-16194-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Pantelis Antoniou Cc: Rob Herring , Grant Likely , Matt Porter , Koen Kooi , Guenter Roeck , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Pantelis Antoniou List-Id: devicetree@vger.kernel.org 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include You're including 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html