linux-kernel - Re: [RFC PATCH 6/9] memory: apple: Add apple-mcc driver to manage MCC perf in Apple SoCs

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: <f81467d4-74b2-176d-06bf-f04e073efce4@canonical.com>
Date:   Thu, 14 Oct 2021 09:36:25 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Hector Martin <marcan@...can.st>,
        linux-arm-kernel@...ts.infradead.org
Cc:     Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Sven Peter <sven@...npeter.dev>, Marc Zyngier <maz@...nel.org>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Viresh Kumar <vireshk@...nel.org>, Nishanth Menon <nm@...com>,
        Catalin Marinas <catalin.marinas@....com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 6/9] memory: apple: Add apple-mcc driver to manage MCC
 perf in Apple SoCs

On 14/10/2021 08:59, Hector Martin wrote:
> On 12/10/2021 18.19, Krzysztof Kozlowski wrote:
>>> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
>>> +/*
>>> + * Apple SoC MCC memory controller performance control driver
>>> + *
>>> + * Copyright The Asahi Linux Contributors
>>
>> Copyright date?
> 
> We've gone over this one a few times already; most copyright dates 
> quickly become outdated and meaningless :)
> 
> See: 
> https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/
> 
>>> +static int apple_mcc_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *node = dev->of_node;
>>
>> By convention mostly we call the variable "np".
> 
> Ack, I'll change it for v2.
> 
>>> +	mcc->reg_base = devm_platform_ioremap_resource(pdev, 0);
>>> +	if (IS_ERR(mcc->reg_base))
>>> +		return PTR_ERR(mcc->reg_base);
>>> +
>>> +	if (of_property_read_u32(node, "apple,num-channels", &mcc->num_channels)) {
>>
>> Don't you have a limit of supported channels? It cannot be any uint32...
> 
> Today, it's max 8. But if come Monday we find out Apple's new chips have 
> 16 channels and otherwise the same register layout, I'd much rather not 
> have to change the driver...

OK, however if the driver ever receives different DT with a different
value, it will accept it unconditionally and go via address space. I am
just saying that being conservative on received values is safer, but I
am fine with skipping this problem. At the end we trust DT that it will
always match the kernel, don't we? Oh wait, someone can use DT from
other kernel in this one...

> 
>>> +		dev_err(dev, "missing apple,num-channels property\n");
>>
>> Use almost everywhere dev_err_probe - less code and you get error msg
>> printed.
> 
> Heh, I didn't know about that one. Thanks!
> 
>>> +
>>> +	dev_info(dev, "Apple MCC performance driver initialized\n");
>>
>> Please skip it, or at least make it a dev_dbg, you don't print any
>> valuable information here.
> 
> Ack, I'll remove this.
> 
>>> +static struct platform_driver apple_mcc_driver = {
>>> +	.probe = apple_mcc_probe,
>>> +	.driver = {
>>> +		.name = "apple-mcc",
>>> +		.of_match_table = apple_mcc_of_match,
>>> +	},
>>> +};
>>
>> module_platform_driver() goes here.
> 
> Ack, will fix for v2.
> 
>>
>>> +
>>> +MODULE_AUTHOR("Hector Martin <marcan@...can.st>");
>>> +MODULE_DESCRIPTION("MCC memory controller performance tuning driver for Apple SoCs");
>>> +MODULE_LICENSE("GPL v2");
>>
>> I think this will be "Dual MIT/GPL", based on your SPDX.
> 
> Ah, I didn't realize that was a valid option for MODULE_LICENSE. I guess 
> anything containing "GPL" works with EXPORT_SYMBOL_GPL?

I don't think exporting symbols is related to how you license your code.


Best regards,
Krzysztof