linux-kernel - Re: [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property

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, 1 May 2019 06:00:28 +0000
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Christophe Leroy <christophe.leroy@....fr>,
        Qiang Zhao <qiang.zhao@....com>, Li Yang <leoyang.li@....com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Scott Wood <oss@...error.net>,
        Rasmus Villemoes <Rasmus.Villemoes@...vas.se>,
        Rob Herring <robh+dt@...nel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 4/5] soc/fsl/qe: qe.c: support fsl,qe-snums property

On 30/04/2019 19.19, Christophe Leroy wrote:
> 
> 
> Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit :
>> The current code assumes that the set of snum _values_ to populate the
>> snums[] array with is a function of the _number_ of snums
>> alone. However, reading table 4-30, and its footnotes, of the QUICC
>> Engine Block Reference Manual shows that that is a bit too naive.
>>
>> As an alternative, this introduces a new binding fsl,qe-snums, which
>> automatically encodes both the number of snums and the actual values to
>> use. Conveniently, of_property_read_variable_u8_array does exactly
>> what we need.
>>
>> For example, for the MPC8309, one would specify the property as
>>
>>                 fsl,qe-snums = /bits/ 8 <
>>                         0x88 0x89 0x98 0x99 0xa8 0xa9 0xb8 0xb9
>>                         0xc8 0xc9 0xd8 0xd9 0xe8 0xe9>;
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
>> ---
>>   .../devicetree/bindings/soc/fsl/cpm_qe/qe.txt      |  8 +++++++-
>>   drivers/soc/fsl/qe/qe.c                            | 14 +++++++++++++-
>>   2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> index d7afaff5faff..05f5f485562a 100644
>> --- a/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> +++ b/Documentation/devicetree/bindings/soc/fsl/cpm_qe/qe.txt
>> @@ -18,7 +18,8 @@ Required properties:
>>   - reg : offset and length of the device registers.
>>   - bus-frequency : the clock frequency for QUICC Engine.
>>   - fsl,qe-num-riscs: define how many RISC engines the QE has.
>> -- fsl,qe-num-snums: define how many serial number(SNUM) the QE can
>> use for the
>> +- fsl,qe-snums: This property has to be specified as '/bits/ 8' value,
>> +  defining the array of serial number (SNUM) values for the virtual
>>     threads.
>>     Optional properties:
>> @@ -34,6 +35,11 @@ Recommended properties
>>   - brg-frequency : the internal clock source frequency for baud-rate
>>     generators in Hz.
>>   +Deprecated properties
>> +- fsl,qe-num-snums: define how many serial number(SNUM) the QE can use
>> +  for the threads. Use fsl,qe-snums instead to not only specify the
>> +  number of snums, but also their values.
>> +
>>   Example:
>>        qe@...00000 {
>>       #address-cells = <1>;
>> diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
>> index aff9d1373529..af3c2b2b268f 100644
>> --- a/drivers/soc/fsl/qe/qe.c
>> +++ b/drivers/soc/fsl/qe/qe.c
>> @@ -283,7 +283,6 @@ EXPORT_SYMBOL(qe_clock_source);
>>    */
>>   static void qe_snums_init(void)
>>   {
>> -    int i;
> 
> Why do you move this one ?

To keep the declarations of the auto variables together. When reading
the code and needing to know the type of i, it's much harder to find its
declaration if one has to skip back over the two tables, and it's
unnatural to have it separate from the others.

>>       static const u8 snum_init_76[] = {
>>           0x04, 0x05, 0x0C, 0x0D, 0x14, 0x15, 0x1C, 0x1D,
>>           0x24, 0x25, 0x2C, 0x2D, 0x34, 0x35, 0x88, 0x89,
>> @@ -304,9 +303,22 @@ static void qe_snums_init(void)
>>           0x28, 0x29, 0x38, 0x39, 0x48, 0x49, 0x58, 0x59,
>>           0x68, 0x69, 0x78, 0x79, 0x80, 0x81,
>>       };
>> +    struct device_node *qe;
>>       const u8 *snum_init;
>> +    int i;
>>         bitmap_zero(snum_state, QE_NUM_OF_SNUM);
>> +    qe = qe_get_device_node();
>> +    if (qe) {
>> +        i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
>> +                               snums, 1, QE_NUM_OF_SNUM);
>> +        of_node_put(qe);
>> +        if (i > 0) {
>> +            qe_num_of_snum = i;
>> +            return;
> 
> In that case you skip the rest of the init ? Can you explain ?

If of_property_read_variable_u8_array is succesful, it has already
stored the values into the snums array, so there's no copying left to
do, and the return value is the length of the array (which we save for
later in qe_num_of_snum). So there's really nothing more to do.

This was what I tried to hint at with "Conveniently,
of_property_read_variable_u8_array does exactly
what we need.", but I can see that that might need elaborating a little.

Rasmus