Re: [PATCH net-next 0/3] r8152: configuration setting
Hayes Wangwrites: > Bjørn Mork [mailto:bj...@mork.no] >> Sent: Thursday, September 08, 2016 3:55 PM > [...] >> Yes, I see that. But is that strictly necessary? Couldn't you just say: >> "CDC ECM is supported by cdc_ether and therefore limited to the features >> implemented by cdc_ether. If you want feature X, then please use our >> vendor specific mode with the r8152 driver?" > > My customs have a case that they must force the speed to 100M > for some reasons. I also wish to implement the driver as simple > as possible, but I don't think I could determine this. I accept > you reject my patches. However, I couldn't deny the requests > from the boss or customs without doing anything. I must prove > the way is unacceptable. That's an odd combination of requirements, but I know how customers can be :) Just to make it clear: I provide comments, but I am in no position to reject any of your patches. Or have any wish to do so. You maintain r8152. Oliver maintains cdc_ether. I am confident that whatever you two decide is going to be fine. > [...] >> Each USB configuation comes with a set of descriptors identifying the >> functions, and USB interface drivers attach to the functions they >> support. The user can dynamically switch the device from e.g. cfg #1 to >> cfg #3 by writing "3" to /sys/bus/usb/devices//bConfigurationValue >> This will cause the ECM and ACM USB interfaces to disappear, and the >> associated class drivers will unbind, and new vendor specific USB >> interfaces appear instead, causing a matching vendor specific driver to >> load and bind. >> >> Naturally, end users will not switch configurations all the time. They >> will select the configuration providing the set of functions they want. >> If this is different from the default configuration selected by the >> Linux USB core, then that's a simple udev rule to update the >> bConfigurationValue sysfs attribute on device disceovery. > > I tested above method before. And I found that the cdc_ether > was loaded before switching the configuration. The behavior > of loading one driver and changing to another driver has > opportunity to let our some previous chips become abnormal. > To switch configuration is fine. However, it may have problem > to switch driver. That is why the current kernel only supports > vendor mode. If the method works fine, I have no trouble now. Yes, many firmwares/devices are not prepared to do "late" config switching and can end up in a strange limbo if they are initialized before switching. An udev rule should still run early enough to prevent this problem I believe. But if that doesn't work, then I agree that a blacklist makes more sense. Just make it runtime configurable so that distros can do something sane with it. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] r8152: configuration setting
On Thu, 2016-09-08 at 13:02 +, Hayes Wang wrote: > I tested above method before. And I found that the cdc_ether > was loaded before switching the configuration. The behavior > of loading one driver and changing to another driver has > opportunity to let our some previous chips become abnormal. > To switch configuration is fine. However, it may have problem > to switch driver. That is why the current kernel only supports > vendor mode. If the method works fine, I have no trouble now. We may talk about creating extensions to cdc-ether, if they are needed for the phy. What is unacceptable is to override the configuration mechanism in the kernel. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next 0/3] r8152: configuration setting
Bjørn Mork [mailto:bj...@mork.no] > Sent: Thursday, September 08, 2016 3:55 PM [...] > Yes, I see that. But is that strictly necessary? Couldn't you just say: > "CDC ECM is supported by cdc_ether and therefore limited to the features > implemented by cdc_ether. If you want feature X, then please use our > vendor specific mode with the r8152 driver?" My customs have a case that they must force the speed to 100M for some reasons. I also wish to implement the driver as simple as possible, but I don't think I could determine this. I accept you reject my patches. However, I couldn't deny the requests from the boss or customs without doing anything. I must prove the way is unacceptable. [...] > Each USB configuation comes with a set of descriptors identifying the > functions, and USB interface drivers attach to the functions they > support. The user can dynamically switch the device from e.g. cfg #1 to > cfg #3 by writing "3" to /sys/bus/usb/devices//bConfigurationValue > This will cause the ECM and ACM USB interfaces to disappear, and the > associated class drivers will unbind, and new vendor specific USB > interfaces appear instead, causing a matching vendor specific driver to > load and bind. > > Naturally, end users will not switch configurations all the time. They > will select the configuration providing the set of functions they want. > If this is different from the default configuration selected by the > Linux USB core, then that's a simple udev rule to update the > bConfigurationValue sysfs attribute on device disceovery. I tested above method before. And I found that the cdc_ether was loaded before switching the configuration. The behavior of loading one driver and changing to another driver has opportunity to let our some previous chips become abnormal. To switch configuration is fine. However, it may have problem to switch driver. That is why the current kernel only supports vendor mode. If the method works fine, I have no trouble now. Best Regards, Hayes
Re: [PATCH net-next 0/3] r8152: configuration setting
Hayes Wangwrites: > Bjørn Mork [mailto:bj...@mork.no] >> Sent: Wednesday, September 07, 2016 9:51 PM > [...] >> So this adds a lot of code to work around the issues you introduced by >> unnecessarily blacklisting the CDC ECM configuration earlier, and still >> makes the r8152 driver handle the device even in ECM mode. > > I suggest to use vendor mode only, but some people ask me to > submit such patches. If these patches are rejected, I have > enough reasons to tell them it is unacceptable rather than > I don't do it. > >> Just remove the completely unnecessary blacklist, and let the cdc_ether >> driver handle the device if the user selects the ECM configuration. >> That't how the USB system works. There is no need for any code in r8152 >> to do that. > > The pure cdc_ether driver couldn't change the speed of the > ethernet, because it doesn't know how to access the PHY of > the device. Therefore, I add relative code in r8152 driver. Yes, I see that. But is that strictly necessary? Couldn't you just say: "CDC ECM is supported by cdc_ether and therefore limited to the features implemented by cdc_ether. If you want feature X, then please use our vendor specific mode with the r8152 driver?" As for the dynamic switching of multi-configuration USB devices: This is not special for your device. It is very common for e.g. LTE modems to provide 2 or more configurations, allowing the user to select between for example 1: ECM class + ACM class functions 2: MBIM class function 3: vendor specific functions Each USB configuation comes with a set of descriptors identifying the functions, and USB interface drivers attach to the functions they support. The user can dynamically switch the device from e.g. cfg #1 to cfg #3 by writing "3" to /sys/bus/usb/devices//bConfigurationValue This will cause the ECM and ACM USB interfaces to disappear, and the associated class drivers will unbind, and new vendor specific USB interfaces appear instead, causing a matching vendor specific driver to load and bind. Naturally, end users will not switch configurations all the time. They will select the configuration providing the set of functions they want. If this is different from the default configuration selected by the Linux USB core, then that's a simple udev rule to update the bConfigurationValue sysfs attribute on device disceovery. This is how the USB core works by *default*, as long as you do not deliberately break it by blacklisting any functions or forcing a specific configuration. You are overdesigning to solve a non-existing problem. Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next 0/3] r8152: configuration setting
David Miller [mailto:da...@davemloft.net] > Sent: Thursday, September 08, 2016 8:38 AM [...] > By forcing a certain mode via a Kconfig value, you are basically making it > impossible for distributions to do something reasonable here. The request is always from some manufacturers, not end users. They always ask the driver to control everything. And I don't think the end user knows or cares about how the device is set. That is why I try to satisfy them via Kconfig. I think you have rejected this way. I would think if there is any other method. Thanks. Best Regards, Hayes -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next 0/3] r8152: configuration setting
Bjørn Mork [mailto:bj...@mork.no] > Sent: Wednesday, September 07, 2016 9:51 PM [...] > So this adds a lot of code to work around the issues you introduced by > unnecessarily blacklisting the CDC ECM configuration earlier, and still > makes the r8152 driver handle the device even in ECM mode. I suggest to use vendor mode only, but some people ask me to submit such patches. If these patches are rejected, I have enough reasons to tell them it is unacceptable rather than I don't do it. > Just remove the completely unnecessary blacklist, and let the cdc_ether > driver handle the device if the user selects the ECM configuration. > That't how the USB system works. There is no need for any code in r8152 > to do that. The pure cdc_ether driver couldn't change the speed of the ethernet, because it doesn't know how to access the PHY of the device. Therefore, I add relative code in r8152 driver. Best Regards, Hayes
Re: [PATCH net-next 0/3] r8152: configuration setting
From: Hayes WangDate: Wed, 7 Sep 2016 16:12:19 +0800 > Some people prefer to use ECM mode rather than vendor mode. Therefore, I add > CONFIG_RTL8152_CONFIG_VALUE in Kconfig. Then, the users could choose the USB > configuration value which they want. The default is to support vendor mode > only. By forcing a certain mode via a Kconfig value, you are basically making it impossible for distributions to do something reasonable here. This needs to be somehow a runtime thing, and I'm pretty sure we've had many other situations of USB devices which want to be run by different "personality" drivers in the past. Perhaps we can do something generically for USB devices for this kind of situation and make r8152 use it. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next 0/3] r8152: configuration setting
[ CCing Oliver, who AFAIK still is the cdc_ether maintainer and should have the final word on this ] Hayes Wangwrites: > Some people prefer to use ECM mode rather than vendor mode. Therefore, I add > CONFIG_RTL8152_CONFIG_VALUE in Kconfig. Then, the users could choose the USB > configuration value which they want. The default is to support vendor mode > only. > > Hayes Wang (3): > r8152: check hw version first > r8152: support ECM mode > r8152: add CONFIG_RTL8152_CONFIG_VALUE > > drivers/net/usb/Kconfig | 13 ++ > drivers/net/usb/r8152.c | 383 > +--- > 2 files changed, 345 insertions(+), 51 deletions(-) So this adds a lot of code to work around the issues you introduced by unnecessarily blacklisting the CDC ECM configuration earlier, and still makes the r8152 driver handle the device even in ECM mode. Sorry, but this is a total mess. Just remove the completely unnecessary blacklist, and let the cdc_ether driver handle the device if the user selects the ECM configuration. That't how the USB system works. There is no need for any code in r8152 to do that. Ref https://lkml.org/lkml/2014/1/3/57 Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html