(I would wish to be personally CC'ed the answers/comments posted to the list in response to my posting.)

---

Removing the wrong comment.
The lock is needed before calling new_tape_buffer(), at least in some cases.
So the comment above new_tape_buffer() is inconsistent with the code and
may mislead developers.

I simply removed the wrong comment, as I am not sure if the lock is required
in all situations. If so, we should add "Caller must hold os_scsi_tapes_lock".

Signed-off-by: Lin Tan <tammy000@gmail.com>

---

--- a/drivers/scsi/osst.c	2008-09-25 11:53:09.000000000 -0500
+++ b/drivers/scsi/osst.c	2008-09-25 11:59:46.000000000 -0500
@@ -5209,7 +5209,7 @@
 
 /* Memory handling routines */
 
-/* Try to allocate a new tape buffer skeleton. Caller must not hold os_scsi_tapes_lock */
+/* Try to allocate a new tape buffer skeleton. */
 static struct osst_buffer * new_tape_buffer( int from_initialization, int need_dma, int max_sg )
 {
 	int i;
--

[ message continues ]

On Thu, 25 Sep 2008 12:49:48 -0500

Looks true to me for the current versions of the code. In fact it is only
ever called from the initialisation function that I can see so chunks of
the code could simply go away as well as bits of the comment. Ditto the
one in drivers/scsi/st.c

Acked-by: Alan Cox <alan@redhat.com>

--

[ message continues ]

I am sorry I didn't quite understand. You mean it is true that caller
must hold os_scsi_tapes_lock?

new_tape_buffer in drivers/scsi/st.c is called without the lock, but
the new_tape_buffer in drivers/scsi/osst.c
is called with the lock. Both comments says no lock is needed. Should
the two new_tap_buffer functions have similar usage?

BTW, I am on the mailing list now, so I no longer need to be
personally CC-ed. Thanks.

Lin
--

[ message continues ]

Sorry - I mean what you claim is true - that the comment is incorrect.
--

[ message continues ]

So, I think I'm missing a piece of this:  There's no patch in this
thread (I presume it's lurking somewhere on lkml).  Could someone repost
the proposed patch and copy the tape maintainer: Kai Makisara
<Kai.Makisara@kolumbus.fi> to get his input?

Thanks,

James


--

[ message continues ]

The patch was in the orginal message. I am resending it now with Makisara CC-ed.

Lin 
[ message continues ]

I am talking only about st.c but most of this probably applies also to 
osst.c.


It is true that the function is currently called only from initialisation. 
It was called from other contexts earlier and the extra code is leftover 
I saw the patch but my mail client does not include attachments as text in 
reply ;-(

Anyway, the comment is there because new_tape_buffer() may sleep and 
st_dev_arr_lock is a spinlock. I have been under the impression that code 
inside a spinlock must not sleep.

When called from initialisation, new_tape_buffer() _currently_ used 
GFP_ATOMIC. I am not sure that this is necessary but as long as I don't 
know, I don't want to change it. The comment protects callers even if 
someone knows that it is safe to use GFP_KERNEL from initialisation.

I don't see why new_tape_buffer() needs to be inside the lock. It just 
allocates an initialises a structure.

If someone wants to remove the comment from st.c, I don't mind. But please 
make sure that what you write to the commit is correct.

Kai

--

[ message continues ]