lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAE_wzQ_Rq5u83UXOYRLShxuCzcgxA0BSPFZT4RmvDj8nfTSGcg@mail.gmail.com>
Date:	Fri, 6 Mar 2015 09:35:51 -0800
From:	Dmitry Torokhov <dtor@...gle.com>
To:	Ray Jui <rjui@...adcom.com>
Cc:	Paul Bolle <pebolle@...cali.nl>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Arnd Bergmann <arnd@...db.de>,
	Hauke Mehrtens <hauke@...ke-m.de>,
	Florian Fainelli <f.fainelli@...il.com>,
	Anatol Pomazau <anatol@...gle.com>,
	Scott Branden <sbranden@...adcom.com>,
	linux-pci@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-arm-kernel@...ts.infradead.org,
	bcm-kernel-feedback-list@...adcom.com, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/3] pci: iproc: Add Broadcom iProc PCIe support

On Fri, Mar 6, 2015 at 9:26 AM, Ray Jui <rjui@...adcom.com> wrote:
> Hi Paul,
>
> On 3/6/2015 3:00 AM, Paul Bolle wrote:
>> On Thu, 2015-03-05 at 17:01 -0800, Ray Jui wrote:
>>> --- a/drivers/pci/host/Kconfig
>>> +++ b/drivers/pci/host/Kconfig
>>> @@ -106,4 +106,21 @@ config PCI_VERSATILE
>>>      bool "ARM Versatile PB PCI controller"
>>>      depends on ARCH_VERSATILE
>>>
>>> +config PCIE_IPROC
>>> +    bool "Broadcom iProc PCIe controller"
>>
>> bool symbol.
>>
>>> +    help
>>> +      This enables the iProc PCIe core controller support for Broadcom's
>>> +      iProc family of SoCs. An appropriate bus interface driver also needs
>>> +      to be enabled
>>> +
>>> +config PCIE_IPROC_PLTFM
>>> +    bool "Broadcom iProc PCIe platform bus driver"
>>
>> Another bool symbol.
>>
>>> +    depends on ARCH_BCM_IPROC || COMPILE_TEST
>>> +    depends on OF
>>> +    select PCIE_IPROC
>>> +    default ARCH_BCM_IPROC
>>> +    help
>>> +      Say Y here if you want to use the Broadcom iProc PCIe controller
>>> +      through the generic platform bus interface
>>> +
>>>  endmenu
>>
>>> --- a/drivers/pci/host/Makefile
>>> +++ b/drivers/pci/host/Makefile
>>
>>> +obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>>> +obj-$(CONFIG_PCIE_IPROC_PLTFM) += pcie-iproc-pltfm.o
>>
>> Both objectfiles will never be part of a module.
>>
>>> --- /dev/null
>>> +++ b/drivers/pci/host/pcie-iproc-pltfm.c
>>
>>> +#include <linux/module.h>
>>
>> Is this needed?
>>
>>> +MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
>>
>> This macro will be preprocessed away, won't it?
>>
>>> +static struct platform_driver iproc_pcie_pltfm_driver = {
>>> +    .driver = {
>>> +            .name = "iproc-pcie",
>>> +            .of_match_table = of_match_ptr(iproc_pcie_of_match_table),
>>> +            .suppress_bind_attrs = true,
>>> +    },
>>> +    .probe = iproc_pcie_pltfm_probe,
>>> +};
>>> +module_platform_driver(iproc_pcie_pltfm_driver);
>>
>> Perhaps it's clearer to have make this a call to
>> platform_driver_register(), put that in a wrapper function, and invoke
>> that wrapper function through device_initcall() or similar. I haven't
>> actually tested that, so the details could be off.
>>
>>> +MODULE_AUTHOR("Ray Jui <rjui@...adcom.com>");
>>> +MODULE_DESCRIPTION("Broadcom iPROC PCIe platform driver");
>>> +MODULE_LICENSE("GPL v2");
>>
>> And these three macros will, effectively, be preprocessed away.
>>
>>> --- /dev/null
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>
>>> +#include <linux/module.h>
>>
>> See above.
>>
>>> +MODULE_AUTHOR("Ray Jui <rjui@...adcom.com>");
>>> +MODULE_DESCRIPTION("Broadcom iPROC PCIe common driver");
>>> +MODULE_LICENSE("GPL v2");
>>
>> Ditto.
>>
>>
>> Paul Bolle
>>
>
> So every single PCIe host driver under drivers/pci/host/* has their
> config flag of "bool" type, and all of them except pci-keystone-dw.c
> have these MODULE based macros in their driver. While I agree with you
> that these macros will preprocessed away while compiled as statically
> linked in, I thought it's a convention to have these macros in the
> driver. At least it provides information on the author, driver
> description, and license (although one can also argue you can find all
> of those info from the maintainer list, Kconfig, and license header).
>
> Would you be able to sort this out with a developer or subsystem
> maintainer who is familiar with this matter and let us know what is the
> convention for MODULE macros usage in a driver when its config flag is
> set to "bool" instead of "tristate"?

Is there a technical reason why the driver can't be a module? We can
suppress module unloading if unloading properly is hard (i see you
already suppress unbinding via sysfs), but we should be able to load
it after kernel booted, no?

Thanks,
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