linux-kernel - Re: [RFC PATCH] soc/fsl/qe: support MPC8309

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:   Thu, 28 Feb 2019 08:11:28 +0000
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Qiang Zhao <qiang.zhao@....com>, Leo Li <leoyang.li@....com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Valentin Longchamp <valentin.longchamp@...mile.com>,
        Scott Wood <oss@...error.net>,
        Rasmus Villemoes <Rasmus.Villemoes@...vas.se>
Subject: Re: [RFC PATCH] soc/fsl/qe: support MPC8309

On 28/02/2019 08.14, Qiang Zhao wrote:
> On Tue, Feb 26, 2019 at 2:49 AM Rasmus Villemoes <rasmus.villemoes@...vas.dk> wrote:
>> -----Original Message-----
>> From: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
>> Sent: 2019年2月26日 16:48
>> To: Qiang Zhao <qiang.zhao@....com>; Leo Li <leoyang.li@....com>
>> Cc: linux-kernel@...r.kernel.org; Valentin Longchamp
>> <valentin.longchamp@...mile.com>; Scott Wood <oss@...error.net>;
>> Rasmus Villemoes <Rasmus.Villemoes@...vas.se>
>> Subject: [RFC PATCH] soc/fsl/qe: support MPC8309
>>
>> Currently, when device tree specifies fsl,qe-num-snums = 28 (which a number
>> of in-tree .dts files do, and which is the default when that property is missing),
>> qe_snums_init() ends up using the first 28 elements of the snum_init_46[]
>> array.
>>
>> The situation is quite messy. This patch may break existing setups that for
>> some reason work with specifying fsl,qe-num-snums = 28 and using the
>> existing snum_init_46 array. OTOH, the current code certainly does not work
>> for the MPC8309-based board we're working on, since the first 14 of the
>> elements in snum_init_46 are "Not available on
>> MPC8306/MPC8306S/MPC8309" according to the QUICC Engine Reference
>> Manual (Table 4-30) - and indeed, without this patch (or something to the
>> same effect), we get
> 
> According to the QUICC Engine Reference Manual (Table 4-30), the number of snums used for 
> " MPC8306/MPC8306S/MPC8309" should be 14 instead of 28, so maybe we should assign "fsl,qe-num-snums = 14"
> And define a new snum_init_14 array, meanwhile, modify the minimum value of "num_of_snums" to 14 in function "qe_get_num_of_snums"
> (I mean: " if ((num_of_snums < 14) || (num_of_snums > QE_NUM_OF_SNUM)) {")

Perhaps, but it's completely unclear to me why the code would contain a
comment /* No QE ever has fewer than 28 SNUMs */ when that then appears
to be wrong. It is also far from clear why the snums such as 0x08, 0x09
etc. that appear in table 4-30 without any assigned meaning do appear in
the existing snum_init_46 table - but of course, those elements of that
array don't currently happen to be used when qe_get_num_of_snums==28
(but they do get used for e.g. mpc8569 which specifies fsl,qe-num-snums=46).

Altogether, I think it's a completely wrong approach to pretend that the
_set of valid snums_ is a function of the _number of snums_ alone - the
footnotes to table 4-30 make it abundantly clear that one really cannot
expect such a simple relationship.

So, I suggest deprecating the fsl,qe-num-snums binding, in favor of a
more sensible

fsl,qe-snums = /bits/ 8 <0x88 0x89 0x98 0x99 ...>

which will automatically encode both the number of snums as well as the
concrete values to be used.

Rasmus