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-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
