linux-kernel - Re: [PATCH v3 7/9] mfd: intel-m10-bmc: Add PMCI driver

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, 5 Dec 2022 23:08:24 +0800
From:   Xu Yilun <yilun.xu@...el.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        linux-fpga@...r.kernel.org, Wu Hao <hao.wu@...el.com>,
        Tom Rix <trix@...hat.com>, Moritz Fischer <mdf@...nel.org>,
        Lee Jones <lee@...nel.org>,
        Matthew Gerlach <matthew.gerlach@...ux.intel.com>,
        Russ Weight <russell.h.weight@...el.com>,
        Tianfei zhang <tianfei.zhang@...el.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Marco Pagani <marpagan@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 7/9] mfd: intel-m10-bmc: Add PMCI driver

On 2022-12-05 at 12:05:14 +0000, Mark Brown wrote:
> On Mon, Dec 05, 2022 at 11:51:15AM +0200, Ilpo Järvinen wrote:
> > On Sat, 3 Dec 2022, Xu Yilun wrote:
> > > On 2022-12-02 at 12:08:39 +0200, Ilpo Järvinen wrote:
> 
> > > > +struct regmap *__devm_m10_regmap_indirect(struct device *dev,
> 
> > > We name the file intel-m10-bmc-pmci-xxx.c, and this function
> > > xx_m10_regmap_xx(). But I can see the implementation is just about the indirect
> > > bus which from your commit message could be used by various DFL features
> > > like HSSI or PMCI. So is it better we put the implementation in
> > > drivers/fpga and name the file dfl-indirect-regmap.c and the
> > > initialization function dfl_indirect_regmap_init()?
> 
> > I guess that would be doable unless Mark objects. My understanding was 
> > that he preferred to have in the driver that is currently using it.
> 
> > Mark, any opinion on this?
> 
> The above does not look good.  As I have said several times now drivers
> implementing their own regmap operations should use the reg_read() and
> reg_write() operations in regmap_config when allocating their regmap
> unless they're doing something unusual.  There are a few cases where it
> makes sense but nothing I've seen here makes it look like this is one of
> them.  Most of the current users don't fit.

It is good for now to implement the indirect access interface in
regmap_config, as intel-m10-bmc is the only one who uses it. But I'm not
sure when a second IP block(like HSSI) in intel FPGA uses it, how to
implement? A shared library?

Some background about hardware:
Several IP blocks in intel FPGA integrate the same mmio register layout
(so called indirect access interface here) as the bridge to the IP's real
registers address space. Like:

 +---------+          +---------+
 | m10 BMC |          |  HSSI   |
 +---------+          +---------+
 |indirect |          |indirect |
 | access  |          | access  |
 |  MMIOs  |          |  MMIOs  |
 +----+----+          +----+----+
      |                    |
      |                    |
 +----+-----+         +---------+
 |m10 bmc   |         | HSSI    |
 |registers |         |registers|
 +----------+         +---------+

Thanks,
Yilun

> 
> Please, just implement a normal driver using a normal regmap_config as
> I've repeatedly said you should if you don't want to provide something
> generic.