linux-kernel - Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access controls

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: <11feabb2-dc8b-7acc-6e4d-0903fc435b00@linux.intel.com>
Date:   Mon, 14 Sep 2020 09:44:58 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     alsa-devel@...a-project.org, tiwai@...e.de, broonie@...nel.org,
        gregkh@...uxfoundation.org,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>,
        Guennadi Liakhovetski <guennadi.liakhovetski@...ux.intel.com>,
        Kai Vehmanen <kai.vehmanen@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] soundwire: SDCA: add helper macro to access
 controls




> For LSB bits, I dont think this is an issue. I expect it to work, for example:
> #define CONTROL_LSB_MASK  GENMASK(2, 0)
>          foo |= u32_encode_bits(control, CONTROL_LSB_MASK);
> 
> would mask the control value and program that in specific bitfeild.
> 
> But for MSB bits, I am not sure above will work so, you may need to extract
> the bits and then use, for example:
> #define CONTROL_MSB_BITS        GENMASK(5, 3)
> #define CONTROL_MSB_MASK        GENMASK(17, 15)
> 
>          control = FIELD_GET(CONTROL_MSB_BITS, control);
>          foo |= u32_encode_bits(control, CONTROL_MSB_MASK);
> 
>> If you have a better suggestion that the FIELD_PREP/FIELD_GET use, I am all
>> ears. At the end of the day, the mapping is pre-defined and we don't have
>> any degree of freedom. What I do want is that this macro/inline function is
>> shared by all codec drivers so that we don't have different interpretations
>> of how the address is constructed.
> 
> Absolutely, this need to be defined here and used by everyone else.

Compare:

#define SDCA_CONTROL_MSB_BITS        GENMASK(5, 3)
#define SDCA_CONTROL_MSB_MASK        GENMASK(17, 15)
#define SDCA_CONTROL_LSB_MASK        GENMASK(2, 0)

foo |= u32_encode_bits(control, SDCA_CONTROL_LSB_MASK);
control = FIELD_GET(SDCA_CONTROL_MSB_BITS, control);
foo |= u32_encode_bits(control, SDCA_CONTROL_MSB_MASK);

with the original proposal:

foo |= FIELD_GET(GENMASK(2, 0), control))	
foo |= FIELD_PREP(GENMASK(17, 15), FIELD_GET(GENMASK(5, 3), control))	

it gets worse when the LSB positions don't match, you need another 
variable and an additional mask.

I don't see how this improves readability? I get that hard-coding magic 
numbers is a bad thing in general, but in this case there are limited 
benefits to the use of additional defines.