On Wed, Feb 13, 2019 at 12:00:42PM +0100, Lorenzo Bianconi wrote: > On Feb 13, Stanislaw Gruszka wrote: > > On Tue, Feb 12, 2019 at 11:09:03PM +0100, Lorenzo Bianconi wrote: > > > > > On Tue, Feb 12, 2019 at 04:08:37PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > What unnecessary operation ? > > > > > > > > > > > > > > > > the ones in mt76u_fill_bulk_urb() > > > > > > > > > > > > > > Your patches also add extra operations on hotpath due to urb->num_sgs and > > > > > > > dev->usb.sg_en checks. > > > > > > > > > > > > here I guess you should use mt76u_check_sg() instead of > > > > > > udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in > > > > > > the hotpath > > > > > > > > > > It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest > > > > > of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to > > > > > usb host driver, what is fine. > > > > > > > > > > > Moreover the RFC series has been tested by multiple users and on multiple > > > > > > devices > > > > > > > > > > I know. Perhaps you could test my patch on rpi ? > > > > > > > > sure, I will do it later > > > > > > I confirm that even with Stanislaw's patch the usb dongle is properly working > > > on rpi3+ > > > > Thanks for testing. Would be ok to you to post my patch against > > wireless-drivers tree and cc stable as fix for non-SG usb hosts, > > drop your set for -next and work for fix for SG issue on AMD IOMMU? > > I agree that your patch works (since it does not use SG I/O :)) but It still use SG in mt76usb driver (urb->sg, sg_set_page(), sg_init_marker(), etc ...) for all usb host controllers. It just not submit urb->num_sgs > 1 to USB host driver if it does not support SG. > I think it is more clear (and manageable) to have two separated > routines for memory allocation. Hmm, I disagree on that and don't understand how you consider clearer and manageable design with two separate buffer allocation methods and bunch of extra ->num_sgs and ->sg_en checks, compered to solution with one allocation method and without those checks. Also some additional cleanups/simplification can be done after applying my patch in mt76usb not possible after applying your set. > Moreover I think that this check has to be done in the control plane instead > of the data plane, so I would like to spend some more time in order to see if it is > possible to remove some checks in the hot-path I'm not sure what you mean by data-pane and control-plane in this context. I considered to do mt76u_fill_buk_urb: urb->num_sgs = (buf->num_sgs > 1) ? buf->num_sgs : 0; Instead of usb->bus->sg_tablesize check. It should work, but it's not what usb_sg_init() does and as pointed by Alan that function always do the right thing, so I just copied code from there. Beside that I don't think this check in mt76u_fill_buk_urb() affects performance whatsoever. Stanislaw
