linux-kernel - RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc 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:   Thu, 17 Sep 2020 22:18:39 +0000
From:   Ben Levinsky <BLEVINSK@...inx.com>
To:     Michael Auchter <michael.auchter@...com>
CC:     "punit1.agrawal@...hiba.co.jp" <punit1.agrawal@...hiba.co.jp>,
        Stefano Stabellini <stefanos@...inx.com>,
        Michal Simek <michals@...inx.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>,
        "Ed T. Mooring" <emooring@...inx.com>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jiaying Liang <jliang@...inx.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Jiaying Liang <jliang@...inx.com>,
        Michal Simek <michals@...inx.com>,
        "Ed T. Mooring" <emooring@...inx.com>, Jason Wu <j.wu@...inx.com>
Subject: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
 driver

Hi Michael,

Thanks for the comments,

> -----Original Message-----
> From: Michael Auchter <michael.auchter@...com>
> Sent: Thursday, September 17, 2020 3:11 PM
> To: Ben Levinsky <BLEVINSK@...inx.com>
> Cc: punit1.agrawal@...hiba.co.jp; Stefano Stabellini <stefanos@...inx.com>;
> Michal Simek <michals@...inx.com>; devicetree@...r.kernel.org;
> mathieu.poirier@...aro.org; Ed T. Mooring <emooring@...inx.com>; linux-
> remoteproc@...r.kernel.org; linux-kernel@...r.kernel.org; Jiaying Liang
> <jliang@...inx.com>; robh+dt@...nel.org; linux-arm-
> kernel@...ts.infradead.org; Jiaying Liang <jliang@...inx.com>; Michal Simek
> <michals@...inx.com>; Ed T. Mooring <emooring@...inx.com>; Jason Wu
> <j.wu@...inx.com>
> Subject: Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> driver
> 
> Hey Ben,
> 
> Split mode is still not functional in this patch series (as was the case
> with the last few revisions).
> 
> Before sending out the next revision, can you _please_ ensure you're
> testing all supported configurations?
> 
[Ben Levinsky]  I will make sure to update in next revision.
 As per review, I tested on QEMU and hardware firmware loading in split mode on R5 0 split, R5 1 split and R5 lockstep and  was  able to successfully load, start and establish IPC links

That being said, I will update the to reflect the values between the enum for rpu operation mode and the documentation in the binding.

For testing, I can provide a pointer to a publicly available device tree I am using if that helps. If not, can you expand on the testing of supported configurations?

> On Thu, Sep 17, 2020 at 12:43:41PM -0700, Ben Levinsky wrote:
> > +/**
> > + * RPU core configuration
> > + */
> > +static enum rpu_oper_mode rpu_mode;
> > +
> 
> <.. snip ..>
> 
> > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
> > +{
> > +	int ret, i = 0;
> > +	u32 lockstep_mode;
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *nc;
> > +
> > +	ret = of_property_read_u32(dev->of_node,
> > +				   "lockstep-mode",
> > +				   &lockstep_mode);
> > +	if (ret < 0) {
> > +		return ret;
> > +	} else if (lockstep_mode != PM_RPU_MODE_LOCKSTEP &&
> > +	    lockstep_mode != PM_RPU_MODE_SPLIT) {
> > +		dev_err(dev,
> > +			"Invalid lockstep-mode %x in %pOF\n",
> > +			lockstep_mode, dev->of_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	rpu_mode = lockstep_mode;
> > +
> > +	dev_dbg(dev, "RPU configuration: %s\n",
> > +		lockstep_mode ? "lockstep" : "split");
> 
> The binding documents lockstep-mode as:
> 
> > +  lockstep-mode:
> > +    description:
> > +      R5 core configuration (split is 0 or lock-step and 1)
> > +    maxItems: 1
> 
will update this as you note so that lockstep and split mode are accurately reflected. 

> (Which needs to be reworded, but it looks like the intent was "split is
> 0 and lock-step is 1")
> 
> However, rpu_oper_mode is defined as:
> 
> > +enum rpu_oper_mode {
> > +       PM_RPU_MODE_LOCKSTEP = 0,
> > +       PM_RPU_MODE_SPLIT = 1,
> > +};
> 
> so the assignment "rpu_mode = lockstep_mode" is incorrect.
> 
once the binding is updated, why would this still be incorrect? Assuming the documentation is updated, the above line would be ok, right?

Thank you for the review
Ben

> - Michael