lkml.org 
[lkml]   [2012]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCHv3] remoteproc: Add STE modem driver for remoteproc
Hi Sjur,

On Wed, Sep 19, 2012 at 7:39 PM, <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add support for the STE modem shared memory driver.
> This driver hooks into the remoteproc framework
> in order to manage configuration and the virtio
> devices.
>
> This driver adds custom firmware handlers, because
> STE modem uses a custom firmware layout.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> cc: Linus Walleij <linus.walleij@linaro.org>
> cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
> Changes since v3:

Looks good, thanks.

I have just one comment/question:

> +/* STE modem device is unregistered */
> +static int sproc_drv_remove(struct platform_device *pdev)
> +{
> + struct ste_modem_device *mdev =
> + container_of(pdev, struct ste_modem_device, pdev);
> + struct sproc *sproc = mdev->drv_data;
> +
> + sproc_dbg(sproc, "remove ste-modem\n");
> +
> + /* Unregister as remoteproc device */
> + rproc_del(sproc->rproc);
> + rproc_put(sproc->rproc);
> +
> + mdev->drv_data = NULL;
> + mdev->drv_ops = NULL;
> + return 0;
> +}
> +
> +struct ste_modem_drv_ops sproc_drv_ops = {
> + .kick = sproc_kick_callback
> +};
> +
> +/* Handle probe of a modem device */
> +static int sproc_probe(struct platform_device *pdev)
> +{
...
> + mdev->drv_data = sproc;
> +
> + /* Provide callbacks to modem device */
> + mdev->drv_ops = &sproc_drv_ops;

Implicitly providing the modem with drv_ops and drv_data in this
manner feels racy and somewhat error prone.

E.g., on remove these members are set to NULL, under the assumption
that the modem won't invoke sproc_kick_callback or access sproc
anymore, but this doesn't feel safe.

Any chance you can add an explicit registration method to
ste_modem_dev_ops, with which you'll be able to explicitly set/unset
sproc_kick_callback and sproc ?

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2012-09-20 12:41    [W:0.059 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site