linux-kernel - RE: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data" 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]

Message-ID: <DB8PR04MB6795046DC70B8238963C343AE6D49@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Wed, 8 Sep 2021 07:14:25 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh@...nel.org>
CC:     "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        dl-linux-imx <linux-imx@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data" property


Hi Srinivas,

[...]
> I have pushed some nvmem core patches which are just compile tested to
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fsrini%2Fnvmem.git%2Flog%
> 2F%3Fh%3Dtopic%2Fpost-processing&amp;data=04%7C01%7Cqiangqing.zhan
> g%40nxp.com%7Cadfa3ba63c634937876308d971e7e71f%7C686ea1d3bc2b4c6
> fa92cd99c5c301635%7C0%7C0%7C637666063097239185%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJ
> XVCI6Mn0%3D%7C1000&amp;sdata=W9yAnGm9rYzlSZuAAGiN4VHUtKYUTt9S
> oyGQ9QsY7fI%3D&amp;reserved=0
> 
> This should provide the callback hook I was talking about.

Thanks a lot! Yes, this could be more common, vendors can parse their mac
address for different encoding style, also can extend for other cases.

> Can you take a look at them and let me know if it works for you.

There are some small issues need to be update:
1) https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=topic/post-processing&id=624f2cc99b48bbfe05c11e58fb73f84abb1a646e
	of_get_property() can't get the cell value, so I change to of_property_read_s32()
2) https://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git/commit/?h=topic/post-processing&id=a424302c7b15da41e1e8de56b0c78021b9a96c1e
	if (!nvmem->cell_post_process) {} should be if (nvmem->cell_post_process) {}, if we have this callback, we need do the post-processing.

> I have also added some test changes to imx provider driver as well, which you
> might have to take a closer look to get it working.
> 
> You need to look at adding/changing two things:
> 
> 1. setting reverse_mac_address flag in imx driver.
> 	Does IMX always has mac-address reversed? if yes then we do not need
> any new bindings for imx nvmem provider, if no we might need to add some
> kind of flag to indicate this.

No, it's depend on how to program the effuse. 
To avoid introducing consumer property in devicetree, I prefer to move reverse_mac_address
flag into ocotp_params struct, since each platforms has their own, it's easy to indicate this. I tried
it, and works.

> 2. In imx devicetree for mac-address nvmem cell make sure you add
> 
> cell-type = <NVMEM_CELL_TYPE_MAC_ADDRESS>;
> 
> 
> 
> 
> >
> >> Option 2: nvmem core handles the post processing.
> >>
> >> Pros:
> >> - provider driver does not need to implement callbacks
> >>
> >> Cons:
> >> - We have to find a way to define vendor specific non-standard
> >> encoding information in generic bindings which is going to be a
> >> challenge and high chance of ending up in to much of clutter in generic
> bindings.
> >>
> >> Finally, The way I look at this is that once we start adding
> >> post-processing in nvmem core then we might endup with code that will
> >> not be really used for most of the usecases and might endup with
> >> cases that might not be possible to handle in the core.
> >>
> >>
> >> Does Option 1 work for you?
> >
> > Yes, I also prefer to implement it in specific driver, as you mention
> > above, these code are for very rarely use cases.
> >
> > If we chose Option 1, I want to implement it totally in specific
> > driver(imx-ocotp.c), and I have a draft, could it be acdeptable?
> Yes, this is the direction, however we need a proper callback to do this. And
> offset information is still comes from Device tree.
> 
> 
> Have a look at the patches pushed into topic/post-processing branch.

I have improved this patch set according above comments and tested it. Also rebase to
the nvmem/for-next branch.

I plan to keep you as the nvmem part author and send out this patch set with dts changes. If it's fine for you?
  
Best Regards,
Joakim Zhang