Hi Rob,
On Fri, Feb 1, 2013 at 4:24 AM, Robert Tivy <rtivy@xxxxxx> wrote:
> drivers/remoteproc/Kconfig | 29 ++-
> drivers/remoteproc/Makefile | 1 +
> drivers/remoteproc/da8xx_remoteproc.c | 346 +++++++++++++++++++++++++++++++++
> drivers/remoteproc/remoteproc_core.c | 22 ++-
It looks like this patch squashes:
1. A fix to drivers/remoteproc/Kconfig
2. New functionality in remoteproc_core.c
3. A new da8xx driver
These are independent changes, so we better split them up to separate patches.
> config OMAP_REMOTEPROC
> tristate "OMAP remoteproc support"
> - depends on EXPERIMENTAL
...
> config STE_MODEM_RPROC
> tristate "STE-Modem remoteproc support"
> - depends on EXPERIMENTAL
These look unrelated to this patch, and it seems that Kees Cook
submitted these awhile ago so it should probably already be in
linux-next (haven't looked). I think we can drop these.
> +/**
> + * handle_event() - inbound virtqueue message workqueue function
> + *
> + * This function is registered as a kernel thread and is scheduled by the
> + * kernel handler.
> + */
> +static irqreturn_t handle_event(int irq, void *p)
> +{
> + struct rproc *rproc = (struct rproc *)p;
> +
> + /* Process incoming buffers on our vring */
> + while (IRQ_HANDLED == rproc_vq_interrupt(rproc, 0))
> + ;
> +
> + /* Must allow wakeup of potenitally blocking senders */
> + rproc_vq_interrupt(rproc, 1);
IIRC we agreed on removing this part, and instead adding it to the
generic remoteproc framework (i.e. Cyril's patch).
> +static int da8xx_rproc_start(struct rproc *rproc)
> +{
...
> + dsp_clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(dsp_clk)) {
> + dev_err(dev, "clk_get error: %ld\n", PTR_ERR(dsp_clk));
> +
> + return PTR_RET(dsp_clk);
> + }
> + drproc->dsp_clk = dsp_clk;
Have you considered doing this in ->probe() instead? Do we need to
call get/put every time we start/stop the remote processor?
> +/* kick a virtqueue */
> +static void da8xx_rproc_kick(struct rproc *rproc, int vqid)
> +{
> + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
> + int timed_out;
> + unsigned long timeout;
> +
> + timed_out = 0;
> + timeout = jiffies + HZ/100;
> +
> + /* Poll for ack from other side first */
> + while (readl(drproc->chipsig) & SYSCFG_CHIPSIG2)
> + if (time_after(jiffies, timeout)) {
> + if (readl(drproc->chipsig) & SYSCFG_CHIPSIG2) {
> + dev_err(rproc->dev.parent,
> + "failed to receive ack\n");
> + timed_out = 1;
> + }
> +
> + break;
> + }
This still looks a bit out of place here.
Kicking should be a fast unilateral action, that doesn't depend on the
other side.
> +static int da8xx_rproc_probe(struct platform_device *pdev)
> +{
...
> + ret = rproc_add(rproc);
> + if (ret) {
> + dev_err(dev, "rproc_add failed: %d\n", ret);
> + goto free_rproc;
> + }
> +
> + drproc->chipsig = chipsig;
> + drproc->bootreg = bootreg;
> + drproc->ack_fxn = irq_data->chip->irq_ack;
> + drproc->irq_data = irq_data;
> + drproc->irq = irq;
> +
> + /* everything the ISR needs is now setup, so hook it up */
> + ret = devm_request_threaded_irq(dev, irq, da8xx_rproc_callback,
> + handle_event, 0, "da8xx-remoteproc", rproc);
> + if (ret) {
> + dev_err(dev, "devm_request_threaded_irq error: %d\n", ret);
> + rproc_del(rproc);
> + goto free_rproc;
> + }
Shouldn't we be requesting this before we rproc_add() ?
> +static int da8xx_rproc_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct rproc *rproc = platform_get_drvdata(pdev);
> + struct da8xx_rproc *drproc = (struct da8xx_rproc *)rproc->priv;
> + int ret;
...
> + ret = rproc_del(rproc);
You can safely remove 'ret' altogether. We will just crash in
rproc_put if rproc is NULL.
> + rproc_put(rproc);
> +
> + return ret;
> +}
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index dd3bfaf..e0f703e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1222,19 +1222,39 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> const char *firmware, int len)
> {
> struct rproc *rproc;
> + char *template = "rproc-%s-fw";
> + char *p;
>
> if (!dev || !name || !ops)
> return NULL;
>
> + if (!firmware)
> + /*
> + * Make room for default firmware name (minus %s plus '\0').
> + * If the caller didn't pass in a firmware name then
> + * construct a default name. We're already glomming 'len'
> + * bytes onto the end of the struct rproc allocation, so do
> + * a few more for the default firmware name (but only if
> + * the caller doesn't pass one).
> + */
> + len += strlen(name) + strlen(template) - 2 + 1;
> +
> rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL);
> if (!rproc) {
> dev_err(dev, "%s: kzalloc failed\n", __func__);
> return NULL;
> }
>
> + if (!firmware) {
> + p = (char *)rproc + sizeof(struct rproc) + len;
> + sprintf(p, template, name);
Looks like p you're writing to is outside of the rproc memory allocation.
Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
