linux-kernel - Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD

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]

Date:   Wed, 31 May 2017 09:28:31 +0100
From:   Phil Elwell <phil@...pberrypi.org>
To:     Stefan Wahren <stefan.wahren@...e.com>, linux-clk@...r.kernel.org,
        Eric Anholt <eric@...olt.net>, linux-kernel@...r.kernel.org,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        linux-rpi-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] clk: bcm2835: Limit PCM clock to OSC and PLLD_PER

Hi Stefan,

On 30/05/2017 19:41, Stefan Wahren wrote:
> Hi Phil,
> 
>> Phil Elwell <phil@...pberrypi.org> hat am 30. Mai 2017 um 18:28 geschrieben:
>>
>>
>> Restrict clock sources for the PCM peripheral to the oscillator and
>> PLLD_PER because other source may have varying rates or be switched off.
> 
>> Prevent other sources from being selected by replacing their names in
>> the list of potential parents with dummy entries (entry index is
>> significant).
> 
> i like to have this as a comment above the definition of bcm2835_pcm_per_parents.

Sure - good idea.

>>
>> Signed-off-by: Phil Elwell <phil@...pberrypi.org>
>> ---
>>  drivers/clk/bcm/clk-bcm2835.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
>> index 0258538..facc346 100644
>> --- a/drivers/clk/bcm/clk-bcm2835.c
>> +++ b/drivers/clk/bcm/clk-bcm2835.c
>> @@ -1511,6 +1511,16 @@ struct bcm2835_clk_desc {
>>  	"pllh_aux",
>>  };
>>
>> +static const char *const bcm2835_pcm_per_parents[] = {
> 
> As mentioned above, there should be a comment like all the others.

Yes, will do.

>> +	"-",
>> +	"xosc",
>> +	"-",
>> +	"-",
>> +	"-",
>> +	"-",
>> +	"plld_per",
>> +};
> 
> Is there a dummy entry for "pllh_aux" missing?

Yes and no - adding it will cause an extra iteration around the loop, but 
perhaps it's less confusing. I'll add one.

>> +
>>  #define REGISTER_PER_CLK(...)	REGISTER_CLK(				\
>>  	.num_mux_parents = ARRAY_SIZE(bcm2835_clock_per_parents),	\
>>  	.parents = bcm2835_clock_per_parents,				\
>> @@ -2000,6 +2010,7 @@ struct bcm2835_clk_desc {
>>  		.int_bits = 12,
>>  		.frac_bits = 12,
>>  		.is_mash_clock = true,
>> +		.parents = bcm2835_pcm_per_parents,
> 
> This looks a little bit hacky to me. Not sure, but can we do something like this?
> 
> #define REGISTER_PCM_CLK(...)	REGISTER_CLK(				\
> 	.num_mux_parents = ARRAY_SIZE(bcm2835_pcm_per_parents),	\
> 	.parents = bcm2835_pcm_per_parents,				\
> 	__VA_ARGS__)

Of course - no problem.

Thanks for the feedback - it will be incorporated into V2.

Phil