Re: [PATCH v3 3/5] st: get rid of scsi_tapes array

[Lee Duncan]

Kai:

Your added patch looks great, and I see you fixed the documentation as
well. Thanks for your help.


On 07/02/2012 05:16 PM, Lee Duncan wrote:
> 
> On 07/01/2012 01:57 AM, Kai Makisara wrote:
>> On Mon, 21 May 2012, Lee Duncan wrote:
>>
>>> From: Jeff Mahoney <jeffm@xxxxxxxx>
>>>
>>> st currently allocates an array to store pointers to all of the
>>> scsi_tape objects. It's used to discover available indexes to use as the
>>> base for the minor number selection and later to look up scsi_tape
>>> devices for character devices.
>>>
>>> We switch to using an IDR for minor selection and a pointer from
>>> st_modedef back to scsi_tape for the lookups.
>>>
>>> Signed-off-by: Jeff Mahoney <jeffm@xxxxxxxx>
>>> Signed-off-by: Lee Duncan <lduncan@xxxxxxxx>
>>> ---
>>>  drivers/scsi/st.c |  172 ++++++++++++++++++++---------------------------------
>>>  drivers/scsi/st.h |    2 +
>>>  2 files changed, 65 insertions(+), 109 deletions(-)
>>>
>> ... patch removed
>>
>> I have finally had time to review and test this patch set. I am sorry this 
>> has taken so long.
>>
>> I have found one change of behaviour and a theoretical problem:
>> The new code does not re-use the tape numbers when freed and re-scanned. 
>> The current code does re-use the freed numbers. Are there any reasons for 
>> this changed behaviour? (The theoretical problem is that the new code 
>> frees the tape structure but leaves the pointer in the idr tree.)
>>
>> The patch at the end of this message (applies after the whole series) is 
>> an attempt to implement re-use of tape numbers. I am not completely sure 
>> that the change is correctly placed but it seems to work.
> 
> 
> Thanks for the review, and good catch. I'll look over your added patch
> and give feedback as soon as I can.
> 
>>
>> Another minor thing is that the documentation should be updated :-)
> 
> Of course.
> 
>>
>> The patch at the end also updates the version code. I am not sure if the 
>> version code is useful, but it should be either updated or removed.
>>
>> Otherwise no problems found. I am ready to ack the patch set after the 
>> re-use thing has been resolved (one way or another).
>>
>> Thanks,
>> Kai
>>

- Lee

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/