Re: [PATCH git latest] drivers/scsi: fixing wrong comment beforenew_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@xxxxxxxxxx>
> > > > >
> > > > 
> > > > 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@xxxxxxxxxxx> 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

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