On Thu, 2015-05-21 at 18:58 +1000, Ian Munsie wrote:
> Hi Mikey,
>
> > +/* wrappers around afu_* file ops which are EXPORTED */
>
> This is fine, though alternatively you could export the original
> functions directly from file.c (feel free to rename them to your
> versions if you do change it) - I don't really mind either way :)
I'll probably just keep it so all the API functions are in one spot.
> > +static void cxl_pci_reset_secondary_bus(struct pci_dev *dev)
> > +{
> > + /* Should we do an AFU reset here ? */
>
> I'm still not sure what the best answer should be to this question... At
> the moment it's working fine without an afu reset here, so we can delay
> adding it until (and if) it becomes clear we need it?
Yeah I think I'll leave it out
> Could this be called while the afu is in use (need to be careful), or
> should it only ever be called when the afu is inactive (afu reset should
> be safe)?
Yeah, I assume we'd need to give some EEH events to the attached driver
so they could handle the AFU losing all that state/contexts.
> > +/*
> > + * Context lifetime overview:
> > + *
> > + * An AFU context may be inited and then started and stoppped multiple
> > times
> > + * before it's released. ie.
> > + * - cxl_dev_context_init()
> > + * - cxl_start_context()
> > + * - cxl_stop_context()
> > + * - cxl_start_context()
> > + * - cxl_stop_context()
> > + * ...repeat...
> > + * - cxl_release_context()
> > + * Once released, a context can't be started again.
>
> Ok, we'll need to be a little careful here as this differs from the
> userspace api (which cannot reuse a context and relies on the file
> descriptor release() to stop the context).
>
> Let's see...
Yep.
>
> > +int cxl_start_context(struct cxl_context *ctx, u64 wed,
> > + struct task_struct *task)
> > +{
> > + int rc = 0;
> > + bool kernel = true;
> > +
> > + pr_devel("%s: pe: %i\n", __func__, ctx->pe);
> > +
> > + mutex_lock(&ctx->status_mutex);
> > + if (ctx->status == STARTED)
> > + goto out; /* already started */
> > + if (task) {
> > + ctx->pid = get_task_pid(task, PIDTYPE_PID);
> > + get_pid(ctx->pid);
>
> OK, this pid is put in the release call (in reclaim_ctx, which is an rcu
> callback from cxl_context_free), which means if we reuse a context we
> will prevent the pid from ever being reused, reducing the total number
> of runnable processes by one.
As discussed offline, I've moved this around.
> > + kernel = false;
> > + }
> > +
> > + cxl_ctx_get();
>
> Likewise this is mirrored in the release call instead of the stop call,
> so if we reuse a context we will then permanently mark cxl as being in
> use, which will then permanently enable the slbia hook.
Ditto.
>
> > + if ((rc = cxl_attach_process(ctx, kernel, wed , 0))) {
> > + put_pid(ctx->pid);
> > + cxl_ctx_put();
> > + goto out;
> > + }
> > +
> > + ctx->status = STARTED;
> > + get_device(&ctx->afu->dev);
> > +out:
> > + mutex_unlock(&ctx->status_mutex);
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_start_context);
>
> > +/* Stop a context. Returns 0 on success, otherwise -Errno */
> > +int cxl_stop_context(struct cxl_context *ctx)
> > +{
> > + int rc;
> > +
> > + rc = __detach_context(ctx);
> > + if (!rc)
> > + put_device(&ctx->afu->dev);
> > + return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_stop_context);
>
> > +int cxl_release_context(struct cxl_context *ctx)
> > +{
> > + if (ctx->status != CLOSED)
> > + return -EBUSY;
> > +
> > + cxl_context_free(ctx);
> > +
> > + cxl_ctx_put();
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(cxl_release_context);
>
> Otherwise it looks good :)
Thanks,
Mikey
>
> Cheers,
> -Ian
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
