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]

Date:   Mon, 14 Sep 2020 10:38:25 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
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

Hi Pierre,

On 11-09-20, 09:50, Pierre-Louis Bossart wrote:
> > > > > > > > > + *	25		0 (Reserved)
> > > > > > > > > + *	24:22		Function Number [2:0]
> > > > > > > > > + *	21		Entity[6]
> > > > > > > > > + *	20:19		Control Selector[5:4]
> > > > > > > > > + *	18		0 (Reserved)
> > > > > > > > > + *	17:15		Control Number[5:3]
> > > > > > > > > + *	14		Next
> > > > > > > > > + *	13		MBQ
> > > > > > > > > + *	12:7		Entity[5:0]
> > > > > > > > > + *	6:3		Control Selector[3:0]
> > > > > > > > > + *	2:0		Control Number[2:0]
> 
> [...]
> 
> > > > > 
> > > > > #define SDCA_CONTROL_DEST_MASK1 GENMASK(20, 19)
> > > > > #define SDCA_CONTROL_ORIG_MASK1 GENMASK(5, 4)
> > > > > #define SDCA_CONTROL_DEST_MASK2 GENMASK(6, 3)
> > > > > #define SDCA_CONTROL_ORIG_MASK2 GENMASK(3, 0)
> > 
> > I think I missed ORIG and DEST stuff, what does this mean here?
> 
> If you missed this, it means my explanations are not good enough and I need
> to make it clearer in the commit log/documentation. Point taken, I'll
> improve this for the next version.
> 
> > Relooking at the bit definition, for example 'Control Number' is defined
> > in both 17:15 as well as 2:0, why is that. Is it split?
> > 
> > How does one program a control number into this?
> 
> A Control Number is represented on 6 bits.
> 
> See the documentation above.
> 
> 	17:15		Control Selector[5:3]
> 	2:0		Control Selector[2:0]
> 
> The 3 MSBs for into bits 17:15 of the address, and the 3 LSBs into bits 2:0
> of the address. The second part is simpler for Control Number but for
> entities and control selectors the LSB positions don't match.
> 
> Yes it's convoluted but it was well-intended: in most cases, there is a
> limited number of entities, control selectors, channel numbers, and putting
> the LSBs together in the 16-LSB of the address helps avoid reprogramming
> paging registers: all the addresses for a given function typically map into
> the same page.
> 
> That said, I am not sure the optimization is that great in the end, because
> we end-up having to play with bits for each address. Fewer changes of the
> paging registers but tons of operations in the core.
> 
> I wasn't around when this mapping was defined, and it is what is is now.
> There's hardware built based on this formula so we have to make it work.
> 
> Does this clarify the usage?

Thanks, that is very helpful. I have overlooked this bit.

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.

-- 
~Vinod