Message-ID: <3a3e506d-b40f-a407-27a9-c2e0bfdda97c@imgtec.com>
Date:   Wed, 5 Oct 2016 17:22:00 +0100
From:   Matt Redfearn <matt.redfearn@...tec.com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
CC:     Ralf Baechle <ralf@...ux-mips.org>,
        Ohad Ben-Cohen <ohad@...ery.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        <linux-mips@...ux-mips.org>, <linux-remoteproc@...r.kernel.org>,
        <lisa.parratt@...tec.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS

Hi Bjorn,

Thanks for the review and comments, much appreciated :-)


On 03/10/16 23:16, Bjorn Andersson wrote:
> On Tue 20 Sep 01:47 PDT 2016, Matt Redfearn wrote:
>
>> Add a remoteproc driver to steal, load the firmware, and boot an offline
>> MIPS core, turning it into a coprocessor.
>>
>> This driver provides a sysfs to allow arbitrary firmware to be loaded
>> onto a core, which may expose virtio devices. Coprocessor firmware must
>> abide by the UHI coprocessor boot protocol.
> Hi Matt,
>
> Sorry for my very slow response, I kept getting side tracked on the
> sysfs part every time I attempted to review this. After discussing with
> others it's obvious that being able to boot a remoteproc with a specific
> firmware image for some amount of time is a very common request.
>
> Rather than adding a MIPS specific interface for controlling this rproc
> I would like for us to bring this to the core.

Yes, makes perfect sense. I had a bit of fun allowing for the firmware 
name / information to be changed in an allocated struct rproc, but I 
have a proof of concept working now and will post it soon. The main 
issue remaining is that when the virtio devices are removed in the 
context of the sysfs write, a warning from dma-mapping.h is triggered 
because interrupts are disabled:

[   28.413958] WARNING: CPU: 0 PID: 121 at 
./include/linux/dma-mapping.h:433 free_buf+0x1a8/0x288
[   28.423542] Modules linked in:
[   28.426951] CPU: 0 PID: 121 Comm: sh Tainted: G        W 4.8.0+ #729
[   28.434600] Stack : 00000000 00000000 00000000 00000000 80d26cea 
0000003e 80c50000 00000000
       00000000 80c50000 80c50000 00040800 80c50000 8f658bc8 80b992dc 
00000079
       00000000 80d23824 00000000 0067544c 0067542c 8048af58 80c50000 
00000001
       80c50000 00000000 80b9fcd0 8f7c7b74 80d23824 8051d8bc 00000000 
0067544c
       00000007 80c50000 8f7c7b74 00040800 00000000 00000000 00000000 
00000000
       ...
[   28.474274] Call Trace:
[   28.477005] [<8040c538>] show_stack+0x74/0xc0
[   28.481860] [<80757240>] dump_stack+0xd0/0x110
[   28.486813] [<80430d98>] __warn+0xfc/0x130
[   28.491379] [<80430ee0>] warn_slowpath_null+0x2c/0x3c
[   28.497007] [<807e7c6c>] free_buf+0x1a8/0x288
[   28.501862] [<807ea590>] remove_port_data+0x50/0xac
[   28.507298] [<807ea6a0>] unplug_port+0xb4/0x1bc
[   28.512346] [<807ea858>] virtcons_remove+0xb0/0xfc
[   28.517689] [<807b6734>] virtio_dev_remove+0x58/0xc0
[   28.523223] [<807f918c>] __device_release_driver+0xac/0x134
[   28.529433] [<807f924c>] device_release_driver+0x38/0x50
[   28.535352] [<807f7edc>] bus_remove_device+0xfc/0x130
[   28.540980] [<807f4b74>] device_del+0x17c/0x21c
[   28.546027] [<807f4c38>] device_unregister+0x24/0x38
[   28.551562] [<807b6b50>] unregister_virtio_device+0x28/0x44
[   28.557777] [<80948ab0>] rproc_change_firmware+0xb4/0x114
[   28.563795] [<80949464>] firmware_store+0x2c/0x40
[   28.569039] [<8060186c>] kernfs_fop_write+0x154/0x1dc
[   28.574669] [<805823f0>] __vfs_write+0x5c/0x17c
[   28.579719] [<805834ac>] vfs_write+0xe0/0x190
[   28.584574] [<80584520>] SyS_write+0x80/0xf4
[   28.589336] [<80415908>] syscall_common+0x34/0x58
[   28.594570]
[   28.596228] ---[ end trace 3f6cae675f2fcee9 ]---


