linux-kernel - Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for Keystone 2 DSPs

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:   Sun, 25 Jun 2017 13:15:45 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Suman Anna <s-anna@...com>
Cc:     Ohad Ben-Cohen <ohad@...ery.com>, Rob Herring <robh+dt@...nel.org>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-remoteproc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, "Andrew F. Davis" <afd@...com>,
        Sam Nelson <sam.nelson@...com>
Subject: Re: [PATCH v3 2/3] remoteproc/keystone: Add a remoteproc driver for
 Keystone 2 DSPs

On Tue 13 Jun 16:45 PDT 2017, Suman Anna wrote:

> +static int keystone_rproc_start(struct rproc *rproc)
> +{
> +	struct keystone_rproc *ksproc = rproc->priv;
> +	int ret;
> +
> +	INIT_WORK(&ksproc->workqueue, handle_event);
> +
> +	ret = request_irq(ksproc->irq_ring, keystone_rproc_vring_interrupt, 0,
> +			  dev_name(ksproc->dev), ksproc);
> +	if (ret) {
> +		dev_err(ksproc->dev, "failed to enable vring interrupt, ret = %d\n",
> +			ret);
> +		goto out;
> +	}
> +
> +	ret = request_irq(ksproc->irq_fault, keystone_rproc_exception_interrupt,
> +			  0, dev_name(ksproc->dev), ksproc);
> +	if (ret) {
> +		dev_err(ksproc->dev, "failed to enable exception interrupt, ret = %d\n",
> +			ret);
> +		goto free_vring_irq;
> +	}

I do prefer that your request any resources during probe() and
potentially enable/disable them here. If below concern about using a
GPIO driver is cleared already I'll take it as is though.

[..]
> +static void keystone_rproc_kick(struct rproc *rproc, int vqid)
> +{
> +	struct keystone_rproc *ksproc = rproc->priv;
> +
> +	if (WARN_ON(ksproc->kick_gpio < 0))
> +		return;
> +
> +	gpio_set_value(ksproc->kick_gpio, 1);
> +}
> +

This doesn't sound like a gpio-controller and the GPIO maintainer did
reject an attempt by me to use the GPIO framework to abstract a similar
thing. Do you already have this driver upstream or have you clarified
with the maintainer that the GPIO framework is an acceptable abstraction
for this?

It looks equivalent to the "APCS IPC" register found in Qualcomm
platforms, previously implemented through a syscon but in v4.13 being
pushed to being a mailbox driver.

Apart from this I think the series looks good.

Regards,
Bjorn