RE: [PATCH v2 7/9] accel/neutron: Add job submission IOCTL

[Ioana Ciocoi Radulescu]

On Friday, March 6, 2026 at 7:03 PM, Frank Li wrote:
> > +	if (appstatus & APPSTATUS_FAULTCAUSE_MASK) {
> > +		dev_err(ndev->dev, "Neutron halted due to fault: 0x%lx\n",
> > +			FIELD_GET(APPSTATUS_FAULTCAUSE_MASK,
> appstatus));
> > +		return neutron_job_err_handler(ndev);
> 
> AI: neutron_job_err_handler() returns void, not int. Remove 'return'.

Ok, will fix.

> 
> > +	ret = drm_sched_job_init(&job->base, &npriv->sched_entity, 1, NULL,
> > +				 filp->client_id);
> > +	if (ret)
> > +		goto out_put_syncobj;
> > +
> > +	ret = neutron_push_job(job, syncobj);
> > +	if (ret)
> > +		goto out_sched_cleanup;
> > +
> > +	neutron_put_job(job);
> > +	drm_syncobj_put(syncobj);
> > +
> > +	return 0;
> > +
> > +out_sched_cleanup:
> > +	drm_sched_job_cleanup(&job->base);
> > +out_put_syncobj:
> > +	drm_syncobj_put(syncobj);
> > +out_put_gem:
> > +	drm_gem_object_put(job->bo);
> 
> AI: In the success path, neutron_put_job(job) is called which decrements
> refcnt. But if neutron_push_job() fails and we hit out_sched_cleanup, the job
> refcnt is never decremented. This leaks the job structure.
> Consider: if neutron_push_job() succeeds, it calls kref_get() inside sched_lock.
> If it fails, no kref_get() happens, so don't call
> 
> (Need owner do judgment. Not sure if AI said correctly.)

I don't see an issue here, kref_get() is called at a point where
neutron_push_job() can't fail anymore. And if neutron_push_job() fails
earlier, error path looks clean, it frees everything in reverse order,
including the job struct.

Btw, what agent did you use for review?

Thanks,
Ioana

> 
> Frank