| Message ID | cover.1549977282.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | do not use sg if not properly supported by usb controller | expand |
On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > From: Lorenzo Bianconi <lorenzo@kernel.org> > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > {tx,rx} datapath if the usb controller has sg data length constraints. > Moreover add disable_usb_sg module parameter in order to explicitly > disable scatter-gather. SG I/O is not supported by all host drivers and > some users have reported sg issues on AMD IOMMU. Again. This is not right approach. SG issues should be fixed not workarounded. > This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+ > - rebased on top of 'fix multiple issues in mt76u error path' > https://patchwork.kernel.org/cover/10804919/ > > Lorenzo Bianconi (4): > mt76: usb: move mt76u_check_sg in usb.c > mt76: usb: do not use sg buffers for mcu messages > mt76: usb: use a linear buffer for tx/rx datapath if sg is not > supported > mt76: usb: introduce disable_usb_sg parameter > > drivers/net/wireless/mediatek/mt76/mt76.h | 14 +- > .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +- > .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +- > .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +- > drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++----- > drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +- > 6 files changed, 105 insertions(+), 54 deletions(-) I really think my approach is simpler. Diffstat from my proposed patch is: drivers/net/wireless/mediatek/mt76/mt76.h | 1 + drivers/net/wireless/mediatek/mt76/usb.c | 55 ++++++++++++++--------- 2 files changed, 36 insertions(+), 20 deletions(-) Also this fix bug(s), presumably regression for mt76x0, should be backported. Stanislaw
(repost with corrected Lorenzo email) On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote: > On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > > From: Lorenzo Bianconi <lorenzo@kernel.org> > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > > {tx,rx} datapath if the usb controller has sg data length constraints. > > Moreover add disable_usb_sg module parameter in order to explicitly > > disable scatter-gather. SG I/O is not supported by all host drivers and > > some users have reported sg issues on AMD IOMMU. > > Again. This is not right approach. SG issues should be fixed > not workarounded. > > > This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+ > > > - rebased on top of 'fix multiple issues in mt76u error path' > > https://patchwork.kernel.org/cover/10804919/ > > > > Lorenzo Bianconi (4): > > mt76: usb: move mt76u_check_sg in usb.c > > mt76: usb: do not use sg buffers for mcu messages > > mt76: usb: use a linear buffer for tx/rx datapath if sg is not > > supported > > mt76: usb: introduce disable_usb_sg parameter > > > > drivers/net/wireless/mediatek/mt76/mt76.h | 14 +- > > .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +- > > .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +- > > .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +- > > drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++----- > > drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +- > > 6 files changed, 105 insertions(+), 54 deletions(-) > > I really think my approach is simpler. Diffstat from my proposed patch is: > > drivers/net/wireless/mediatek/mt76/mt76.h | 1 + > drivers/net/wireless/mediatek/mt76/usb.c | 55 ++++++++++++++--------- > 2 files changed, 36 insertions(+), 20 deletions(-) > > Also this fix bug(s), presumably regression for mt76x0, should be > backported. > > Stanislaw >
> > (repost with corrected Lorenzo email) > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote: > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > > > From: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > > > {tx,rx} datapath if the usb controller has sg data length constraints. > > > Moreover add disable_usb_sg module parameter in order to explicitly > > > disable scatter-gather. SG I/O is not supported by all host drivers and > > > some users have reported sg issues on AMD IOMMU. > > > > Again. This is not right approach. SG issues should be fixed > > not workarounded. Hi Stanislaw, here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do not see how I am working around the issue. Moreover with this approach we avoid some unnecessary operation in the hotpath Regards, Lorenzo > > > > > This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+ > > > > > - rebased on top of 'fix multiple issues in mt76u error path' > > > https://patchwork.kernel.org/cover/10804919/ > > > > > > Lorenzo Bianconi (4): > > > mt76: usb: move mt76u_check_sg in usb.c > > > mt76: usb: do not use sg buffers for mcu messages > > > mt76: usb: use a linear buffer for tx/rx datapath if sg is not > > > supported > > > mt76: usb: introduce disable_usb_sg parameter > > > > > > drivers/net/wireless/mediatek/mt76/mt76.h | 14 +- > > > .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +- > > > .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +- > > > .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +- > > > drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++----- > > > drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +- > > > 6 files changed, 105 insertions(+), 54 deletions(-) > > > > I really think my approach is simpler. Diffstat from my proposed patch is: > > > > drivers/net/wireless/mediatek/mt76/mt76.h | 1 + > > drivers/net/wireless/mediatek/mt76/usb.c | 55 ++++++++++++++--------- > > 2 files changed, 36 insertions(+), 20 deletions(-) > > > > Also this fix bug(s), presumably regression for mt76x0, should be > > backported. > > > > Stanislaw > >
On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote: > > > > (repost with corrected Lorenzo email) > > > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote: > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > > > > From: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > > > > {tx,rx} datapath if the usb controller has sg data length constraints. > > > > Moreover add disable_usb_sg module parameter in order to explicitly > > > > disable scatter-gather. SG I/O is not supported by all host drivers and > > > > some users have reported sg issues on AMD IOMMU. > > > > > > Again. This is not right approach. SG issues should be fixed > > > not workarounded. > > Hi Stanislaw, > > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do > not see how I am working around the issue. By avoiding SG buffer allocation and configuration which most likely need to be fixed. > Moreover with this approach we avoid some unnecessary operation in the hotpath What unnecessary operation ? Stanislaw
> On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote: > > > > > > (repost with corrected Lorenzo email) > > > > > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote: > > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > > > > > {tx,rx} datapath if the usb controller has sg data length constraints. > > > > > Moreover add disable_usb_sg module parameter in order to explicitly > > > > > disable scatter-gather. SG I/O is not supported by all host drivers and > > > > > some users have reported sg issues on AMD IOMMU. > > > > > > > > Again. This is not right approach. SG issues should be fixed > > > > not workarounded. > > > > Hi Stanislaw, > > > > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do > > not see how I am working around the issue. > > By avoiding SG buffer allocation and configuration which most likely > need to be fixed. In my series I: 1- set num_sg to 0 2- use transfer_buffer please correct me if I am wrong but in your solution you did the same since AFAIK PageHighMem is always 0 so you end up setting num_sg to 0 and using transfer_buffer as well. Is my understanding correct? > > > Moreover with this approach we avoid some unnecessary operation in the hotpath > > What unnecessary operation ? the ones in mt76u_fill_bulk_urb() Regards, Lorenzo > > Stanislaw
On Tue, Feb 12, 2019 at 03:25:30PM +0100, Lorenzo Bianconi wrote: > > On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote: > > > > > > > > (repost with corrected Lorenzo email) > > > > > > > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote: > > > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > > > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > > > > > > {tx,rx} datapath if the usb controller has sg data length constraints. > > > > > > Moreover add disable_usb_sg module parameter in order to explicitly > > > > > > disable scatter-gather. SG I/O is not supported by all host drivers and > > > > > > some users have reported sg issues on AMD IOMMU. > > > > > > > > > > Again. This is not right approach. SG issues should be fixed > > > > > not workarounded. > > > > > > Hi Stanislaw, > > > > > > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do > > > not see how I am working around the issue. > > > > By avoiding SG buffer allocation and configuration which most likely > > need to be fixed. > > In my series I: > 1- set num_sg to 0 > 2- use transfer_buffer > > please correct me if I am wrong but in your solution you did the same since AFAIK > PageHighMem is always 0 so you end up setting num_sg to 0 and using > transfer_buffer as well. Is my understanding correct? Yes. But it still using all existing SG allocation and setup code and buffer is tracked in urb->sg[0]. > > > Moreover with this approach we avoid some unnecessary operation in the hotpath > > > > 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. Stanislaw
> On Tue, Feb 12, 2019 at 03:25:30PM +0100, Lorenzo Bianconi wrote: > > > On Tue, Feb 12, 2019 at 03:09:53PM +0100, Lorenzo Bianconi wrote: > > > > > > > > > > (repost with corrected Lorenzo email) > > > > > > > > > > On Tue, Feb 12, 2019 at 02:45:50PM +0100, Stanislaw Gruszka wrote: > > > > > > On Tue, Feb 12, 2019 at 02:24:47PM +0100, LorenzoBianconilorenzo@kernel.org wrote: > > > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org> > > > > > > > > > > > > > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > > > > > > > {tx,rx} datapath if the usb controller has sg data length constraints. > > > > > > > Moreover add disable_usb_sg module parameter in order to explicitly > > > > > > > disable scatter-gather. SG I/O is not supported by all host drivers and > > > > > > > some users have reported sg issues on AMD IOMMU. > > > > > > > > > > > > Again. This is not right approach. SG issues should be fixed > > > > > > not workarounded. > > > > > > > > Hi Stanislaw, > > > > > > > > here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do > > > > not see how I am working around the issue. > > > > > > By avoiding SG buffer allocation and configuration which most likely > > > need to be fixed. > > > > In my series I: > > 1- set num_sg to 0 > > 2- use transfer_buffer > > > > please correct me if I am wrong but in your solution you did the same since AFAIK > > PageHighMem is always 0 so you end up setting num_sg to 0 and using > > transfer_buffer as well. Is my understanding correct? > > Yes. But it still using all existing SG allocation and setup code and > buffer is tracked in urb->sg[0]. both of them are from page_frag_alloc() > > > > > Moreover with this approach we avoid some unnecessary operation in the hotpath > > > > > > 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 Moreover the RFC series has been tested by multiple users and on multiple devices Regards, Lorenzo > > Stanislaw
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 ? Stanislaw
> 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 Regards, Lorenzo > > Stanislaw > >
> > 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+ Regards, Lorenzo > > Regards, > Lorenzo > > > > > Stanislaw > > > >
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? Stanislaw
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 I think it is more clear (and manageable) to have two separated routines for memory allocation. 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 Regards, Lorenzo > > Stanislaw
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
On 2019-02-12 14:42, lorenzo@kernel.org wrote: > From: Lorenzo Bianconi <lorenzo@kernel.org> > > Use linear fragment and not a single usb scatter-gather buffer in mt76u > {tx,rx} datapath if the usb controller has sg data length constraints. > Moreover add disable_usb_sg module parameter in order to explicitly > disable scatter-gather. SG I/O is not supported by all host drivers and > some users have reported sg issues on AMD IOMMU. > This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+ > > Changes since RFC: > - rebased on top of 'fix multiple issues in mt76u error path' > https://patchwork.kernel.org/cover/10804919/ > > I am resending the series since the first attempt seems to be rejected by > the ML Applied, thanks. - Felix
From: Lorenzo Bianconi <lorenzo@kernel.org> Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU. This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+ Changes since RFC: - rebased on top of 'fix multiple issues in mt76u error path' https://patchwork.kernel.org/cover/10804919/ I am resending the series since the first attempt seems to be rejected by the ML Lorenzo Bianconi (4): mt76: usb: move mt76u_check_sg in usb.c mt76: usb: do not use sg buffers for mcu messages mt76: usb: use a linear buffer for tx/rx datapath if sg is not supported mt76: usb: introduce disable_usb_sg parameter drivers/net/wireless/mediatek/mt76/mt76.h | 14 +- .../net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +- .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 3 +- .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +- drivers/net/wireless/mediatek/mt76/usb.c | 133 +++++++++++++----- drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +- 6 files changed, 105 insertions(+), 54 deletions(-)