On Mon, Aug 14, 2017 at 06:32:17PM +0200, Benjamin Block wrote:
> > - blk_end_request_all(rq, BLK_STS_OK);
> > -
> > put_device(job->dev); /* release reference for the request */
> >
> > kfree(job->request_payload.sg_list);
> > kfree(job->reply_payload.sg_list);
> > - kfree(job);
> > + blk_end_request_all(rq, BLK_STS_OK);
>
> What is the reason for moving that last line? Just wondering whether
> that might change the behavior somehow, although it doesn't look like it
> from the code.
The job is now allocated as part of the request, so we must fre
it last. The only change in behavior is that the reference gets dropped
a bit earlier, but once ownership is handed to the block layer
it's not needed, as are the memory allocations for the S/G lists.
> > +{
> > + struct bsg_job *job = blk_mq_rq_to_pdu(req);
> > +
> > + memset(job, 0, sizeof(*job));
> > + job->req = req;
> > + job->request = job->sreq.cmd;
>
> That doesn't work with bsg.c if the request submitted by the user is
> bigger than BLK_MAX_CDB. There is code in blk_fill_sgv4_hdr_rq() that
> will reassign the req->cmd point in that case to something else.
>
> This is maybe wrong in the same vein as my Patch 1 is. If not for the
> legacy code in bsg.c, setting this here, will miss changes to that
> pointer between request-allocation and job-submission.
>
> So maybe just move this to bsg_create_job().
Yes, this should be in indeed.
>
> > + job->dd_data = job + 1;
> > + job->reply = job->sreq.sense = kzalloc(job->reply_len, gfp);
>
> job->reply_len will be 0 here, won't it? You probably have to pull the
> "job->reply_len = SCSI_SENSE_BUFFERSIZE" here from bsg_create_job().
True.
