Hi Sjur,
On Wed, Sep 19, 2012 at 7:39 PM, <sjur.brandel...@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandel...@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.brandel...@stericsson.com>
> cc: Linus Walleij <linus.wall...@linaro.org>
> cc: Alan Cox <a...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
