| Message ID | edbcbe3f-9b5d-cead-5885-27bc55ef550b@lwfinger.net (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
On 04.02.2017 21:41, Larry Finger wrote: > On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: >> Seems the problem is caused by rtl92c_dm_*() casting .priv to "struct >> rtl_pci_priv", while it is "struct rtl_usb_priv". > > Those routines are shared by rtl8192ce and rtl8192cu, thus we need to make that > difference in cast to be immaterial. I think we need to move "struct > bt_coexist_info" to the beginning of both rtlpci_priv and rtl_usb_priv. Then it > should not matter. > > I do not have a gcc version new enough to turn KASAN testing on, thus the > attached patch is only compile tested. Does it fix the problem? Thank you for the patch, it indeed fixes the bug. I noticed that struct rtl_priv contains .btcoexist, isn't it duplicated in the struct rtl_pci_priv?
On 02/04/2017 01:32 PM, Dmitry Osipenko wrote: > On 04.02.2017 21:41, Larry Finger wrote: >> On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: >>> Seems the problem is caused by rtl92c_dm_*() casting .priv to "struct >>> rtl_pci_priv", while it is "struct rtl_usb_priv". >> >> Those routines are shared by rtl8192ce and rtl8192cu, thus we need to make that >> difference in cast to be immaterial. I think we need to move "struct >> bt_coexist_info" to the beginning of both rtlpci_priv and rtl_usb_priv. Then it >> should not matter. >> >> I do not have a gcc version new enough to turn KASAN testing on, thus the >> attached patch is only compile tested. Does it fix the problem? > > Thank you for the patch, it indeed fixes the bug. > > I noticed that struct rtl_priv contains .btcoexist, isn't it duplicated in the > struct rtl_pci_priv? Thanks for testing. When I submit the patch, is it OK to cite your reporting and testing? Yes, the bt_coexist_info structure is in two different places. I will change the code in rtl8192c-common and rtl8192ce to use only the one in rtlpriv. That should satisfy the problem you reported, as well as clean up the code. Thanks again, Larry
On 05.02.2017 04:05, Larry Finger wrote: > On 02/04/2017 01:32 PM, Dmitry Osipenko wrote: >> On 04.02.2017 21:41, Larry Finger wrote: >>> On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: >>>> Seems the problem is caused by rtl92c_dm_*() casting .priv to "struct >>>> rtl_pci_priv", while it is "struct rtl_usb_priv". >>> >>> Those routines are shared by rtl8192ce and rtl8192cu, thus we need to make that >>> difference in cast to be immaterial. I think we need to move "struct >>> bt_coexist_info" to the beginning of both rtlpci_priv and rtl_usb_priv. Then it >>> should not matter. >>> >>> I do not have a gcc version new enough to turn KASAN testing on, thus the >>> attached patch is only compile tested. Does it fix the problem? >> >> Thank you for the patch, it indeed fixes the bug. >> >> I noticed that struct rtl_priv contains .btcoexist, isn't it duplicated in the >> struct rtl_pci_priv? > > Thanks for testing. When I submit the patch, is it OK to cite your reporting and > testing? Sure, Tested-by: Dmitry Osipenko <digetx@gmail.com> > Yes, the bt_coexist_info structure is in two different places. I will change the > code in rtl8192c-common and rtl8192ce to use only the one in rtlpriv. That > should satisfy the problem you reported, as well as clean up the code. > > Thanks again, Good, thank you. BTW, I have an issue with the 8192cu: WiFi stops to work after a while (3-15 minutes) if I enable WMM QoS on the AP. There is nothing suspicious in KMSG, connection is up but no packets go in/out. I tried to enable debug messages in the driver, so when the WiFi stops to work I see that some "temperature/led" notify still going on in the driver, but nothing happens when I try to initiate a transfer (say to open a web page) - the log is silent, like the requests are getting stuck/dropped somewhere before reaching the driver. Is it a known issue? With the QoS disabled everything works hunky-dory, however I get 2x-4x faster download speed with QoS enabled (while it works.) I noticed that rtl92c_init_edca_param() isn't wired in the driver, so I suppose the QoS isn't implemented yet, right? If it is an expected behaviour, I think at least printing a warning message in the KMSG like "QoS unimplemented, you may expect problems" should be good enough to avoid confusion.
On Sat, 2017-02-04 at 12:41 -0600, Larry Finger wrote: > On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: > > Seems the problem is caused by rtl92c_dm_*() casting .priv to > > "struct > > rtl_pci_priv", while it is "struct rtl_usb_priv". > > Those routines are shared by rtl8192ce and rtl8192cu, thus we need to > make that > difference in cast to be immaterial. I think we need to move "struct > bt_coexist_info" to the beginning of both rtlpci_priv and > rtl_usb_priv. Then it > should not matter. I think you really should consider putting a struct rtl_common into that or something, and getting rid of all the casting that causes this problem to start with? johannes
On 02/06/2017 04:29 AM, Johannes Berg wrote: > On Sat, 2017-02-04 at 12:41 -0600, Larry Finger wrote: >> On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: >>> Seems the problem is caused by rtl92c_dm_*() casting .priv to >>> "struct >>> rtl_pci_priv", while it is "struct rtl_usb_priv". >> >> Those routines are shared by rtl8192ce and rtl8192cu, thus we need to >> make that >> difference in cast to be immaterial. I think we need to move "struct >> bt_coexist_info" to the beginning of both rtlpci_priv and >> rtl_usb_priv. Then it >> should not matter. > > I think you really should consider putting a struct rtl_common into > that or something, and getting rid of all the casting that causes this > problem to start with? The fix you suggest is prepared and will be submitted soon. As it is much more invasive with ~150 insertions and ~160 deletions, I decided not to have it be the one that is pushed to all stable kernels from 4.0 onward. Larry
Am Montag, 6. Februar 2017, 09:45:31 CET schrieb Larry Finger: > On 02/06/2017 04:29 AM, Johannes Berg wrote: > > On Sat, 2017-02-04 at 12:41 -0600, Larry Finger wrote: > >> On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: > >>> Seems the problem is caused by rtl92c_dm_*() casting .priv to > >>> "struct > >>> rtl_pci_priv", while it is "struct rtl_usb_priv". > >> > >> Those routines are shared by rtl8192ce and rtl8192cu, thus we need to > >> make that > >> difference in cast to be immaterial. I think we need to move "struct > >> bt_coexist_info" to the beginning of both rtlpci_priv and > >> rtl_usb_priv. Then it > >> should not matter. > > > > I think you really should consider putting a struct rtl_common into > > that or something, and getting rid of all the casting that causes this > > problem to start with? > > The fix you suggest is prepared and will be submitted soon. As it is much > more invasive with ~150 insertions and ~160 deletions, I decided not to > have it be the one that is pushed to all stable kernels from 4.0 onward. > > Larry This is possibly related to the following Fedora Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1391987
On 07.02.2017 19:45, Tobias Guggenmos wrote: > Am Montag, 6. Februar 2017, 09:45:31 CET schrieb Larry Finger: >> On 02/06/2017 04:29 AM, Johannes Berg wrote: >>> On Sat, 2017-02-04 at 12:41 -0600, Larry Finger wrote: >>>> On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: >>>>> Seems the problem is caused by rtl92c_dm_*() casting .priv to >>>>> "struct >>>>> rtl_pci_priv", while it is "struct rtl_usb_priv". >>>> >>>> Those routines are shared by rtl8192ce and rtl8192cu, thus we need to >>>> make that >>>> difference in cast to be immaterial. I think we need to move "struct >>>> bt_coexist_info" to the beginning of both rtlpci_priv and >>>> rtl_usb_priv. Then it >>>> should not matter. >>> >>> I think you really should consider putting a struct rtl_common into >>> that or something, and getting rid of all the casting that causes this >>> problem to start with? >> >> The fix you suggest is prepared and will be submitted soon. As it is much >> more invasive with ~150 insertions and ~160 deletions, I decided not to >> have it be the one that is pushed to all stable kernels from 4.0 onward. >> >> Larry > > This is possibly related to the following Fedora Bug: > https://bugzilla.redhat.com/show_bug.cgi?id=1391987 > Bug only affects USB adapters (8192cu), PCIe (8192ce) should be fine. The Fedora bug sounds like the one I have with the enabled AP QoS.
On 02/07/2017 10:45 AM, Tobias Guggenmos wrote: > Am Montag, 6. Februar 2017, 09:45:31 CET schrieb Larry Finger: >> On 02/06/2017 04:29 AM, Johannes Berg wrote: >>> On Sat, 2017-02-04 at 12:41 -0600, Larry Finger wrote: >>>> On 02/04/2017 10:58 AM, Dmitry Osipenko wrote: >>>>> Seems the problem is caused by rtl92c_dm_*() casting .priv to >>>>> "struct >>>>> rtl_pci_priv", while it is "struct rtl_usb_priv". >>>> >>>> Those routines are shared by rtl8192ce and rtl8192cu, thus we need to >>>> make that >>>> difference in cast to be immaterial. I think we need to move "struct >>>> bt_coexist_info" to the beginning of both rtlpci_priv and >>>> rtl_usb_priv. Then it >>>> should not matter. >>> >>> I think you really should consider putting a struct rtl_common into >>> that or something, and getting rid of all the casting that causes this >>> problem to start with? >> >> The fix you suggest is prepared and will be submitted soon. As it is much >> more invasive with ~150 insertions and ~160 deletions, I decided not to >> have it be the one that is pushed to all stable kernels from 4.0 onward. >> >> Larry > > This is possibly related to the following Fedora Bug: > https://bugzilla.redhat.com/show_bug.cgi?id=1391987 This bug is unlikely to be the cause of that problem. In fact, this bug only affects rtl8192cu, not rtl8192ce. The RedHat problem is more likely caused by the not-yet-merged patch entitled "rtlwifi: rtl8192ce: Fix loading of incorrect firmware". Larry
diff --git a/drivers/net/wireless/realtek/rtlwifi/pci.h b/drivers/net/wireless/realtek/rtlwifi/pci.h index 578b1d9..d9039ea 100644 --- a/drivers/net/wireless/realtek/rtlwifi/pci.h +++ b/drivers/net/wireless/realtek/rtlwifi/pci.h @@ -271,10 +271,10 @@ struct mp_adapter { }; struct rtl_pci_priv { + struct bt_coexist_info bt_coexist; + struct rtl_led_ctl ledctl; struct rtl_pci dev; struct mp_adapter ndis_adapter; - struct rtl_led_ctl ledctl; - struct bt_coexist_info bt_coexist; }; #define rtl_pcipriv(hw) (((struct rtl_pci_priv *)(rtl_priv(hw))->priv)) diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.h b/drivers/net/wireless/realtek/rtlwifi/usb.h index a6d43d2..cdb9e06 100644 --- a/drivers/net/wireless/realtek/rtlwifi/usb.h +++ b/drivers/net/wireless/realtek/rtlwifi/usb.h @@ -146,8 +146,9 @@ struct rtl_usb { }; struct rtl_usb_priv { - struct rtl_usb dev; + struct bt_coexist_info bt_coexist; struct rtl_led_ctl ledctl; + struct rtl_usb dev; }; #define rtl_usbpriv(hw) (((struct rtl_usb_priv *)(rtl_priv(hw))->priv))