> -----Original Message----- > From: David Miller [mailto:davem@xxxxxxxxxxxxx] > Sent: Saturday, March 8, 2014 3:18 AM > To: KY Srinivasan > Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; > jasowang@xxxxxxxxxx > Subject: Re: [PATCH V2 1/6] Drivers: net: hyperv: Enable scatter gather I/O > > From: "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx> > Date: Thu, 6 Mar 2014 21:32:36 -0800 > > > +static u32 fill_pg_buf(struct page *page, u32 offset, u32 len, > > + struct hv_page_buffer *pb) > > +{ > > + int j = 0; > > + > > + /* Deal with compund pages by ignoring unused part > > + * of the page. > > + */ > > + page += offset >> PAGE_SHIFT; > > Please only one space between "offset" and ">>" > > > + offset &= ~PAGE_MASK; > > + > > + while (len > 0) { > > + unsigned long bytes; > > + > > + bytes = PAGE_SIZE - offset; > > + if (bytes > len) > > + bytes = len; > > + pb[j].pfn = page_to_pfn(page); > > + pb[j].offset = offset; > > + pb[j].len = bytes; > > + > > + offset += bytes; > > + len -= bytes; > > + > > + if (offset == PAGE_SIZE && len) { > > + page++; > > + offset = 0; > > + j++; > > + } > > + } > > + > > + return j + 1; > > +} > > I think this function has some edge case errors. > > As I understand it, this function returns how many page buffer entries were > filled in. > > But if we fill exactly the end of a page, we will report one too many in the > return value. I may be missing something here; but in the case you are describing we would return a value of one as we should. When offset + len is exactly equal to the page size, we would not increment the value of j, since at that point len == 0. Since the initial value of j was 0 and we return (j + 1), we will return 1. Regards, K. Y > > Consider an "offset" of X, where X is smaller than PAGE_SIZE. And a "len" > which is exactly "PAGE_SIZE - offset". > > We will fill in exactly one page buffer entry, however we will erroneously > return 2 from this function. > > I believe the fix is to first replace the test at the end of the loop > with: > > page++; > offset = 0; > j++; > > And subsequently change the function to just return plain "j". _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
