On Sun, 6 Dec 2015, Steinar H. Gunderson wrote:
> >>> You don't really need to do it earlier. An unnecessary calculation of
> >>> num_sgs won't hurt.
> >> Is it unnecessary? I thought the test was really to force num_sgs==0 for the
> >> DMA case, not to save a little arithmetic.
> > Well, if you calculate num_sgs and then force it to 0 anyway, the
> > calculation was unnecessary, right?
>
> But the logic never calculates num_sgs if usbm == NULL? The if terminates
> early and num_sgs stays 0.
At this point you don't know whether as->usbm will end up being NULL.
> >> - num_sgs = DIV_ROUND_UP(uurb->buffer_length, USB_SG_SIZE);
> >> - if (num_sgs == 1 || num_sgs > ps->dev->bus->sg_tablesize)
> >> - num_sgs = 0;
> >> + /* do not use SG buffers when memory mapped segments
> >> + * are in use
> >> + */
> >> + if (as->usbm) {
> >> + num_sgs = DIV_ROUND_UP(uurb->buffer_length,
> >> + USB_SG_SIZE);
> >> + if (num_sgs == 1 ||
> >> + num_sgs > ps->dev->bus->sg_tablesize) {
> >> + num_sgs = 0;
> >> + }
> >> + }
> > No, no. Leave this part of the code unchanged. as hasn't even been
> > allocated yet.
>
> Good point. I did warn you about the lack of testing... :-)
>
> By “unchanged”, do you mean unchanged from previous patch or from the
> original? Because this was here all along from Markus' version of the patch.
Unchanged from the original. After all, your patch gets applied to the
original code, not on top of Markus's patch or any of your earlier
patches. Therefore anything your patch doesn't touch will remain the
same as the original code, not the same as in Markus's patch.
> > As pointed out repeatedly by the kbuild test robot, this should be
> > IS_ERR, not IS_ERR_VALUE. Also, you need to set as->usbm back to NULL
> > before jumping.
> >
> > At this point, set num_sgs to 0 if as->usbm is non-NULL.
>
> Didn't you just point out that this would be redundant calculation?
No, I pointed out that the calculation of num_sgs earlier would be
unnecessary. But it's also unavoidable, since you don't know at the
time whether it will turn out to be necessary or not.
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
