[PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[Lin Tan]

(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 @@
 \f
 /* 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;

Re: [PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[Alan Cox]

On Thu, 25 Sep 2008 12:49:48 -0500
Lin Tan <tammy000@gmail.com> wrote:

> (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>

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>

Re: [PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[Tammy]

>> 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>
>
> 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>
>

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

Re: [PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[Alan Cox]

> > 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>
> >
> 
> I am sorry I didn't quite understand. You mean it is true that caller
> must hold os_scsi_tapes_lock?

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

Re: [PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[James Bottomley]

On Fri, 2008-09-26 at 17:09 +0100, Alan Cox wrote:
> > > 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>
> > >
> > 
> > I am sorry I didn't quite understand. You mean it is true that caller
> > must hold os_scsi_tapes_lock?
> 
> Sorry - I mean what you claim is true - that the comment is incorrect.

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

Re: [PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[Lin Tan]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

> > > > 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>
> > > >
> > > 
> > > I am sorry I didn't quite understand. You mean it is true that caller
> > > must hold os_scsi_tapes_lock?
> > 
> > Sorry - I mean what you claim is true - that the comment is incorrect.
> 
> 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
> 
>

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

Lin 

[-- Attachment #2: new_tape_buffer.readytosend --]
[-- Type: text/plain, Size: 845 bytes --]

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 @@
 \f
 /* 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;

Re: [PATCH git latest] drivers/scsi: fixing wrong comment before new_buffer_tape()

[Kai Makisara]

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

On Fri, 26 Sep 2008, Lin Tan wrote:

> > > > > 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

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 
from that time.

> > > > > one in drivers/scsi/st.c
> > > > >
> > > > > Acked-by: Alan Cox <alan@redhat.com>
> > > > >
> > > > 
> > > > I am sorry I didn't quite understand. You mean it is true that caller
> > > > must hold os_scsi_tapes_lock?
> > > 
> > > Sorry - I mean what you claim is true - that the comment is incorrect.
> > 
> > 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?
> > 
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