I'm still investigating how to decouple removal of the virtio devices 
from interrupts being disabled.


>
>
> I would also appreciate if Ralf had some input on the MIPS specifics.
>
>
> Also regarding the 32-bit requirement, have you investigated what is
> needed to support 64-bit ELFs?

Not yet - I wanted to get the 32bit version accepted first, then we can 
look at the required changes to support 64bit since no doubt that will 
necessitate quite a few changes to the core code.

>
> [..]
>> diff --git a/drivers/remoteproc/mips_remoteproc.c b/drivers/remoteproc/mips_remoteproc.c
> [..]
>> +int mips_rproc_op_start(struct rproc *rproc)
>> +{
>> +	struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
>> +	int err;
>> +	int cpu = mproc->cpu;
>> +
>> +	if (mips_rprocs[cpu]) {
>> +		dev_err(&rproc->dev, "CPU%d in use\n", cpu);
>> +		return -EBUSY;
>> +	}
>> +	mips_rprocs[cpu] = rproc;
>> +
>> +	/* Create task for the CPU to use before handing off to firmware */
>> +	mproc->tsk = fork_idle(cpu);
>> +	if (IS_ERR(mproc->tsk)) {
>> +		dev_err(&rproc->dev, "fork_idle() failed for CPU%d\n", cpu);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	/* We won't be needing the Linux IPIs anymore */
>> +	if (mips_smp_ipi_free(get_cpu_mask(cpu)))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Direct IPIs from the remote processor to CPU0 since that can't be
>> +	 * offlined while the remote CPU is running.
>> +	 */
>> +	mproc->ipi_linux = irq_reserve_ipi(ipi_domain(), get_cpu_mask(0));
>> +	if (!mproc->ipi_linux) {
>> +		dev_err(&mproc->dev, "Failed to reserve incoming kick\n");
>> +		goto exit_rproc_nofrom;
>> +	}
>> +
>> +	mproc->ipi_remote = irq_reserve_ipi(ipi_domain(), get_cpu_mask(cpu));
>> +	if (!mproc->ipi_remote) {
>> +		dev_err(&mproc->dev, "Failed to reserve outgoing kick\n");
>> +		goto exit_rproc_noto;
>> +	}
>> +
>> +	/* register incoming ipi */
>> +	err = devm_request_threaded_irq(&mproc->dev, mproc->ipi_linux,
>> +					mips_rproc_ipi_handler,
>> +					mips_rproc_vq_int, 0,
>> +					"mips-rproc IPI in", mproc->rproc);
> Based on how you've designed this I think it makes sense to just depend
> on the fact that stop() will always be called and hence you do not
> benefit from the devm_ version of this api.

Yeah, makes sense.

>
>> +	if (err) {
>> +		dev_err(&mproc->dev, "Failed to register incoming kick: %d\n",
>> +			err);
>> +		goto exit_rproc_noint;
>> +	}
>> +
>> +	if (!mips_cps_steal_cpu_and_execute(cpu, &mips_rproc_cpu_entry,
>> +						mproc->tsk))
>> +		return 0;
> Please flip this around, to follow the pattern of the others, like:
>
> 	if (mips_cps_steal_cpu_and_execute()) {
> 		dev_err()
> 		goto exit_free_irq;
> 	}
>
> 	return 0;
>
> exit_free_irq:

OK.

>
>> +
>> +	dev_err(&mproc->dev, "Failed to steal CPU%d for remote\n", cpu);
>> +	devm_free_irq(&mproc->dev, mproc->ipi_linux, mproc->rproc);
>> +exit_rproc_noint:
>> +	irq_destroy_ipi(mproc->ipi_remote, get_cpu_mask(cpu));
>> +exit_rproc_noto:
>> +	irq_destroy_ipi(mproc->ipi_linux, get_cpu_mask(0));
>> +exit_rproc_nofrom:
>> +	free_task(mproc->tsk);
>> +	mips_rprocs[cpu] = NULL;
>> +
>> +	/* Set up the Linux IPIs again */
>> +	mips_smp_ipi_allocate(get_cpu_mask(cpu));
>> +	return -EINVAL;
>> +}
>> +
>> +int mips_rproc_op_stop(struct rproc *rproc)
>> +{
>> +	struct mips_rproc *mproc = *(struct mips_rproc **)rproc->priv;
>> +
>> +	if (mproc->ipi_linux)
> stop() should not be called unless start() succeeded, so ipi_linux
> should not be able to be 0.

True.

>
>> +		devm_free_irq(&mproc->dev, mproc->ipi_linux, mproc->rproc);
>> +
>> +	irq_destroy_ipi(mproc->ipi_linux, get_cpu_mask(0));
>> +	irq_destroy_ipi(mproc->ipi_remote, get_cpu_mask(mproc->cpu));
>> +
>> +	/* Set up the Linux IPIs again */
>> +	mips_smp_ipi_allocate(get_cpu_mask(mproc->cpu));
>> +
>> +	free_task(mproc->tsk);
>> +
>> +	mips_rprocs[mproc->cpu] = NULL;
>> +
>> +	return mips_cps_halt_and_return_cpu(mproc->cpu);
>> +}
>> +
>> +
> [..]
>> +
>> +/* Steal a core and run some firmware on it */
>> +int mips_rproc_start(struct mips_rproc *mproc, const char *firmware, size_t len)
>> +{
>> +	int err = -EINVAL;
>> +	struct mips_rproc **priv;
>> +
>> +	/* Duplicate the filename, dropping whitespace from the end via len */
>> +	mproc->firmware = kstrndup(firmware, len, GFP_KERNEL);
>> +	if (!mproc->firmware)
>> +		return -ENOMEM;
>> +
>> +	mproc->rproc = rproc_alloc(&mproc->dev, "mips", &mips_rproc_proc_ops,
>> +				   mproc->firmware,
>> +				   sizeof(struct mips_rproc *));
>> +	if (!mproc->rproc)
>> +		return -ENOMEM;
>> +
>> +	priv = mproc->rproc->priv;
>> +	*priv = mproc;
> If we move the class into the core, everyone will share the same
> interface for setting firmware, booting and shutting down remoteproc.

Yes, that sounds like the best way forward.

>
> I think you should set rproc->auto_boot to false and move this code into
> the probe function above. It would make there be an remoteproc instance
> whenever the cpu is offlined, do you see any problems with this?

No problem with it being available any time the CPU is offline.


>
>> +
>> +	/* go live! */
>> +	err = rproc_add(mproc->rproc);
>> +	if (err) {
>> +		dev_err(&mproc->dev, "Failed to add rproc: %d\n", err);
>> +		rproc_put(mproc->rproc);
>> +		kfree(mproc->firmware);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/* Stop a core, and return it to being offline */
>> +int mips_rproc_stop(struct mips_rproc *mproc)
>> +{
>> +	rproc_shutdown(mproc->rproc);
> I presume this shutdown is related to the implicit boot happening in
> rproc_add() if you have virtio devices; I've changed this for v4.9 so
> that rproc_del() shuts down the core if rproc_add() booted it.

Yeah it is, ok great.

>
> There needs to be some more work done in this area though, because there
> are plenty of corner cases that we don't handle properly today...
>
>> +	rproc_del(mproc->rproc);
>> +	rproc_put(mproc->rproc);
>> +	mproc->rproc = NULL;
>> +	return 0;
>> +}
>> +
> [..]
>> +
>> +/* sysfs interface to mips_rproc_stop */
>> +static ssize_t stop_store(struct device *dev,
>> +			      struct device_attribute *attr,
>> +			      const char *buf, size_t count)
>> +{
>> +	struct mips_rproc *mproc = to_mips_rproc(dev);
>> +	int err = -EINVAL;
>> +
>> +
>> +	if (mproc->rproc)
>> +		err = mips_rproc_stop(mproc);
>> +	else
>> +		err = -EBUSY;
>> +
>> +	return err ? err : count;
>> +}
>> +static DEVICE_ATTR_WO(stop);
> Please move this control into the core, preferably as "state" which can
> be passed "boot" or "shutdown" - i.e. what we today have in debugfs.

OK, though I've stuck with "start" and "stop" as are in the debugfs version.

Thanks,
Matt


>
>> +
>> +/* Boiler plate for devclarng mips-rproc sysfs devices */
>> +static struct attribute *mips_rproc_attrs[] = {
>> +	&dev_attr_firmware.attr,
>> +	&dev_attr_stop.attr,
>> +	NULL
>> +};
>> +
> Regards,
> Bjorn

Powered by blists - more mailing lists