On Wed, 28 Oct 2009, Pete Zaitcev wrote:
> On Tue, 27 Oct 2009 15:29:15 -0400 (EDT), Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> > This patch (as1299) fixes a bug in an error-handling path of usbmon's
> > binary interface. The storage area for URB data is divided into
> > fixed-size blocks. If an URB's data can't be copied, the area
> > reserved for it should be decreased to the size of the truncated
> > information (rounded up to a block boundary). Rounding up the amount
> > to be removed and subtracting it from the reserved size is definitely
> > the wrong thing to do.
>
> Right, I see. Still, the solution seems a bit heavy handed.
> I find it easier to reason about mon_buff_area_shrink if it remains
> a compliment to mon_buff_area_free.
>
> > Also, when the data for an isochronous URB can't be copied, we can
> > still copy the isoc packet descriptors. In fact the current code does
> > copy the descriptors, but then sets the capture length to 0 so they
> > become inaccessible. The capture length should be reduced to the
> > length of the descriptors, not set to 0.
>
> That part is fine, I see that we should do it.
>
> How about the attached? It should be doing exactly what your patch
> did, only reformatted a bit, and keeps mon_buff_area_shrink focused.
Your version is fine, as far as it goes.
> -- Pete
>
> diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
> index 9ed3e74..6469a8d 100644
> --- a/drivers/usb/mon/mon_bin.c
> +++ b/drivers/usb/mon/mon_bin.c
> @@ -537,8 +538,10 @@ static void mon_bin_event(struct mon_reader_bin *rp, struct urb *urb,
> if (length != 0) {
> ep->flag_data = mon_bin_get_data(rp, offset, urb, length);
> if (ep->flag_data != 0) { /* Yes, it's 0x00, not '0' */
> - ep->len_cap = 0;
> - mon_buff_area_shrink(rp, length);
> + delta = (ep->len_cap + PKT_ALIGN-1) & ~(PKT_ALIGN-1);
> + delta -= (lendesc + PKT_ALIGN-1) & ~(PKT_ALIGN-1);
> + ep->len_cap = lendesc;
> + mon_buff_area_shrink(rp, delta);
However in a later patch, I change mon_bin_get_data() so that it gives
its caller an explicit "remainder" value. When no data is copied,
remainder is naturally equal to length. But if only part of a
scatter-gather buffer can be copied then remainder would be smaller
than length.
For this reason, it would be better to orient this code around what is
missing instead of what is kept. For example:
- ep->len_cap = 0;
- mon_buff_area_shrink(rp, length);
+ delta = (ep->len_cap + PKT_ALIGN-1) & ~(PKT_ALIGN-1);
+ ep->len_cap -= length;
+ delta -= (ep->len_cap + PKT_ALIGN-1) & ~(PKT_ALIGN-1);
+ mon_buff_area_shrink(rp, delta);
How's that?
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
