Re: [PATCH net-next 0/3] r8152: configuration setting

Hayes Wang [off-list ref] writes:

Bjørn Mork [mailto:bjorn@mork.no]

quoted

Sent: Wednesday, September 07, 2016 9:51 PM

[...]

quoted

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.

quoted

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/<port>/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

Bjørn Mork [mailto:bjorn@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/<port>/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

On Thu, 2016-09-08 at 13:02 +0000, 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