Re: [PATCH v5 16/23] sg: rework sg_vma_fault

[Douglas Gilbert <dgilbert@interlog.com>]

On 2019-10-18 6:17 a.m., Hannes Reinecke wrote:
> On 10/8/19 9:50 AM, Douglas Gilbert wrote:
>> Simple refactoring of the sg_vma_fault() function.
>>
>> Signed-off-by: Douglas Gilbert <dgilbert@interlog.com>
>> ---
>>   drivers/scsi/sg.c | 33 +++++++++++++++++++++++----------
>>   1 file changed, 23 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 903faafaeff9..befcbfbcece1 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -1389,14 +1389,16 @@ sg_fasync(int fd, struct file *filp, int mode)
>>   	return fasync_helper(fd, filp, mode, &sfp->async_qp);
>>   }
>>   
>> +/* Note: the error return: VM_FAULT_SIGBUS causes a "bus error" */
>>   static vm_fault_t
>>   sg_vma_fault(struct vm_fault *vmf)
>>   {
>> -	struct vm_area_struct *vma = vmf->vma;
>> -	struct sg_fd *sfp;
>> +	int k, length;
>>   	unsigned long offset, len, sa;
>> +	struct vm_area_struct *vma = vmf->vma;
>>   	struct sg_scatter_hold *rsv_schp;
>> -	int k, length;
>> +	struct sg_device *sdp;
>> +	struct sg_fd *sfp;
>>   	const char *nbp = "==NULL, bad";
>>   
>>   	if (!vma) {
> Of course, one would prefer normal kernel-doc style for the comment ...

For static functions, that is left up to the discretion of the maintainer,
according to the document to which you refer :-)

I prefer comments that aren't compilable, IOWs that don't state the
bleeding obvious. While I was debugging sg_vma_fault() it took me a
while to understand why my test harness was crashing, hence that
comment.

Doug Gilbert

sg_vma_fault(
> Otherwise:
> Reviewed-by: Hannes Reinecke <hare@suse.com>
> 
> Cheers,
> 
> Hannes
>