| Message ID | cover.1547549771.git.lorenzo.bianconi@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | do not use sg if not properly supported by usb controller | expand |
On Tue, Jan 15, 2019 at 01:33:41PM +0100, Lorenzo Bianconi wrote: > 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. Some users have reported sg issues on AMD IOMMU Not sure what is the problem , but this patch set look like a workaround not fix. If this an issue with IOMMU and sg, seems there is something wrong in sg page mappings eigher on mt76 dirver or IOMMU driver. If things need to be fixed in mt76 I whould check if page mappings for sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO version do not use sg for framgments, so most likely USB don't need it as well. Thanks Stanislaw
> On Tue, Jan 15, 2019 at 01:33:41PM +0100, Lorenzo Bianconi wrote: > > 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. Some users have reported sg issues on AMD IOMMU Hi Stanislaw, > > Not sure what is the problem , but this patch set look like a workaround > not fix. If this an issue with IOMMU and sg, seems there is something wrong > in sg page mappings eigher on mt76 dirver or IOMMU driver. The main point here I guess is we do not need sg if fragment number is one (e.g usb2.0). Moreover this can fix IOMMU reported issues. @Rosen: could you please try this series enabling IOMMU? > > If things need to be fixed in mt76 I whould check if page mappings for > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO > version do not use sg for framgments, so most likely USB don't need it > as well. usb scatter-gather is used to properly support non-linear skbs (A-MSDU, with usb3.0) since the hw (unlike pci counterpart) does not support it, so we need it. Regards, Lorenzo > > Thanks > Stanislaw
On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote: > Hi Stanislaw, Hi :-) > > Not sure what is the problem , but this patch set look like a workaround > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > The main point here I guess is we do not need sg if fragment number is one (e.g > usb2.0). Moreover this can fix IOMMU reported issues. So there if diffrence for USB host driver when we have one usb->sg sengment and if we just pass the buffer via urb->transfer_buf . I think most USB host drivers behave the same in such cases. For what USB hardware/driver this is needed ? Perhaps simpler fix could be done in USB host driver? Also I'm not sure for what this new module parameter is needed ? > @Rosen: could you please try this series enabling IOMMU? > > > > > If things need to be fixed in mt76 I whould check if page mappings for > > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO > > version do not use sg for framgments, so most likely USB don't need it > > as well. > > usb scatter-gather is used to properly support non-linear skbs (A-MSDU, > with usb3.0) since the hw (unlike pci counterpart) does not support it, > so we need it. Ok, having separe URB for each fragment make no sense since we have embedded sg structure in URB. Regards Stanislaw
> On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote: > > Hi Stanislaw, > > Hi :-) > > > > Not sure what is the problem , but this patch set look like a workaround > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > > > The main point here I guess is we do not need sg if fragment number is one (e.g > > usb2.0). Moreover this can fix IOMMU reported issues. > > So there if diffrence for USB host driver when we have one usb->sg > sengment and if we just pass the buffer via urb->transfer_buf . I think > most USB host drivers behave the same in such cases. For what USB > hardware/driver this is needed ? Perhaps simpler fix could be done > in USB host driver? According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557 single sg urb and urb with a configured transfer_buf are managed in a different way. Please not I have not received any confirm that this series fixes the reported issue yet :) > > Also I'm not sure for what this new module parameter is needed ? > This is used to disable sg on usb3.0 since it is enabled by default > > @Rosen: could you please try this series enabling IOMMU? > > > > > > > > If things need to be fixed in mt76 I whould check if page mappings for > > > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO > > > version do not use sg for framgments, so most likely USB don't need it > > > as well. > > > > usb scatter-gather is used to properly support non-linear skbs (A-MSDU, > > with usb3.0) since the hw (unlike pci counterpart) does not support it, > > so we need it. > > Ok, having separe URB for each fragment make no sense since we have > embedded sg structure in URB. In this case each fragment will be mapped independently I guess Regards, Lorenzo > > Regards > Stanislaw
On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote: > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote: > > > Hi Stanislaw, > > > > Hi :-) > > > > > > Not sure what is the problem , but this patch set look like a workaround > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > > > > > The main point here I guess is we do not need sg if fragment number is one (e.g > > > usb2.0). Moreover this can fix IOMMU reported issues. > > > > So there if diffrence for USB host driver when we have one usb->sg > > sengment and if we just pass the buffer via urb->transfer_buf . I think > > most USB host drivers behave the same in such cases. For what USB > > hardware/driver this is needed ? Perhaps simpler fix could be done > > in USB host driver? > > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557 > single sg urb and urb with a configured transfer_buf are managed in a different way. But this should not make any difference for underlying low level USB host driver, since we map 1 buffer of the same size, just by using different routines for that. > Please not I have not received any confirm that this series fixes the reported issue > yet :) What is reported issue ? Thanks Stanislaw
On Jan 16, Stanislaw Gruszka wrote: > On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote: > > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote: > > > > Hi Stanislaw, > > > > > > Hi :-) > > > > > > > > Not sure what is the problem , but this patch set look like a workaround > > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > > > > > > > The main point here I guess is we do not need sg if fragment number is one (e.g > > > > usb2.0). Moreover this can fix IOMMU reported issues. > > > > > > So there if diffrence for USB host driver when we have one usb->sg > > > sengment and if we just pass the buffer via urb->transfer_buf . I think > > > most USB host drivers behave the same in such cases. For what USB > > > hardware/driver this is needed ? Perhaps simpler fix could be done > > > in USB host driver? > > > > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557 > > single sg urb and urb with a configured transfer_buf are managed in a different way. > > But this should not make any difference for underlying low level USB > host driver, since we map 1 buffer of the same size, just by using > different routines for that. probably amd iommu has some constraints on sg buffer layout respect to intel one, not sure > > > Please not I have not received any confirm that this series fixes the reported issue > > yet :) > > What is reported issue ? https://marc.info/?l=linux-wireless&m=154716096506037&w=2 Regards, Lorenzo > > Thanks > Stanislaw
On Wed, Jan 16, 2019 at 02:40:46PM +0100, Lorenzo Bianconi wrote: > On Jan 16, Stanislaw Gruszka wrote: > > On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote: > > > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote: > > > > > Hi Stanislaw, > > > > > > > > Hi :-) > > > > > > > > > > Not sure what is the problem , but this patch set look like a workaround > > > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > > > > > > > > > The main point here I guess is we do not need sg if fragment number is one (e.g > > > > > usb2.0). Moreover this can fix IOMMU reported issues. > > > > > > > > So there if diffrence for USB host driver when we have one usb->sg > > > > sengment and if we just pass the buffer via urb->transfer_buf . I think > > > > most USB host drivers behave the same in such cases. For what USB > > > > hardware/driver this is needed ? Perhaps simpler fix could be done > > > > in USB host driver? > > > > > > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557 > > > single sg urb and urb with a configured transfer_buf are managed in a different way. > > > > But this should not make any difference for underlying low level USB > > host driver, since we map 1 buffer of the same size, just by using > > different routines for that. > > probably amd iommu has some constraints on sg buffer layout respect to intel > one, not sure > > > > > > Please not I have not received any confirm that this series fixes the reported issue > > > yet :) > > > > What is reported issue ? > > https://marc.info/?l=linux-wireless&m=154716096506037&w=2 So, this if for AMD IOMMU issue as I thought initally. For the moment I thought you are trying to fix some diffrent problem with some non-standart usb host controler. I still think not using sg is just workaround for the problem not worth to do, since we have already workaround in form of disabling IOMMU. Right fix will be fixing AMD IOMMU driver or fix sg usage in mt76-usb if it does something wrong. Regards Stanislaw
> On Wed, Jan 16, 2019 at 02:40:46PM +0100, Lorenzo Bianconi wrote: > > On Jan 16, Stanislaw Gruszka wrote: > > > On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote: > > > > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote: > > > > > > Hi Stanislaw, > > > > > > > > > > Hi :-) > > > > > > > > > > > > Not sure what is the problem , but this patch set look like a workaround > > > > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > > > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > > > > > > > > > > > The main point here I guess is we do not need sg if fragment number is one (e.g > > > > > > usb2.0). Moreover this can fix IOMMU reported issues. > > > > > > > > > > So there if diffrence for USB host driver when we have one usb->sg > > > > > sengment and if we just pass the buffer via urb->transfer_buf . I think > > > > > most USB host drivers behave the same in such cases. For what USB > > > > > hardware/driver this is needed ? Perhaps simpler fix could be done > > > > > in USB host driver? > > > > > > > > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557 > > > > single sg urb and urb with a configured transfer_buf are managed in a different way. > > > > > > But this should not make any difference for underlying low level USB > > > host driver, since we map 1 buffer of the same size, just by using > > > different routines for that. > > > > probably amd iommu has some constraints on sg buffer layout respect to intel > > one, not sure > > > > > > > > > Please not I have not received any confirm that this series fixes the reported issue > > > > yet :) > > > > > > What is reported issue ? > > > > https://marc.info/?l=linux-wireless&m=154716096506037&w=2 > > So, this if for AMD IOMMU issue as I thought initally. For the moment I > thought you are trying to fix some diffrent problem with some > non-standart usb host controler. Why not standard? Most of net usb drivers do not use sg :) > > I still think not using sg is just workaround for the problem not worth > to do, since we have already workaround in form of disabling IOMMU. > Right fix will be fixing AMD IOMMU driver or fix sg usage in mt76-usb > if it does something wrong. I agree with you that this is not strictly a fix for IOMMU issue but I think we could avoid using sg when we are working just with linear skbs. But first I guess we need some feedbacks from users Regards, Lorenzo > > Regards > Stanislaw
On Tue, Jan 15, 2019 at 7:47 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > > On Tue, Jan 15, 2019 at 01:33:41PM +0100, Lorenzo Bianconi wrote: > > > 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. Some users have reported sg issues on AMD IOMMU > > Hi Stanislaw, > > > > > Not sure what is the problem , but this patch set look like a workaround > > not fix. If this an issue with IOMMU and sg, seems there is something wrong > > in sg page mappings eigher on mt76 dirver or IOMMU driver. > > The main point here I guess is we do not need sg if fragment number is one (e.g > usb2.0). Moreover this can fix IOMMU reported issues. > > @Rosen: could you please try this series enabling IOMMU? I will try it later today. Note that the same behavior occurs with USB 2.0 ports as well. I do feel that the actual bug is in the IOMMU driver. > > > > > If things need to be fixed in mt76 I whould check if page mappings for > > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO > > version do not use sg for framgments, so most likely USB don't need it > > as well. > > usb scatter-gather is used to properly support non-linear skbs (A-MSDU, > with usb3.0) since the hw (unlike pci counterpart) does not support it, > so we need it. > > Regards, > Lorenzo > > > > > Thanks > > Stanislaw
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. Some users have reported sg issues on AMD IOMMU Lorenzo Bianconi (4): mt76: usb: move mt76u_check_sg in usb.c mt76: usb: do not use sg buffer for fw upload 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 | 9 +- .../wireless/mediatek/mt76/mt76x2/usb_init.c | 2 +- drivers/net/wireless/mediatek/mt76/usb.c | 132 +++++++++++++----- drivers/net/wireless/mediatek/mt76/usb_mcu.c | 5 +- 6 files changed, 107 insertions(+), 57 deletions(-)