<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2//EN">
<HTML>
<HEAD>
<META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=iso-8859-1">
<META NAME="Generator" CONTENT="MS Exchange Server version 5.5.2654.45">
<TITLE>RE: [openib-general] Re: [PATCH] SRP: don't use TX IU after freeing it</TITLE>
</HEAD>
<BODY>
<P><FONT SIZE=2>Hi Roland,</FONT>
<BR><FONT SIZE=2>When do you expect to apply the FMRs patch for SRP?</FONT>
</P>
<P><FONT SIZE=2>Thanks,</FONT>
<BR><FONT SIZE=2>Tziporet</FONT>
</P>
<P><FONT SIZE=2>-----Original Message-----</FONT>
<BR><FONT SIZE=2>From: Vu Pham [<A HREF="mailto:vuhuong@mellanox.com">mailto:vuhuong@mellanox.com</A>]</FONT>
<BR><FONT SIZE=2>Sent: Tuesday, October 11, 2005 8:03 PM</FONT>
<BR><FONT SIZE=2>To: Roland Dreier</FONT>
<BR><FONT SIZE=2>Cc: kingman@storagegear.com; openib-general@openib.org</FONT>
<BR><FONT SIZE=2>Subject: [openib-general] Re: [PATCH] SRP: don't use TX IU after freeing</FONT>
<BR><FONT SIZE=2>it</FONT>
</P>
<BR>
<P><FONT SIZE=2>Roland,</FONT>
<BR><FONT SIZE=2> Thanks or reviewing it.</FONT>
<BR><FONT SIZE=2> Responding to your feedback, I prepare new patch (attached)</FONT>
</P>
<BR>
<P><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> Why put a pointer to struct list_head here instead of just a struct</FONT>
<BR><FONT SIZE=2>> list_head? If you just used the struct, then you wouldn't need this:</FONT>
<BR><FONT SIZE=2>></FONT>
</P>
<P><FONT SIZE=2>Done. Using struct list_head instead of pointer</FONT>
</P>
<BR>
<BR>
<P><FONT SIZE=2>> > + u16 in_use;</FONT>
<BR><FONT SIZE=2>> > };</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> I can't find anywhere that the in_use flag is used.</FONT>
<BR><FONT SIZE=2>></FONT>
</P>
<P><FONT SIZE=2>Removed</FONT>
</P>
<BR>
<P><FONT SIZE=2>> > +static int srp_map_fmr(struct srp_target_port *target, struct scatterlist *scat,</FONT>
<BR><FONT SIZE=2>> > + int sg_cnt, struct srp_request *req)</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> [...]</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > + return -ENOMEM;</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> > + } else if (fmr_cnt <= 0) {</FONT>
<BR><FONT SIZE=2>> </FONT>
<BR><FONT SIZE=2>> fmr_cnt is unsigned so I think this is going to get you in trouble.</FONT>
<BR><FONT SIZE=2>> Might as well make fmr_cnt a plain int to make things simpler.</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
<P><FONT SIZE=2>In previous patch, fmr_cnt was already declared as int</FONT>
</P>
<P><FONT SIZE=2>> Also, it might be good to try and add some more comments explaining</FONT>
<BR><FONT SIZE=2>> srp_map_fmr() -- it would definitely help me review.</FONT>
<BR><FONT SIZE=2>> </FONT>
</P>
<P><FONT SIZE=2>I added some comments - Hope they help your review (instead </FONT>
<BR><FONT SIZE=2>of confusing you more :))</FONT>
</P>
<P><FONT SIZE=2>Signed-off-by: Vu Pham <vu@mellanox.com></FONT>
</P>
<BR>
<BR>
<BR>
</BODY>
</HTML>